Skip to content

security: enable plaintext authentication with SCRAM credentials (scram 5/10)#74845

Merged
craig[bot] merged 1 commit intomasterfrom
20211228-scram5
Jan 21, 2022
Merged

security: enable plaintext authentication with SCRAM credentials (scram 5/10)#74845
craig[bot] merged 1 commit intomasterfrom
20211228-scram5

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Jan 14, 2022

(PR peeled away from #74301; previous PR in series #74844; next PR in series: #74846)
Epic CRDB-5349

This commit introduces support for cleartext password authentication
against SCRAM-SHA-256 stored credentials.

This feature is necessary in addition to support for the full SCRAM
handshake, because the web UI authn will continue using cleartext
passwords for the foreseeable future, and we want to continue
supporting the password and cert-password HBA methods for backward
compatibility.

When performing cleartext authn against SCRAM credentials, the hash
must be computed server-side. Like bcrypt, this is meant to be
computationally expensive. Therefore, for the same reason as bcrypt,
we want to restrict CPU usage for this using a semaphore.

This commit achieves this by using the same semaphore as was used for
bcrypt computations. Because we're using the same semaphore, the
commit also renames the semaphore and its configuration env var to
denote the more general usage.

Release note (backward-incompatible change): The environment variable
that controls the max amount of CPU that can be taken by password
hash computations during authentication was renamed from
COCKROACH_MAX_BCRYPT_CONCURRENCY to
COCKROACH_MAX_PW_HASH_COMPUTE_CONCURRENCY.
Its semantics remain unchanged.

Release note (security update): CockroachDB is now able to
authenticate users via the web UI and through SQL sessions when the
client provides a cleartext password and the stored credentials are
encoded using the SCRAM-SHA-256 algorithm.

(Note: support for a SCRAM authentication flow is a separate
feature and is not the target of this release note.)

In particular, for SQL client sessions it makes it possible
to use the authentication methods password (cleartext passwords),
and cert-password (TLS client cert or cleartext password) with
either CRDB-BCRYPT or SCRAM-SHA-256 stored credentials. Previously,
only CRDB-BCRYPT stored credentials were supported for cleartext
password authentication.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz knz force-pushed the 20211228-scram4 branch from daf3bf7 to 03128b5 Compare January 14, 2022 10:42
@knz knz force-pushed the 20211228-scram5 branch from 3c095df to a0866f8 Compare January 14, 2022 10:45
@knz knz changed the title wip5 security: enable plaintext authentication with SCRAM credentials Jan 14, 2022
@knz knz requested review from a team, bdarnell and kpatron-cockroachlabs January 14, 2022 10:47
@knz knz marked this pull request as ready for review January 14, 2022 10:47
@knz knz requested a review from a team January 14, 2022 10:47
@knz knz changed the title security: enable plaintext authentication with SCRAM credentials security: enable plaintext authentication with SCRAM credentials (scram 5/10) Jan 14, 2022
@knz knz force-pushed the 20211228-scram4 branch from 03128b5 to 56202e9 Compare January 14, 2022 15:08
@knz knz force-pushed the 20211228-scram5 branch from a0866f8 to 3d34cc0 Compare January 14, 2022 15:08
@knz knz force-pushed the 20211228-scram4 branch from 56202e9 to 6243645 Compare January 14, 2022 19:18
@knz knz force-pushed the 20211228-scram5 branch from 3d34cc0 to 91a86b7 Compare January 14, 2022 19:18
@knz knz force-pushed the 20211228-scram4 branch from 6243645 to db869c0 Compare January 14, 2022 19:46
@knz knz force-pushed the 20211228-scram5 branch from 91a86b7 to d76e2cb Compare January 14, 2022 19:46
@knz knz force-pushed the 20211228-scram4 branch from db869c0 to a965cf0 Compare January 15, 2022 12:11
@knz knz force-pushed the 20211228-scram5 branch from d76e2cb to 104eb58 Compare January 15, 2022 12:11
@knz knz force-pushed the 20211228-scram4 branch from a965cf0 to 2a97dd4 Compare January 15, 2022 12:39
@knz knz requested a review from a team January 15, 2022 12:39
@knz knz requested a review from a team as a code owner January 15, 2022 12:39
@knz knz force-pushed the 20211228-scram5 branch from 104eb58 to db58d5e Compare January 15, 2022 12:39
Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but will let a security expert review

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @kpatron-cockroachlabs)

Copy link
Copy Markdown
Contributor

@kpatron-cockroachlabs kpatron-cockroachlabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @knz)


pkg/security/password.go, line 287 at r2 (raw file):

	saltedPassword := pbkdf2.Key([]byte(prepared), []byte(s.decoded.Salt), s.decoded.Iters, sha256.Size, sha256.New)
	// As per xdg-go/scram and pg's scram_ServerKey().
	serverKey := computeHMAC(scram.SHA256, saltedPassword, []byte("Server Key"))

Nit: Any chance we can drop a comment here about "Server Key"? It is correct but as someone reading the code without the details of SCRAM in their head it looks...odd/scary

Copy link
Copy Markdown
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)


pkg/security/password.go, line 287 at r2 (raw file):

Previously, kpatron-cockroachlabs wrote…

Nit: Any chance we can drop a comment here about "Server Key"? It is correct but as someone reading the code without the details of SCRAM in their head it looks...odd/scary

Done, good call.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Jan 19, 2022

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 knz force-pushed the 20211228-scram4 branch from 2a97dd4 to 3542a88 Compare January 19, 2022 13:20
This commit introduces support for cleartext password authentication
against SCRAM-SHA-256 stored credentials.

This feature is necessary *in addition to* support for the full SCRAM
handshake, because the web UI authn will continue using cleartext
passwords for the foreseeable future, and we want to continue
supporting the `password` and `cert-password` HBA methods for backward
compatibility.

When performing cleartext authn against SCRAM credentials, the hash
must be computed server-side. Like bcrypt, this is meant to be
computationally expensive. Therefore, for the same reason as bcrypt,
we want to restrict CPU usage for this using a semaphore.

This commit achieves this by using the same semaphore as was used for
bcrypt computations. Because we're using the same semaphore, the
commit also renames the semaphore and its configuration env var to
denote the more general usage.

Release note (backward-incompatible change): The environment variable
that controls the max amount of CPU that can be taken by password
hash computations during authentication was renamed from
`COCKROACH_MAX_BCRYPT_CONCURRENCY` to
`COCKROACH_MAX_PW_HASH_COMPUTE_CONCURRENCY`.
Its semantics remain unchanged.

Release note (security update): CockroachDB is now able to
authenticate users via the web UI and through SQL sessions when the
client provides a cleartext password and the stored credentials are
encoded using the SCRAM-SHA-256 algorithm.

(Note: support for a SCRAM authentication flow is a separate
feature and is not the target of this release note.)

In particular, for SQL client sessions it makes it possible
to use the authentication methods `password` (cleartext passwords),
and `cert-password` (TLS client cert or cleartext password) with
either CRDB-BCRYPT or SCRAM-SHA-256 stored credentials. Previously,
only CRDB-BCRYPT stored credentials were supported for cleartext
password authentication.
@knz knz force-pushed the 20211228-scram4 branch from e616c13 to a28002d Compare January 21, 2022 00:00
@knz knz force-pushed the 20211228-scram5 branch from 51fc85f to 4c6ac1d Compare January 21, 2022 00:00
craig bot pushed a commit that referenced this pull request Jan 21, 2022
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>
Base automatically changed from 20211228-scram4 to master January 21, 2022 09:12
@craig craig bot merged commit 4c6ac1d into master Jan 21, 2022
@craig craig bot deleted the 20211228-scram5 branch January 21, 2022 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants