Skip to content

sql,security: support storing SCRAM hashes in ALTER/CREATE USER/ROLE (scram 3/10)#74843

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

sql,security: support storing SCRAM hashes in ALTER/CREATE USER/ROLE (scram 3/10)#74843
craig[bot] merged 1 commit intomasterfrom
20211228-scram3

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Jan 14, 2022

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

This commit introduces the ability to store SCRAM hashes.
Note: in this commit they are not yet used to authenticate
SQL clients. Authentication will fail if a SCRAM hash is the only
credential available to a user account.

Release note (sql change): The CREATE ROLE and ALTER ROLE
statements now accept password hashes computed using the SCRAM-SHA-256
method, for example: CREATE USER foo WITH PASSWORD 'SCRAM-SHA-256$4096:B5VaT...'.

As for other types of pre-hashed passwords, this auto-detection can be
disabled by changing the cluster setting
server.user_login.store_client_pre_hashed_passwords.enabled to
false.

To ascertain whether a SCRAM-SHA-256 password hash will be recognized
as such, orchestration code can use the built-in function
crdb_internal.check_password_hash_format() that had been previously introduced.

The encoding of the SCRAM-SHA-256 password should be performed as
follows:

  1. take the cleartext password string.
  2. generate a salt, iteration count, stored key and server key as per
    RFC 5802.
  3. encode the hash into the format recognized by CockroachDB: the
    string SCRAM-SHA-256$, followed by the iteration count,
    followed by :, followed by the base64-encoded salt, followed
    by $, followed by the base-64 stored key, followed by :,
    followed by the base-64 server key.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz knz force-pushed the 20211228-scram2 branch from 33f2ebe to 9455399 Compare January 14, 2022 10:34
@knz knz force-pushed the 20211228-scram3 branch from eee689b to 796f659 Compare January 14, 2022 10:40
@knz knz changed the title wip3 sql,security: support storing SCRAM hashes in ALTER/CREATE USER/ROLE Jan 14, 2022
@knz knz requested a review from rafiss January 14, 2022 10:41
@knz knz marked this pull request as ready for review January 14, 2022 10:41
@knz knz requested a review from a team January 14, 2022 10:41
@knz knz changed the title sql,security: support storing SCRAM hashes in ALTER/CREATE USER/ROLE sql,security: support storing SCRAM hashes in ALTER/CREATE USER/ROLE (scram 3/10) Jan 14, 2022
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Jan 14, 2022

Note to self: add a check that the iter count is greater than 4096. edit:done

@knz knz force-pushed the 20211228-scram3 branch from 796f659 to fd18de4 Compare January 14, 2022 19:17
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; i left a comment which maybe can be considered for a separate PR (or if you feel it's easy to add to this one feel free)

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)


-- commits, line 28 at r1:
instead of linking to the RFC (whose instructions i find hard to follow), what are your thoughts on introducing a builtin function that does these steps? maybe crdb_internal.generate_scram_hash(salt, iter_count, stored_key, server_key)

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Jan 14, 2022

what are your thoughts on introducing a builtin function that does these steps? maybe crdb_internal.generate_scram_hash(salt, iter_count, stored_key, server_key)

So you'd want the client to compute the stored_key and server_key, and only use the SQL-side builtin to do the base64 encoding?

@knz knz force-pushed the 20211228-scram2 branch from 9455399 to 9ea2ce9 Compare January 15, 2022 12:38
@knz knz requested a review from a team January 15, 2022 12:38
@knz knz requested a review from a team as a code owner January 15, 2022 12:38
@knz knz force-pushed the 20211228-scram3 branch from fd18de4 to 35f27d3 Compare January 15, 2022 12:38
@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).

This commit introduces the ability to *store* SCRAM hashes.
Note: in this commit they are not yet used to authenticate
SQL clients. Authentication will fail if a SCRAM hash is the only
credential available to a user account.

Release note (sql change): The `CREATE ROLE` and `ALTER ROLE`
statements now accept password hashes computed using the SCRAM-SHA-256
method, for example: `CREATE USER foo WITH PASSWORD
'SCRAM-SHA-256$4096:B5VaT...'`.

As for other types of pre-hashed passwords, this auto-detection can be
disabled by changing the cluster setting
`server.user_login.store_client_pre_hashed_passwords.enabled` to
`false`.

To ascertain whether a SCRAM-SHA-256 password hash will be recognized
as such, orchestration code can use the built-in function
`crdb_internal.check_password_hash_format()` that had been previously introduced.

The encoding of the SCRAM-SHA-256 password should be performed as
follows:

1. take the cleartext password string.
2. generate a salt, iteration count, stored key and server key as per
   [RFC 5802](https://datatracker.ietf.org/doc/html/rfc5802).
3. encode the hash into the format recognized by CockroachDB: the
   string `SCRAM-SHA-256$`, followed by the iteration count,
   followed by `:`, followed by the base64-encoded salt, followed
   by `$`, followed by the base-64 stored key, followed by `:`,
   followed by the base-64 server key.
@knz knz force-pushed the 20211228-scram2 branch from acf0c01 to a6e236e Compare January 21, 2022 00:00
@knz knz force-pushed the 20211228-scram3 branch from f3ee5e9 to b822b1b 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-scram2 to master January 21, 2022 09:12
@craig craig bot merged commit b822b1b into master Jan 21, 2022
@craig craig bot deleted the 20211228-scram3 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.

3 participants