Skip ALTER ROLE when the stored SCRAM verifier already matches the password - #3171
Open
g2px1 wants to merge 2 commits into
Open
Skip ALTER ROLE when the stored SCRAM verifier already matches the password#3171g2px1 wants to merge 2 commits into
g2px1 wants to merge 2 commits into
Conversation
…ssword With password_encryption = scram-sha-256, syncSecrets compared the stored rolpassword with a freshly generated verifier. SCRAM verifiers embed a random salt, so the strings never match and every sync cycle re-issued ALTER ROLE ... PASSWORD for every managed role, re-salting the verifier each time. Besides the WAL and audit noise, this invalidates SCRAM pass-through credentials cached by connection poolers (e.g. pgbouncer behind auth_query), causing a short window of 'password authentication failed' server logins after every sync. Verify the stored hash against the desired password instead: for SCRAM verifiers the salt and iteration count are taken from the stored value and the derived keys are compared. Hashes whose type does not match the configured password_encryption are still reported as outdated, so switching between md5 and scram-sha-256 keeps re-hashing roles as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
g2px1
requested review from
FxKu,
Jan-M,
idanovinda,
jopadi and
mikkeloscar
as code owners
August 13, 2026 17:53
jopadi
approved these changes
Aug 14, 2026
FxKu
reviewed
Aug 14, 2026
| func (e *Encryptor) PGUserPassword(user spec.PgUser) string { | ||
| if (len(user.Password) == md5.Size*2+len(md5prefix) && user.Password[:3] == md5prefix) || | ||
| (len(user.Password) > len(scramsha256prefix) && user.Password[:len(scramsha256prefix)] == scramsha256prefix) || user.Password == "" { | ||
| if isMD5Hash(user.Password) || isScramHash(user.Password) || user.Password == "" { |
Contributor
There was a problem hiding this comment.
could be a function because you also repeat it in PGUserPasswordUpToDate
FxKu
reviewed
Aug 14, 2026
| return false | ||
| } | ||
| return scramVerifierMatches(user.Password, storedPassword) | ||
| } |
Contributor
There was a problem hiding this comment.
maybe it's me but I find this switch case kinda hard to read
Author
There was a problem hiding this comment.
Tbh, I'm C++ programmer and I suddenly find that bug in production. So it might be not the best code you've seen
FxKu
reviewed
Aug 14, 2026
Comment on lines
+213
to
+218
| mac := hmac.New(sha256.New, key) | ||
| mac.Write([]byte("Server Key")) | ||
| derivedServerKey := mac.Sum(nil) | ||
| mac = hmac.New(sha256.New, key) | ||
| mac.Write([]byte("Client Key")) | ||
| derivedStoredKey := sha256.Sum256(mac.Sum(nil)) |
Contributor
There was a problem hiding this comment.
Suggested change
| mac := hmac.New(sha256.New, key) | |
| mac.Write([]byte("Server Key")) | |
| derivedServerKey := mac.Sum(nil) | |
| mac = hmac.New(sha256.New, key) | |
| mac.Write([]byte("Client Key")) | |
| derivedStoredKey := sha256.Sum256(mac.Sum(nil)) | |
| serverMAC := hmac.New(sha256.New, key) | |
| serverMAC.Write([]byte("Server Key")) | |
| derivedServerKey := serverMAC.Sum(nil) | |
| clientMAC := hmac.New(sha256.New, key) | |
| clientMAC.Write([]byte("Client Key")) | |
| derivedStoredKey := sha256.Sum256(clientMAC.Sum(nil)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3170.
With
password_encryption: scram-sha-256, role sync compared the storedrolpasswordwith a freshly generated hash by string equality. SCRAM-SHA-256 verifiers embed a random salt, so the comparison never matched and every sync cycle re-issuedALTER ROLE ... PASSWORDfor every managed role, re-salting the verifier each time. Besides WAL/audit noise, each re-salt invalidates SCRAM pass-through credentials cached by connection poolers (pgbouncer behindauth_query, including the operator-deployed pooler), producing a short window ofpassword authentication failedserver logins after every sync.Changes
util.PGUserPasswordUpToDate(user, storedPassword, encryption): verifies the stored hash against the desired plaintext instead of comparing strings. For SCRAM verifiers the salt and iteration count are parsed from the stored value (SCRAM-SHA-256$<iter>:<salt>$<storedKey>:<serverKey>) and the derived server/stored keys are compared withhmac.Equal.password_encryptionis reported as outdated, so switching betweenmd5andscram-sha-256still re-hashes roles;PGUserPassword;users.gouses the new check inproduceSyncRequests; the ALTER statement itself is generated exactly as before.isMD5Hash/isScramHashhelpers fromPGUserPassword(no behavior change) and added a table test covering match/mismatch, both encryptions, migration between them, pre-hashed and malformed inputs.After the fix, a role whose verifier already corresponds to the secret is left untouched;
pg_authid.rolpasswordstays stable across sync cycles and pooler SCRAM pass-through keeps working.