Skip to content

pgwire: support the SCRAM authentication flow (scram 7/10)#74847

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

pgwire: support the SCRAM authentication flow (scram 7/10)#74847
craig[bot] merged 1 commit intomasterfrom
20211228-scram7

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Jan 14, 2022

(PR peeled away from #74301; previous PR in series #74846; next PR in series: #74848)
Fixes #42519.
Informs #65117.
Epic CRDB-5349

Release note (security update): CockroachDB now supports the
SCRAM-SHA-256 authentication method for SQL clients in a way
compatible with PostgreSQL servers and most PostgreSQL-compatible
client drivers.

SCRAM is a standard authentication protocol defined by IETF RFCs
5802 and
7677. In contrast to
the cleartext-based mechanism that CockroachDB was previously using,
SCRAM offers:

  • computational burden moved to the client: the computational
    cost to compute the authentication hash is borne by the
    client, and thus prevents DoS attacks enabled by forcing the
    server to compute many hashes simultaneously by malicious clients.
  • non-replayability: a malicious intermediary cannot reuse
    a password observed in one session to re-gain access later.
  • protection against credential stuffing: a malicious
    intermediary cannot reuse a password to gain access to other services.
  • credential secrecy: the server never learns the cleartext
    password and cannot impersonate the client to other servers.

As before, the SCRAM credentials are stored on the server in hashed
form, and prevent a malicious attacker from gaining knowledge about
passwords even if they gain access to a copy of the hashes.

To use SCRAM, an operator must take care of the following:

  1. the stored credentials for the SQL accounts that want to use SCRAM
    must use the SCRAM password hash format.

    To store SCRAM hashes in CockroachDB, at this time it is necessary
    to pre-compute the SCRAM hash in a SQL client and store it
    pre-hashed using a CREATE/ALTER USER/ROLE WITH PASSWORD statement.

    This was documented in a previous release note already.

    A mechanism to compute the SCRAM hash server-side from a cleartext
    password might be provided at a later date. Note however that such
    a mechanism is generally undesirable: one of the main benefits of
    SCRAM is to remove the need for the server to know the client's
    cleartext password at any time; a SCRAM hash generation server-side
    would defeat this benefit.

    A plan also exists to auto-migrate existing passwords to the new
    format (refer to a later release note).

  2. the SCRAM authentication method must be enabled.

    This can be done e.g. explicitly to require SCRAM specifically
    via a HBA configuration via the cluster setting
    server.host_based_authentication.configuration.

    For this, two new authentication methods are available:
    scram-sha-256 certs-scram-sha-256. The first one is akin to
    PostgreSQL and requires a SCRAM authentication flow with the
    client. The second one is CockroachDB-specific and allows SQL
    client to authenticate either using a TLS client certificate or
    a valid SCRAM authentication flow.

    For example, the configuration line host all all all scram-sha-256 will require a SCRAM authentication flow for all
    clients besides the root user.

    A plan also exists to automatically opt existing clusters
    into SCRAM (refer to a later release note).

Known limitations:

  • HTTP authentication (web UI, HTTP APIs) still uses cleartext
    passwords.

    Security there can be enhanced in two ways:

    • enable and use OIDC authentication for the web UI.
    • use separate user accounts for access to HTTP than those
      used for access to SQL.
  • the CockroachDB implementation of SCRAM differs from PostgreSQL in
    two ways:

    • the extended protocol SCRAM-SHA-256-PLUS is not yet supported.
      SCRAM-SHA-256-PLUS adds channel binding over TLS, a mechanism that
      offers MITM protection from malicious intermediaries even when these
      have access to well-signed TLS certificates. Without this extension,
      proper MITM protection requires the client to verify the server
      certificate against a known CA and server fingerprint.

      CockroachDB does not yet support SCRAM-SHA-256-PLUS because we
      have observed that support for channel binding is not yet common
      in SQL client drivers besides PostgreSQL's own libpq driver.

      Tracking issue: security: implement SCRAM channel binding #74300

    • CockroachDB does not yet implement zero-knowledge authentication
      failures like PostgreSQL. In PostgreSQL, the implementation ensures
      that a SQL client cannot distinguish the causes of an authentication
      failure: whether a password is missing, expired, invalid, or the
      user account does not exist, the SQL client is forced to pay the
      price of the SCRAM handshake and does not learn the exact cause of
      the failure. This ensures that a malicious attacker cannot use the
      type of authentication failure as a mechanism to learn properties of
      a target SQL account.

      This mechanism may be implemented in CockroachDB at a later time.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz knz force-pushed the 20211228-scram6 branch from 40104ab to 2157577 Compare January 14, 2022 10:48
@knz knz force-pushed the 20211228-scram7 branch from 38bf105 to 320e8c7 Compare January 14, 2022 10:50
@knz knz changed the title wip7 pgwire: support the SCRAM authentication flow Jan 14, 2022
@knz knz marked this pull request as ready for review January 14, 2022 10:51
@knz knz requested review from a team, kpatron-cockroachlabs and rafiss January 14, 2022 10:51
@knz knz changed the title pgwire: support the SCRAM authentication flow pgwire: support the SCRAM authentication flow (scram 7/10) Jan 14, 2022
@knz knz force-pushed the 20211228-scram6 branch from 2157577 to a98a66e Compare January 14, 2022 15:09
@knz knz force-pushed the 20211228-scram7 branch from 320e8c7 to 4226e26 Compare January 14, 2022 15:10
@knz knz force-pushed the 20211228-scram6 branch from a98a66e to b7cdad5 Compare January 14, 2022 19:18
@knz knz force-pushed the 20211228-scram7 branch from 4226e26 to 3e1e533 Compare January 14, 2022 19:18
@knz knz force-pushed the 20211228-scram6 branch from b7cdad5 to 86e4d07 Compare January 14, 2022 19:46
@knz knz force-pushed the 20211228-scram7 branch from 3e1e533 to fb2ee19 Compare January 14, 2022 19:46
@knz knz force-pushed the 20211228-scram6 branch from 86e4d07 to 46ebd02 Compare January 15, 2022 12:11
@knz knz force-pushed the 20211228-scram7 branch from fb2ee19 to 0552233 Compare January 15, 2022 12:12
@knz knz force-pushed the 20211228-scram6 branch from 46ebd02 to bdd7cb2 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-scram7 branch from 0552233 to 38e202c 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.

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


pkg/sql/pgwire/auth_methods.go, line 235 at r1 (raw file):

				// code fallback, to hide the reason for the failure from the
				// client.
				return creds, errors.New("password is expired")

similar question as i had in the previous PR; why have a different error?


pkg/sql/pgwire/auth_methods.go, line 243 at r1 (raw file):

			// The method check above ensures this cast is always valid.
			scramCreds := hashedPassword.(*security.ScramHash)

the ScramHash struct could remain unexported if StoredCredentials() was a method on the PasswordHash interface. that may be a better abstraction so we can avoid the chance someone sees this type assertion and copies it incorrectly.

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 2 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz, @kpatron-cockroachlabs, and @rafiss)

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 @kpatron-cockroachlabs and @rafiss)


pkg/sql/pgwire/auth_methods.go, line 235 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

similar question as i had in the previous PR; why have a different error?

Similar answer as here: you're right and this can be improved.
Are you OK with me doing so in a followup PR?


pkg/sql/pgwire/auth_methods.go, line 243 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the ScramHash struct could remain unexported if StoredCredentials() was a method on the PasswordHash interface. that may be a better abstraction so we can avoid the chance someone sees this type assertion and copies it incorrectly.

Yeah I've thought about this, but the return type of StoredCredentials is really scram-specific.
I changed this to a function call. Do you like it better?

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; optional suggestion

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


pkg/sql/pgwire/auth_methods.go, line 235 at r1 (raw file):

Previously, knz (kena) wrote…

Similar answer as here: you're right and this can be improved.
Are you OK with me doing so in a followup PR?

yes that sounds great


pkg/sql/pgwire/auth_methods.go, line 243 at r1 (raw file):

Previously, knz (kena) wrote…

Yeah I've thought about this, but the return type of StoredCredentials is really scram-specific.
I changed this to a function call. Do you like it better?

yes this seems nice. if you'd like, i feel it the most safe of all options (to prevent the GetSCRAMStoredCredentials from being misused in the future and cause a panic) is something like

func GetSCRAMStoredCredentials(hash PasswordHash) (scram.StoredCredentials, bool) {
  sh, ok := hash.(*scramHash)
  return sh, ok
}

and the Method() check above could be elided

} else if scramCreds, ok := security.GetSCRAMStoredCredentials(hashedPassword); ok {
  return scramCreds
}
const credentialsNotSCRAM = "user password hash not in SCRAM format"
c.LogAuthInfof(ctx, credentialsNotSCRAM)
return creds, errors.New(credentialsNotSCRAM)

i don't feel strongly about it

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 @knz, @kpatron-cockroachlabs, and @rafiss)


pkg/sql/pgwire/auth_methods.go, line 243 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

yes this seems nice. if you'd like, i feel it the most safe of all options (to prevent the GetSCRAMStoredCredentials from being misused in the future and cause a panic) is something like

func GetSCRAMStoredCredentials(hash PasswordHash) (scram.StoredCredentials, bool) {
  sh, ok := hash.(*scramHash)
  return sh, ok
}

and the Method() check above could be elided

} else if scramCreds, ok := security.GetSCRAMStoredCredentials(hashedPassword); ok {
  return scramCreds
}
const credentialsNotSCRAM = "user password hash not in SCRAM format"
c.LogAuthInfof(ctx, credentialsNotSCRAM)
return creds, errors.New(credentialsNotSCRAM)

i don't feel strongly about it

Done.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Jan 20, 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).

Release note (security update): CockroachDB now supports the
SCRAM-SHA-256 authentication method for SQL clients in a way
compatible with PostgreSQL servers and most PostgreSQL-compatible
client drivers.

SCRAM is a standard authentication protocol defined by IETF RFCs
[5802](https://datatracker.ietf.org/doc/html/rfc5802) and
[7677](https://datatracker.ietf.org/doc/html/rfc7677). In contrast to
the cleartext-based mechanism that CockroachDB was previously using,
SCRAM offers:

- computational burden moved to the client: the computational
  cost to compute the authentication hash is borne by the
  client, and thus prevents DoS attacks enabled by forcing the
  server to compute many hashes simultaneously by malicious clients.
- non-replayability: a malicious intermediary cannot reuse
  a password observed in one session to re-gain access later.
- protection against credential stuffing: a malicious
  intermediary cannot reuse a password to gain access to other services.
- credential secrecy: the server never learns the cleartext
  password and cannot impersonate the client to other servers.

As before, the SCRAM credentials are stored on the server in hashed
form, and prevent a malicious attacker from gaining knowledge about
passwords even if they gain access to a copy of the hashes.

To use SCRAM, an operator must take care of the following:

1. the stored credentials for the SQL accounts that want to use SCRAM
   must use the SCRAM password hash format.

   To store SCRAM hashes in CockroachDB, at this time it is necessary
   to pre-compute the SCRAM hash in a SQL client and store it
   pre-hashed using a CREATE/ALTER USER/ROLE WITH PASSWORD statement.

   This was documented in a previous release note already.

   A mechanism to compute the SCRAM hash server-side from a cleartext
   password might be provided at a later date. Note however that such
   a mechanism is generally undesirable: one of the main benefits of
   SCRAM is to remove the need for the server to know the client's
   cleartext password at any time; a SCRAM hash generation server-side
   would defeat this benefit.

   A plan also exists to auto-migrate existing passwords to the new
   format (refer to a later release note).

2. the SCRAM authentication method must be enabled.

   This can be done e.g. explicitly to require SCRAM specifically
   via a HBA configuration via the cluster setting
   `server.host_based_authentication.configuration`.

   For this, two new authentication methods are available:
   `scram-sha-256` `certs-scram-sha-256`. The first one is akin to
   PostgreSQL and requires a SCRAM authentication flow with the
   client. The second one is CockroachDB-specific and allows SQL
   client to authenticate *either* using a TLS client certificate *or*
   a valid SCRAM authentication flow.

   For example, the configuration line `host all all all
   scram-sha-256` will require a SCRAM authentication flow for all
   clients besides the `root` user.

   A plan also exists to automatically opt existing clusters
   into SCRAM (refer to a later release note).

Known limitations:

- HTTP authentication (web UI, HTTP APIs) still uses cleartext
  passwords.

  Security there can be enhanced in two ways:
  - enable and use OIDC authentication for the web UI.
  - use separate user accounts for access to HTTP than those
    used for access to SQL.

- the CockroachDB implementation of SCRAM differs from PostgreSQL in
  two ways:

  - the extended protocol SCRAM-SHA-256-PLUS is not yet supported.
    SCRAM-SHA-256-PLUS adds *channel binding* over TLS, a mechanism that
    offers MITM protection from malicious intermediaries even when these
    have access to well-signed TLS certificates. Without this extension,
    proper MITM protection requires the client to verify the server
    certificate against a known CA and server fingerprint.

    CockroachDB does not yet support SCRAM-SHA-256-PLUS because we
    have observed that support for channel binding is not yet common
    in SQL client drivers besides PostgreSQL's own `libpq` driver.

  - CockroachDB does not yet implement zero-knowledge authentication
    failures like PostgreSQL.  In PostgreSQL, the implementation ensures
    that a SQL client cannot distinguish the causes of an authentication
    failure: whether a password is missing, expired, invalid, or the
    user account does not exist, the SQL client is forced to pay the
    price of the SCRAM handshake and does not learn the exact cause of
    the failure. This ensures that a malicious attacker cannot use the
    type of authentication failure as a mechanism to learn properties of
    a target SQL account.

    This mechanism may be implemented in CockroachDB at a later time.
@knz knz force-pushed the 20211228-scram6 branch from 29f6df2 to 6071d6f Compare January 21, 2022 00:00
@knz knz force-pushed the 20211228-scram7 branch from 86fb393 to ddddd35 Compare January 21, 2022 00:01
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-scram6 to master January 21, 2022 09:12
@craig craig bot merged commit ddddd35 into master Jan 21, 2022
@craig craig bot deleted the 20211228-scram7 branch January 21, 2022 09:12
@Neustradamus
Copy link
Copy Markdown

@knz: Thanks a lot for your work on it :)

Note about HTTP, there is an RFC: https://tools.ietf.org/html/rfc7804.

LDAP exists too: More info here: scram-sasl/info#1...

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.

security: support SCRAM-SHA-256 authentication method

5 participants