pgwire,sqlproxyccl: base support for a SCRAM authentication flow (scram 2/10)#74842
pgwire,sqlproxyccl: base support for a SCRAM authentication flow (scram 2/10)#74842craig[bot] merged 2 commits intomasterfrom
Conversation
|
This change is |
|
Comment from @jeffswenson here:
Yes, we are cognizant of this cost, and we knew about when we started this discussion all the way back. We're ok with this given that we're pushing our customers to co-locate their SQL clients in the same cloud / network. |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cameronnunez, @jeffswenson, and @knz)
pkg/sql/pgwire/auth_methods.go, line 65 at r1 (raw file):
RegisterAuthMethod("scram-sha-256", authScram, hba.ConnAny, chainOptions( requireClusterVersion(clusterversion.SCRAMAuthentication),
i don't follow why we need the cluster version check here. could the cluster version gate be applied at the time of the SET CLUSTER SETTING server.host_based_authentication.configuration statement? having the gate apply at write-time seems safest, and i think if we do it there we already have access to the current clusterversion
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/auth_methods.go, line 65 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i don't follow why we need the cluster version check here. could the cluster version gate be applied at the time of the
SET CLUSTER SETTING server.host_based_authentication.configurationstatement? having the gate apply at write-time seems safest, and i think if we do it there we already have access to the current clusterversion
This is exactly what is happening! This "chainOptions" argument to RegisterAuthMethods places the function into the hbaCheckHBAEntries map, which is used to validate the value before it's written, in checkHBASyntaxBeforeUpdatingSetting(). That function is the pre-write validation function passed in the setting constructor.
|
@rafiss wondering if you'd like anything else to happen here? |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/auth_methods.go, line 65 at r1 (raw file):
Previously, knz (kena) wrote…
This is exactly what is happening! This "chainOptions" argument to
RegisterAuthMethodsplaces the function into thehbaCheckHBAEntriesmap, which is used to validate the value before it's written, incheckHBASyntaxBeforeUpdatingSetting(). That function is the pre-write validation function passed in the setting constructor.
ah ok, thanks for explaining. in that case, a test that shows this would be nice
we can bring back a mixed version logic test configuration (which is straightforward; see c3a154b) then add an onlyif local-mixed-21.2-22.1 logic test somewhere
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/auth_methods.go, line 65 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
ah ok, thanks for explaining. in that case, a test that shows this would be nice
we can bring back a mixed version logic test configuration (which is straightforward; see c3a154b) then add an
onlyif local-mixed-21.2-22.1logic test somewhere
Amazing. Thank you so much for teaching me this.
Done.
|
RFAL |
jeffswenson
left a comment
There was a problem hiding this comment.
sqlproxy changes LGTM
Reviewed 4 of 11 files at r1, 4 of 9 files at r3, 2 of 7 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status:
rafiss
left a comment
There was a problem hiding this comment.
lgtm!
Reviewed 4 of 11 files at r1, 4 of 9 files at r3, 2 of 7 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status:
|
NB: I'm holding off merging this PR until all the PRs in the sequence are ready (otherwise, the entire chain will be auto-closed by github). |
This commit introduces support for the HBA auth methods 'scram-sha-256' and `'cert-scram-sha-256'. They are gated behind a new cluster version. The method callback is merely plumbed in here. There is no implementation yet (it generates an unsupported error that redirects to the issue about implementing the feature). Release note: None
At this time, it is merely able to authenticate a fixed user. We are OK having this imperfect commit in the git history, because the feature at this point is not enabled by default. A user would need to opt into it manually by modifying the HBA configuration. Release note: None
74301: sql,server: support SCRAM authentication for SQL sessions r=rafiss,JeffSwenson,bdarnell,aaron-crl,kpatron-cockroachlabs a=knz Fixes #42519. Fixes #74511. Informs #65117. Epic CRDB-5349 **tldr:** this PR adds support for the SCRAM-SHA-256 authentication method, as defined by IETF RFCs [5802](https://datatracker.ietf.org/doc/html/rfc5802) and [7677](https://datatracker.ietf.org/doc/html/rfc7677) and as supported by PostgreSQL. This offers [multiple security benefits](#scram-benefits) over CockroachDB's current use of cleartext password authentication. To use this feature, 1) the stored password hashes must use the SCRAM encoding (this requires a migration away from crdb's bcrypt-based hashes); and 2) one of the SCRAM authentication methods must be enabled explictly via the HBA configuration (cluster setting `server.host_based_authentication.configuration`). ### How to review this work The addition of the feature goes as follows: 1. adding HBA syntax and authentication method hooks for `scram-sha-256` and `cert-scram-sha-256`. These are gated behind a new cluster version, so that previous-version nodes do not get confused by the new syntax. Split into separate PR: #74842 2. extending ALTER/CREATE USER/ROLE WITH PASSWORD to recognize SCRAM hashes. This allows operators to use these SQL statements to populate SCRAM hashes. (This should also be seen as the more desirable way to configure SQL user accounts: it ensures that the CockroachDB node never ever sees the cleartext password of an account. Even if we later support computing the SCRAM hash server-side, pre-computing the hash client-side should be seen and documented as the superior approach.) Split into separate PR: #74843 3. extending the existing cleartext methods inside CockroachDB (used for the HTTP interface and the `password`-based methods in SQL) to compare a cleartext password to a pre-computed SCRAM hash. This ensures that the existing password mechanisms that are cleartext-based continue to work even after the stored credentials start using the SCRAM encoding. Split into separate PRs: #74844 and #74845 4. extending the code created at step 1 to actually use the stored SCRAM credentials for client authentication. This achieves the main goal. Split into separate PRs: #74846 and #74847 5. making the hash method used to encode cleartext passwords passed to ALTER/CREATE USER/ROLE WITH PASSWORD configurable, using a new cluster setting `server.user_login.password_encryption`, and exposing its value via the pg-compatible session variable `password_encryption`. This is a convenience feature - properly secured deployments should not need this, as they should use pre-hashing client-side instead (see note at point 2 above). Split into separate PR: #74848 6. making crdb auto-select SCRAM authn if the stored password uses SCRAM. This is one step to ensuring that previous-version clusters are automatically opted into SCRAM. Split into separate PR: #74849 7. Auto-upgrade the stored credentials from bcrypt to SCRAM, when enabled and possible. Split into separate PR: #74850 The reviewer is invited to consider each PR in turn, read its commit message(s) to understand the goal of that PR, then review the PR to ascertain that the implementation (and relevant tests) match the goals announced in the commit message. Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
(PR peeled away from #74301; previous PR in series #74841; next PR in series: #74843)
Epic CRDB-5349
This commit introduces support for the HBA auth methods
'scram-sha-256' and `'cert-scram-sha-256'. They are gated behind a new
cluster version.
The method callback is merely plumbed in here. There is no
implementation yet (it generates an unsupported error that redirects
to the issue about implementing the feature).
At this time, the implementation in pgwire is merely able to authenticate a fixed user.
We are OK having this imperfect commit in the git history, because
the feature at this point is not enabled by default. A user would
need to opt into it manually by modifying the HBA configuration.