sql,security: create a password hash abstraction (scram 4/10)#74844
sql,security: create a password hash abstraction (scram 4/10)#74844craig[bot] merged 1 commit intomasterfrom
Conversation
|
This change is |
|
@rafiss this is the part you were already reviewing in #74301, extracted here as a single commit for your reviewing convenience. cc @catj-cockroach , this is what I told you about. |
6243645 to
db869c0
Compare
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)
pkg/security/password.go, line 349 at r1 (raw file):
} func makeSCRAMHash(storedHash []byte, parts [][]byte, invalidHash PasswordHash) PasswordHash {
can this function document the order of the parts slice? would it be too cumbersome to make it accept 4 different []byte params or a struct, so that it's self-documenting instead?
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/security/password.go, line 349 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
can this function document the order of the
partsslice? would it be too cumbersome to make it accept 4 different[]byteparams or a struct, so that it's self-documenting instead?
Changed the data type to hide the details. This is not meant to be used without isSCRAMHash().
(I split the two upon a suggestion by @jeffswenson in this comment but the latter is not meant to be used standalone)
|
@rafiss anything else you'd like to happen here? |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/security/password.go, line 331 at r2 (raw file):
// isSCRAMHash() to makeSCRAMHash(), so that the latter cannot be // legitimately used without the former. type scramParts [][]byte
what do you think about making this
type scramParts [5][]byte
pkg/security/password.go, line 335 at r2 (raw file):
func isSCRAMHash(inputPassword []byte) (bool, scramParts) { parts := scramHashRe.FindSubmatch(inputPassword) return parts != nil, scramParts(parts)
and then here we can convert the parts with
if len(parts) != 5 {
return false, scramParts{}
}
return true, *(*scramParts)(parts)
(new in go 1.17 https://tip.golang.org/ref/spec#Conversions_from_slice_to_array_pointer)
pkg/security/password.go, line 357 at r2 (raw file):
// previous call to isSCRAMHash(). func makeSCRAMHash(storedHash []byte, parts scramParts, invalidHash PasswordHash) PasswordHash { iters, err := strconv.Atoi(string(parts[1]))
could we add a few functions on the scramParts type?
func (sp scramParts) getIters (int, error) {
return strconv.Atoi(string(sp[1]))
}
func (sp scramParts) getSalt (string, error) {
return base64.StdEncoding.DecodeString(string(sp[2]))
}
func (sp scramParts) getStoredKey (string, error) {
return base64.StdEncoding.DecodeString(string(sp[3]))
}
func (sp scramParts) getServerKey (string, error) {
return base64.StdEncoding.DecodeString(string(sp[4]))
}
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/security/password.go, line 331 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
what do you think about making this
type scramParts [5][]byte
Done.
pkg/security/password.go, line 335 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
and then here we can convert the parts with
if len(parts) != 5 { return false, scramParts{} } return true, *(*scramParts)(parts)(new in go 1.17 https://tip.golang.org/ref/spec#Conversions_from_slice_to_array_pointer)
Done.
pkg/security/password.go, line 357 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
could we add a few functions on the
scramPartstype?func (sp scramParts) getIters (int, error) { return strconv.Atoi(string(sp[1])) } func (sp scramParts) getSalt (string, error) { return base64.StdEncoding.DecodeString(string(sp[2])) } func (sp scramParts) getStoredKey (string, error) { return base64.StdEncoding.DecodeString(string(sp[3])) } func (sp scramParts) getServerKey (string, error) { return base64.StdEncoding.DecodeString(string(sp[4])) }
Done.
rafiss
left a comment
There was a problem hiding this comment.
looks great!
Reviewed 5 of 11 files at r1, 6 of 6 files at r3, all commit messages.
Reviewable status:
jeffswenson
left a comment
There was a problem hiding this comment.
LGTM
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). |
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/security/password.go, line 335 at r2 (raw file):
Previously, knz (kena) wrote…
Done.
FYI: it works in the code, but the linter hates it
lint_test.go:1740: err=exit status 2, stderr=panic: in github.com/cockroachdb/cockroach/pkg/security.isSCRAMHash: cannot convert Load <[][]byte> t0 ([][]byte) to *github.com/cockroachdb/cockroach/pkg/security.scramParts
goroutine 5661 [running]:
honnef.co/go/tools/go/ir.emitConv(0xc0a64a30e0, {0x974b68, 0xc0ab54e300}, {0x9656e0, 0xc03bb74c70}, {0x964e20, 0xc009336000})
/go/src/github.com/cockroachdb/cockroach/vendor/honnef.co/go/tools/go/ir/emit.go:261 +0xb25
honnef.co/go/tools/go/ir.(*builder).expr0(0xc0aa34db00, 0xc0a64a30e0, {0x966a00, 0xc009336000}, {0x7, {0x9656e0, 0xc03bb74c70}, {0x0, 0x0}})
/go/src/github.com/cockroachdb/cockroach/vendor/honnef.co/go/tools/go/ir/builder.go:544 +0xa77
Prior to this commit, password hashes loaded from storage were simply represented as byte arrays. The in-RAM credential cache was then simply storing these byte arrays. Keeping this simple encoding is unfortunate for the SCRAM authentication format, because it forces the password check code to re-parse the 4 parameters of the SCRAM credentials upon every authentication event. Instead, we want to pre-parse the parameters once, and store them pre-parsed in the credential cache. To achieve this, this commit introduces a new `security.PasswordHash` to replace `[]byte` to represent password hashes in memory. A stored byte array can be converted to a `security.PasswordHash` using the new `security.LoadPasswordHash()` function. Additionally, this commit performs the following cleanups: - it renames `security.CompareHashAndPassword` to `CompareHashAndCleartextPassword` to clarify the format the password is expected in. - it makes `CompareHashAndCleartextPassword()` return a boolean alongside the error. The function does not return an error any more if the hash is valid but the credentials don't match. Callers are now expected to propagate the returned error, if any. - it un-exports `security.ErrPasswordUserAuthFailed`, which was a formatting string, and replaces it by a new error constructor `NewErrPasswordUserAuthFailed` (this was meant to be used to construct errors originally). NB: this commit merely adds a new abstraction in the APIs. SCRAM hashes are still not used for authentication. 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 #74843; next PR in series: #74845)
Epic CRDB-5349
Prior to this commit, password hashes loaded from storage
were simply represented as byte arrays.
The in-RAM credential cache was then simply storing these byte arrays.
Keeping this simple encoding is unfortunate for the SCRAM
authentication format, because it forces the password check code to
re-parse the 4 parameters of the SCRAM credentials upon every
authentication event.
Instead, we want to pre-parse the parameters once, and store them
pre-parsed in the credential cache.
To achieve this, this commit introduces a new
security.PasswordHashto replace
[]byteto represent password hashes in memory.A stored byte array can be converted to a
security.PasswordHashusing the new
security.LoadPasswordHash()function.Additionally, this commit performs the following cleanups:
security.CompareHashAndPasswordtoCompareHashAndCleartextPasswordto clarify the formatthe password is expected in.
CompareHashAndCleartextPassword()returna boolean alongside the error. The function does not
return an error any more if the hash is valid but the
credentials don't match. Callers are now expected to
propagate the returned error, if any.
security.ErrPasswordUserAuthFailed, whichwas a formatting string, and replaces it by a new
error constructor
NewErrPasswordUserAuthFailed(thiswas meant to be used to construct errors originally).
NB: this commit merely adds a new abstraction in the APIs.
SCRAM hashes are still not used for authentication.
Release note: None