pgwire,hba: introduce the 'trust' and 'reject' auth methods#43731
pgwire,hba: introduce the 'trust' and 'reject' auth methods#43731craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
7295e1b to
5192fab
Compare
pkg/sql/pgwire/hba/conf.rl
Outdated
| } | ||
| host = | ||
| 'host' %newHost ws | ||
| ('host' %newHost | 'local' %newLocal) ws |
There was a problem hiding this comment.
The local connection type, as defined by the postgres docs, only has 3 following fields as opposed to host which has 4 or 5. I would prefer not adding local until our parser correctly supports the expected number of fields that local needs.
I'm reaching the point now where I no longer think ragel is a good fit for this problem. It was an experiment, and I consider it failed. It takes a long time to figure out how to use the language, and even then it is very easy to make blind mistakes. When we need to extend it 6-12 months later (as in here), it is difficult for the reviewer (me in this case, even though I was the original author) to remember how ragel works. The change you made a few days ago that correctly parsed single-letter comma-separated things for example, I didn't really understand what was going on, but it passed the tests so I hit accept.
Unless you really strongly disagree, or have a clean way to add the correct parsing for local, I suggest removing this addition.
https://blog.cloudflare.com/incident-report-on-memory-leak-caused-by-cloudflare-parser-bug/ seems like cloudflare had similar problems with ragel wrt maintainability.
There was a problem hiding this comment.
Oh, I meant to say that I'm very on board with rewriting this parser in Go. It's not a super complicated language. It's annoying since it's sometimes space-separated except with double quotes. But I believe we could look at the pg implementation for some quick guidance about a well-tested way to write our parser.
pkg/sql/pgwire/hba_conf.go
Outdated
|
|
||
| // AnyVersion can be passed as minReqVersion to RegisterAuthMethod() | ||
| // to indicate there is no constraint on the cluster version. | ||
| const AnyVersion cluster.VersionKey = -1 |
There was a problem hiding this comment.
I have a medium preference to remove this and force all auth methods to specify a real version. Can we use whatever the 19.2.0 release version was without breaking anybody? This would make it harder to misuse this feature (i.e., if someone copy-pasted an auth method with AnyVersion in it) and remove the need for special code that handles AnyVersion. Again, only medium preference, so if you feel that keeping it is better, that's fine too.
5192fab to
89e72bc
Compare
knz
left a comment
There was a problem hiding this comment.
RFAL.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mjibson)
pkg/sql/pgwire/hba_conf.go, line 301 at r3 (raw file):
Previously, mjibson (Matt Jibson) wrote…
I have a medium preference to remove this and force all auth methods to specify a real version. Can we use whatever the 19.2.0 release version was without breaking anybody? This would make it harder to misuse this feature (i.e., if someone copy-pasted an auth method with AnyVersion in it) and remove the need for special code that handles AnyVersion. Again, only medium preference, so if you feel that keeping it is better, that's fine too.
Done.
pkg/sql/pgwire/hba/conf.rl, line 150 at r3 (raw file):
Previously, mjibson (Matt Jibson) wrote…
Oh, I meant to say that I'm very on board with rewriting this parser in Go. It's not a super complicated language. It's annoying since it's sometimes space-separated except with double quotes. But I believe we could look at the pg implementation for some quick guidance about a well-tested way to write our parser.
Done.
89e72bc to
db05674
Compare
|
TFYR! bors r+ |
Build failed |
The 'trust' and 'reject' methods, as their name implies, unconditionally allow and deny authentication of matching connections. This patch introduces them as a prerequisite to later work on Unix socket authentication, but also to introduce a bit of infrastructure that binds new auth methods to a minimum required cluster version. This new infrastructure ensures that future new auth methods do not risk hosting a mixed-version cluster. The patch also introduces support for the 'local' rule prefix, which is still unused. Release note (security): CockroachDB now supports the authentication methods 'trust' and 'reject' in the cluster setting server.host_based_authentication.configuration; their meaning is to unconditionally allow and deny matching connection attempts.
db05674 to
3fe7e3c
Compare
|
spurious error; retrying bors r+ |
43731: pgwire,hba: introduce the 'trust' and 'reject' auth methods r=knz a=knz First commits from #43734 and #43726. The 'trust' and 'reject' methods, as their name implies, unconditionally allow and deny authentication of matching connections. This patch introduces them as a prerequisite to later work on Unix socket authentication, but also to introduce a bit of infrastructure that binds new auth methods to a minimum required cluster version. This new infrastructure ensures that future new auth methods do not risk hosting a mixed-version cluster. (See discussion on #43717.) The patch also introduces support for the 'local' rule prefix, which is still unused. Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Build succeeded |
|
(used to fix #31113) |
First commits from #43734 and #43726.
The 'trust' and 'reject' methods, as their name implies,
unconditionally allow and deny authentication of matching connections.
This patch introduces them as a prerequisite to later work on Unix
socket authentication, but also to introduce a bit of infrastructure
that binds new auth methods to a minimum required cluster
version. This new infrastructure ensures that future new auth methods
do not risk hosting a mixed-version cluster. (See discussion on #43717.)
The patch also introduces support for the 'local' rule prefix, which
is still unused.