Don't store plaintext passwords #1040

gdsmith · 2025-05-22T16:39:52Z

Rather store hashed passwords in the way mysql server does.

here we:

update CredentialProvider to return new Credential struct
Credential includes the plugin that the user was created with
update InMemoryProvider to handle hashing of passwords and add default auth method to make usage backwards compatible
update server authentication to use mysql server methods of comparing hashes rather than relying on having the plaintext password available
rework the password negotiation to switch plugin type to match the stored credentials
add hashing and comparison functions for the above where missing from existing libraries

Rather store hashed passwords in the way mysql server does. here we: - update CredentialProvider to return new Credential struct - Credential includes the plugin that the user was created with - update InMemoryProvider to handle hashing of passwords and add default auth method to make usage backwards compatible - update server authentication to use mysql server methods of comparing hashes rather than relying on having the plaintext password available - rework the password negotiation to switch plugin type to match the stored credentials - add hashing and comparison functions for the above where missing from existing libraries

gdsmith · 2025-05-22T16:44:23Z

Code is fully working as much as I can see but I am seeing failures with the TestCachingSha2Cache tests. I don't fully understand if it's a client problem or a change I've introduced means the server is pulling those credentials multiple times when it didn't previously. I did try debugging and it seemed like the cache was empty even if I confirmed the cache was actually written. Would love some feedback, and some help with the failing tests if at all possible.

also update the caching test to allow for the fact that we request the password once on every auth attempt

gdsmith · 2025-05-23T08:27:55Z

The changes to the code is what has broken the test, we are now getting the password to confirm the auth method. I think the code changes break the way you are checking if we are using the cache response, but I'm not sure how to update the test to ensure we are using the cache (though through debugging I can confirm that it does)

encapsulate the creation of credentials to simplify downstream implementation publicise the password and plugin so downstream implementations are able to access them

Copilot

Pull Request Overview

Avoid storing plaintext passwords by hashing credentials and updating authentication flow to match MySQL server behavior.

Change CredentialProvider and InMemoryProvider to return and store a Credential struct containing a hashed password and plugin name.
Refactor server handshake and auth logic to use hashed credentials, plugin negotiation, and unified compareAuthData.
Add utility functions in mysql/util.go for password hashing, encoding/decoding, and plugin-specific comparisons.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
server/server_test.go	Adapt tests for new `AddUser` signature and `NewCustomizedConn`
server/resp.go	Track `authPluginName` on switch requests
server/handshake_resp.go	Use `credential.authPluginName` for auth switch logic
server/credential_provider.go	Return `Credential` (hash+plugin), hash on add, preserve defaults
server/conn.go	Replace plaintext `password` field with `Credential`
server/caching_sha2_cache_test.go	Update call count assertion and `NewCustomizedConn` usage
server/auth_switch_response.go	Simplify to `compareAuthData`
server/auth.go	Refactor all comparisons to use `Credential`
mysql/util.go	Add native, SHA2, SHA256 hashing + encode/decode helpers
client/auth.go	Update client to use new hash util signatures

Comments suppressed due to low confidence (4)

server/auth_switch_response.go:11

The import 'github.com/pingcap/tidb/pkg/parser/auth' is unused in this file and should be removed to avoid compile errors.

"github.com/pingcap/tidb/pkg/parser/auth"

mysql/util.go:216

rand.Read is used here but package 'crypto/rand' is not imported; either import 'crypto/rand' or qualify this call.

_, err := rand.Read(salt)

mysql/util.go:90

strings.ToUpper is used in EncodePasswordHex but the 'strings' package is not imported; add import "strings".

hexstr := strings.ToUpper(hex.EncodeToString(passwordHash))

server/auth.go:106

compareNativePasswordAuthData accepts a Credential parameter but decodes c.credential.password instead of the passed-in credential; change to use credential.password.

password, err := mysql.DecodePasswordHex(c.credential.password)

mysql/util.go

lance6716 · 2025-05-25T10:03:20Z

server/auth.go

-		}
-		if bytes.Equal(plain, dbytes) {
-			return nil
+		clientAuthData = mysql.Xor(dbytes, c.salt)


(I need more time to check if the lengths of dbytes and c.salt are equal)

Seems they may have different lengths, at least a malicious attacker can send authData because it just reads from a packet without length check 🤔

mysql/util.go

lance6716 · 2025-05-25T10:19:27Z

mysql/util.go

+
+// hashCrypt256 salt and hash a password the given number of iterations
+func hashCrypt256(source, salt string, iterations uint64) (string, error) {
+	actualIterations := iterations * ITERATION_MULTIPLIER


for this function, the const ITERATION_MULTIPLIER is directly used. But for generateUserSalt SALT_LENGTH is pass-in as an argument. I think generateUserSalt can also directly use SALT_LENGTH

I tried to match mysql server salt generation, but I'm wondering if it's used there to generate both the salt for the stored password hashes and for scramble generation. I see the scramble generation here is using something similar but not quite the same, perhaps the two can/should be combined?

I didn't get your point clearly.

perhaps the two can/should be combined?

Do you mean the "scramble generation" related functions are similar to "salt generation" functions, so you want to merge them? Can you give a simple explanation about which functions are affected?

Yes, I was looking at RandomBuf in mysql/util.go:319 which I believe may be better suited to use crypto/rand. I think that mysql is avoiding the multi-byte UTF-8 to simplify client/server salt exchanges so it either makes sense to remove that in the salt generation or combine the two functions here and generate salts that suit both purposes.

mysql/util.go

lance6716 · 2025-05-25T10:27:13Z

mysql/util.go

+	}
+
+	hashHex := hex.EncodeToString(hash[:])
+	digest := fmt.Sprintf("$%d$%s$%s", iterations, salt, hashHex)


In generateUserSalt I see some restrictions are added to the random bytes. Is this line the reason of it? I think it's OK to remove the restrictions to make the whole logic more simple. We can hex-encode the salt without the restrictions.

You might be right, I was trying to recreate what was being done in mysql-server, but it may not be necessary here

mysql/util.go

lance6716 · 2025-05-28T04:37:39Z

mysql/util.go

+// FROM vitess.io/vitess/go/mysql/auth_server.go
+// DecodePasswordHex decodes the standard format used by MySQL
+// for 4.1 style password hashes. It drops the optionally leading * before
+// decoding the rest as a hex encoded string.


Suggested change

// FROM vitess.io/vitess/go/mysql/auth_server.go

// DecodePasswordHex decodes the standard format used by MySQL

// for 4.1 style password hashes. It drops the optionally leading * before

// decoding the rest as a hex encoded string.

// DecodePasswordHex decodes the standard format used by MySQL

// for 4.1 style password hashes. It drops the optionally leading * before

// decoding the rest as a hex encoded string.

// the implementation is also learned from vitess.io/vitess/go/mysql/auth_server.go

Golang's coding style suggests the function name DecodePasswordHex being put at the beginning.

mysql/util.go

lance6716 · 2025-05-28T04:57:03Z

mysql/util.go

+
+// hashCrypt256 salt and hash a password the given number of iterations
+func hashCrypt256(source, salt string, iterations uint64) (string, error) {
+	actualIterations := iterations * ITERATION_MULTIPLIER


I didn't get your point clearly.

perhaps the two can/should be combined?

Do you mean the "scramble generation" related functions are similar to "salt generation" functions, so you want to merge them? Can you give a simple explanation about which functions are affected?

dveeden · 2025-08-20T07:12:11Z

@gdsmith could you merge master into your branch to update it and have a look at the conversations that aren't resolved yet?

Co-authored-by: lance6716 <[email protected]>

gdsmith · 2025-08-20T08:29:21Z

I've merged and added the suggestions. Is it my role to resolve the conversations here?

dveeden · 2025-08-20T09:20:32Z

I've merged and added the suggestions. Is it my role to resolve the conversations here?

For the conversations/comments: have a look at them and feel free to click "Resolve conversation" if you think they are handled.

dveeden · 2025-08-20T09:24:11Z

golangci/errcheck is failing Adduser() is called in a few places without checking the error that it returns.

Best to handle the error and if that isn't possible to explicitly ignore it.

lance6716 · 2025-08-23T07:38:07Z

Hi @gdsmith,

I've resolved a few comments, and I'd like to follow up on the remaining ones. This seems like a really valuable and helpful PR. If you're busy with other tasks, I'd be happy to help move it forward. What do you think?

lance6716 requested a review from dveeden May 23, 2025 01:41

Update to ensure we only request the password once

43cc679

also update the caching test to allow for the fact that we request the password once on every auth attempt

gdsmith and others added 3 commits May 23, 2025 09:28

Merge branch 'master' into compare-hashes

4e6aea2

Add NewCredential func

88a5a80

encapsulate the creation of credentials to simplify downstream implementation publicise the password and plugin so downstream implementations are able to access them

allow creation of cleartext password credentials

80c125d

lance6716 requested a review from Copilot May 25, 2025 09:53

Copilot AI reviewed May 25, 2025

View reviewed changes

lance6716 reviewed May 25, 2025

View reviewed changes

fixing some review comments

ea8b036

lance6716 reviewed May 28, 2025

View reviewed changes

gdsmith and others added 2 commits August 20, 2025 09:19

Merge branch 'master' into compare-hashes

0578e29

Apply suggestions from code review

82f6571

Co-authored-by: lance6716 <[email protected]>

Don't store plaintext passwords #1040

Are you sure you want to change the base?

Don't store plaintext passwords #1040

Conversation

gdsmith commented May 22, 2025

Uh oh!

gdsmith commented May 22, 2025

Uh oh!

gdsmith commented May 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dveeden commented Aug 20, 2025

Uh oh!

gdsmith commented Aug 20, 2025

Uh oh!

dveeden commented Aug 20, 2025

Uh oh!

dveeden commented Aug 20, 2025

Uh oh!

lance6716 commented Aug 23, 2025

Uh oh!

Uh oh!