Repository navigation
fix: enforce RFC5802 channel binding negotiation and proof length in SCRAM - #476
Merged
Merged
Conversation
…SCRAM Two strictness gaps reported by greptimedb's review of the SCRAM implementation, both confirmed with proof-of-concept reproductions: Channel binding negotiation was not enforced (RFC 5802 section 6). The client-first parser accepts any "p=<type>" gs2-cbind-flag and compute_channel_binding falls back to base64-of-header for unknown types or when no server certificate is configured, so a client could request an unsupported binding on the plain SCRAM-SHA-256 mechanism and still authenticate with c= carrying no binding data. Upon receipt of the client-first message the server now rejects: - any "p" flag when no server certificate is configured - "p" flags naming a binding type other than tls-server-end-point - the "y" flag while the server supports channel binding, which indicates a downgrade attack (the -PLUS mechanism was advertised but stripped from the negotiation) Overlong SCRAM proofs were accepted on the verifier path. The proof was decoded without a length check and xor zips its operands, so a valid 32-byte proof with appended junk still verified. The proof must now be exactly one hash output long (stored_key.len()). Adds regression tests for both cases, plus verifier-path roundtrip coverage.
The new channel-binding and proof-length tests use the bare/without_proof helpers, which were gated behind the client-api feature; ungate them so the default and minimal feature sets compile. Also apply stable rustfmt formatting to the new test code.
sunng87
added a commit
to sunng87/greptimedb
that referenced
this pull request
Oct 21, 2026
…binding clients - Fix MixedAuthProvider to return a SCRAM verifier for the denied user, so the test exercises rejection by authorization after successful SCRAM verification; record the selected mechanism from the returned variant and assert authorize ran exactly once per verified user. - Assert the denied connection fails with 28P01 before any terminal message, protecting the completion-ordering introduced by pgwire's authentication gate. - Add a raw-protocol test proving a client-first with a p= channel binding flag is rejected (RFC 5802 section 6), while n-flag clients (tokio_postgres) continue normally. Bump pgwire to 0.42.1, which enforces RFC 5802 channel binding negotiation and rejects short/overlong SCRAM proofs on the verifier path (sunng87/pgwire#476). Signed-off-by: Ning Sun <sunning@greptime.com>
MarkkuPekkarinen
pushed a commit
to MarkkuPekkarinen/greptimedb
that referenced
this pull request
Oct 21, 2026
…timeTeam#9634) * feat: use pgwire auth mechanisms API to simplify startuphandler * chore: update pgwire * chore: update pgwire revision to merged one * refactor: model based on upstream api * refactor: simplify implementation by introducing authsource * refactor: update to latest pgwire apis * refactor: adopt pgwire api changes * chore: switch to released pgwire * test: harden pg scram mixed-auth integration test and reject channel-binding clients - Fix MixedAuthProvider to return a SCRAM verifier for the denied user, so the test exercises rejection by authorization after successful SCRAM verification; record the selected mechanism from the returned variant and assert authorize ran exactly once per verified user. - Assert the denied connection fails with 28P01 before any terminal message, protecting the completion-ordering introduced by pgwire's authentication gate. - Add a raw-protocol test proving a client-first with a p= channel binding flag is rejected (RFC 5802 section 6), while n-flag clients (tokio_postgres) continue normally. Bump pgwire to 0.42.1, which enforces RFC 5802 channel binding negotiation and rejects short/overlong SCRAM proofs on the verifier path (sunng87/pgwire#476). Signed-off-by: Ning Sun <sunning@greptime.com> --------- Signed-off-by: Ning Sun <sunning@greptime.com>
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
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.
Summary
Fixes two SCRAM strictness gaps surfaced by greptimedb's review of the SCRAM implementation. Both were confirmed with proof-of-concept reproductions before fixing.
1. Unsupported channel binding no longer accepted (RFC 5802 §6)
The client-first parser accepts any
p=<type>gs2-cbind-flag, andcompute_channel_bindingfalls back to base64-of-header for unknown binding types (or when no server certificate is configured). As a result, a client could request an unsupported channel binding on the plainSCRAM-SHA-256mechanism and still authenticate, withc=carrying no binding data — PoC authenticated withp=tls-unique,,n=user,...against a server with no certificate configured.Upon receipt of the client-first message, the server now rejects:
pflag when no server certificate is configured (no binding supported at all)pflags naming a binding type other thantls-server-end-point(the only supported type)yflag while the server supports channel binding — per RFC 5802 §6 this indicates a downgrade attack (the-PLUSmechanism was advertised but stripped from the negotiation). Without this rule, advertisingSCRAM-SHA-256-PLUSprovides no downgrade protection.The check lives in
on_client_first_message_with_secret, so it covers bothon_client_first_messageand the message-levelprocess_scram_messagepath, as well as direct low-level users ofScramServerAuth.2. Overlong SCRAM proof rejected on the verifier path
The
Verifierpath decodedp=proof with no length check, andxorzips its operands — silently truncating to 32 bytes. A valid 32-byte proof with appended junk still verified (PoC: 35-byte proof = 32 valid + 3 junk bytes authenticated). TheSaltedPasswordpath was already safe (exact base64 comparison).The proof must now be exactly one hash output long (
stored_key.len()).Tests
p=tls-server-end-point,p=tls-unique,p=tls-exporter)yrejected when configured but still accepted without a certificate;nstill accepted when configuredNotes
y-flag clients are now rejected when a certificate is configured. pgwire currently advertisesSCRAM-SHA-256-PLUSwhenever a certificate is configured, even on non-TLS connections — rejectingythere is RFC-correct (a compliant client must usep=when-PLUSis offered), but a possible follow-up is to stop advertising-PLUSon non-TLS connections like postgres does (it only advertises it over SSL).