pgwire: auto-upgrade password authentication with SCRAM hashes (scram 9/10)#74849
pgwire: auto-upgrade password authentication with SCRAM hashes (scram 9/10)#74849craig[bot] merged 3 commits intomasterfrom
Conversation
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @knz)
pkg/sql/pgwire/auth_methods.go, line 233 at r3 (raw file):
} // passwordAuthenticator is the authenticator function for the
nit: passwordAuthenticator -> scramAuthenticator
pkg/sql/pgwire/auth_methods.go, line 494 at r3 (raw file):
// The authenticator function needs a closure to retrieve the same information. // Memoize it. newpwfn := func(ctx context.Context) (bool, security.PasswordHash, error) { return expired, hashedPassword, err }
so just to confirm, both expired and err are checked later when newpwfn is called? perhaps a comment here that reiterates that would help
pkg/sql/pgwire/auth_methods.go, line 499 at r3 (raw file):
if err == nil && hashedPassword.Method() == security.HashSCRAMSHA256 { // Yes: use a SCRAM-SHA-256 exchange. c.LogAuthInfof(ctx, "no client cert but SCRAM credentials found, proceeding with SCRAM-SHA-256")
i wonder if the logging here assumes too much. currently this is only called in the cert-password mode, but if the code is refactored and this gets used elsewhere, the "no client cert" part of the message might be confusing.
in other words, can we have c.LogAuthInfof(no client cert ...) in the authCertPassword function, and keep c.LogAuthInfof(proceeding with SCRAM ...) in this function? (ditto for c.LogAuthInfof(requesting cleartext ...)
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/auth_methods.go, line 233 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: passwordAuthenticator -> scramAuthenticator
Fixed.
pkg/sql/pgwire/auth_methods.go, line 494 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
so just to confirm, both
expiredanderrare checked later whennewpwfnis called? perhaps a comment here that reiterates that would help
Good call. Done.
pkg/sql/pgwire/auth_methods.go, line 499 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i wonder if the logging here assumes too much. currently this is only called in the
cert-passwordmode, but if the code is refactored and this gets used elsewhere, the "no client cert" part of the message might be confusing.in other words, can we have
c.LogAuthInfof(no client cert ...)in theauthCertPasswordfunction, and keepc.LogAuthInfof(proceeding with SCRAM ...)in this function? (ditto forc.LogAuthInfof(requesting cleartext ...)
Great idea. Done.
fdff4d7 to
7ca14c9
Compare
rafiss
left a comment
There was a problem hiding this comment.
lgtm!
Reviewed 8 of 8 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 just lifts the closures into named functions and gives them explicit argument lists. Release note: None
Release note: None
Release note (security update): When using the HBA authentication method `cert-password` (the default) for SQL client connections, and the SQL client does not present a TLS client certificate to the server, CockroachDB now automatically upgrades the password handshake protocol to use SCRAM-SHA-256 if the user's stored password uses the SCRAM encoding. The previous behavior of requesting a cleartext password is still used if the stored password is encoded using the CRDB-BCRYPT format. An operator can force to _always_ request SCRAM-SHA-256 when a TLS client cert is not provided (so as to guarantee the various security benefits of SCRAM) using the authentication methods `cert-scram-sha-256` (either TLS client cert _or_ SCRAM-SHA-256); and `scram-sha-256` (only SCRAM-SHA-256). (As previously, mandatory cleartext password authentication can be requested, e.g. for debugging purposes, by using the HBA method `password`.) This automatic protocol upgrade can be manually disabled using the new cluster setting `server.user_login.cert_password_method.auto_scram_promotion.enable` (by setting it to `false`), for example if certain client drivers are found to not support SCRAM-SHA-256 authentication properly. Release note (sql change): When possible, CockroachDB will now automatically require the PostgreSQL-compatible SCRAM-SHA-256 protocol when performing password validation when SQL client log in. (Reminder: this mechanism is not used when SQL clients use TLS client certs, which is the recommended approach.) This assumes support for SCRAM-SHA-256 in client drivers. As of 2020, we have found this to be prevalent in the pg driver ecosystem. However, users should be mindful of the following possible behavior changes: - an application that tries to detect whether password verification has failed by checking server error messages, might observe different error messages with SCRAM-SHA-256. Those checks, if present, need to be updated. - if a client driver simply does not support SCRAM-SHA-256 at all, the operator retains the option to set the cluster setting `server.user_login.cert_password_method.auto_scram_promotion.enable` to `false` to force the previous password verification method instead.
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 #74848; next PR in series: #74850)
Epic CRDB-5349
Release note (security update): When using the HBA authentication
method
cert-password(the default) for SQL client connections, andthe SQL client does not present a TLS client certificate to the
server, CockroachDB now automatically upgrades the password handshake
protocol to use SCRAM-SHA-256 if the user's stored password uses the
SCRAM encoding.
The previous behavior of requesting a cleartext password is still used
if the stored password is encoded using the CRDB-BCRYPT format.
An operator can force to always request SCRAM-SHA-256 when a TLS
client cert is not provided (so as to guarantee the various
security benefits of SCRAM) using the authentication methods
cert-scram-sha-256(either TLS client cert or SCRAM-SHA-256); andscram-sha-256(only SCRAM-SHA-256).(As previously, mandatory cleartext password authentication
can be requested, e.g. for debugging purposes, by using the HBA
method
password.)This automatic protocol upgrade can be manually disabled
using the new cluster setting
server.user_login.cert_password_method.auto_scram_promotion.enable(bysetting it to
false), for example if certain client driversare found to not support SCRAM-SHA-256 authentication properly.
Release note (sql change): When possible, CockroachDB will now
automatically require the PostgreSQL-compatible SCRAM-SHA-256 protocol
when performing password validation when SQL client log in. (Reminder:
this mechanism is not used when SQL clients use TLS client certs,
which is the recommended approach.)
This assumes support for SCRAM-SHA-256 in client drivers.
As of 2020, we have found this to be prevalent in the pg driver
ecosystem. However, users should be mindful of the following possible
behavior changes:
an application that tries to detect whether password verification
has failed by checking server error messages, might observe
different error messages with SCRAM-SHA-256. Those checks, if
present, need to be updated.
if a client driver simply does not support SCRAM-SHA-256 at all,
the operator retains the option to set the cluster setting
server.user_login.cert_password_method.auto_scram_promotion.enableto
falseto force the previous password verification method instead.