Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: recognize client-supplied hashes in WITH PASSWORD like pg #50757

Closed
knz opened this issue Jun 29, 2020 · 5 comments · Fixed by #72579
Closed

sql: recognize client-supplied hashes in WITH PASSWORD like pg #50757

knz opened this issue Jun 29, 2020 · 5 comments · Fixed by #72579
Assignees
Labels
A-authentication Pertains to authn subsystems A-cc-enablement Pertains to current CC production issues or short-term projects A-security A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security X-server-triaged-202105

Comments

@knz
Copy link
Contributor

knz commented Jun 29, 2020

CockroachDB currently requires the server to learn about the cleartext password of a SQL user when the password is stored (either in CREATE USER WITH PASSWORD, or ALTER USER WITH PASSWORD).

This is a security problem, and has been deprecated in PostgreSQL since v9.6 (released 2016).

The correct best practice is to have the client negotiate the password, then only provide the server with a hash/fingerprint that is sufficient to validate authentication when clients connect.

The way this works is the following:

  • for MD5 authn: the client computes the MD5 hash, and provides the hash in WITH PASSWORD (with a md5: prefix)
  • for SCRAM authn: the client chooses the SCRAM parameters, computes the hash, then provides both with WITH PASSWORD (with a scram-sha-256: prefix and 5 parameter/hash fields)

We may not wish to support MD5 auth at all in CockroachDB because it's considered obsolete (and MD5-based authn is vulnerable to various attacks already). However, perhaps it could be provided as a compatibility opt-in for legacy applications that require it.

SCRAM authn, on the other hand, is very much a thing. That particular project is tracked in #42519.

Epic CRDB-5349

@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. A-security labels Jun 29, 2020
@bdarnell
Copy link
Contributor

It has been suggested that this works already if you update the system.users table by hand. But it's better to have a supported option with clear syntax.

We should not support md5 auth at all, even as an opt-in. We should, however, support client-supplied bcrypt hashes.

I'd strongly prefer an explicit syntax like WITH HASHED PASSWORD instead of recognizing a magic prefix inside the password string.

This feature would make it impossible to enforce password complexity requirements. Given the existence of regulatory requirements around password complexity, and (until we implement scram) the fact that this change would not completely prevent the server from seeing the cleartext password (at login time), it's not clear to me that this is a good tradeoff.

@knz
Copy link
Contributor Author

knz commented Jul 20, 2020

RFC here: #51599

@jlinder jlinder added the T-server-and-security DB Server & Security label Jun 16, 2021
@knz knz added A-authentication Pertains to authn subsystems A-cc-enablement Pertains to current CC production issues or short-term projects labels Jul 29, 2021
@vbabenkoru
Copy link

@bdarnell What's the hashing algorithm used by CockroachDB? How can we create a hash outside of CockroachDB to then update this field with it?

@knz
Copy link
Contributor Author

knz commented Sep 22, 2021

@vbabenkoru

// HashPassword takes a raw password and returns a bcrypt hashed password.
func HashPassword(ctx context.Context, password string) ([]byte, error) {
        sem := getBcryptSem(ctx)
        alloc, err := sem.Acquire(ctx, 1)
        if err != nil {
                return nil, err
        }
        defer alloc.Release()
        return bcrypt.GenerateFromPassword(appendEmptySha256(password), BcryptCost)

@knz
Copy link
Contributor Author

knz commented Nov 2, 2021

NB: postgres has an ENCRYPTED keyword that's optional:

The ENCRYPTED keyword has no effect, but is accepted for backwards compatibility. The method of encryption is determined by the configuration parameter password_encryption. If the presented password string is already in MD5-encrypted or SCRAM-encrypted format, then it is stored as-is regardless of password_encryption (since the system cannot decrypt the specified encrypted password string, to encrypt it in a different format).

We could diverge from pg by requiring the ENCRYPTED keyword to specify pwd hashes perhaps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-authentication Pertains to authn subsystems A-cc-enablement Pertains to current CC production issues or short-term projects A-security A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security X-server-triaged-202105
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants