Use hashlib.pbkdf2_hmac for SCRAM salted password - #1359
Open
twelfthlabor wants to merge 2 commits into
Open
twelfthlabor wants to merge 2 commits into
twelfthlabor wants to merge 2 commits into
Conversation
_generate_salted_password becomes cpdef so tests can reach the compiled method; internal call sites keep the C fast path.
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.
SCRAMAuthentication._generate_salted_password()reimplements the RFC 5802Hi() function as a Python-level PBKDF2 loop: one
hmac.new()and aPython-level XOR over
zip()for each iteration. PostgreSQL's defaultiteration count is 4096, so that loop runs 4095 times before every
password-authenticated connection can finish its handshake.
hashlib.pbkdf2_hmac("sha256", ...)computes exactly the same value in C.Hi() is PBKDF2-HMAC-SHA256 with a single output block, so
dkLenequals thedigest size (32 bytes) and the result is byte-identical to the loop.
The old code's first HMAC used
self.DIGEST, and every iteration after thathardcoded
hashlib.sha256. SinceSCRAM-SHA-256is the only mechanism thisclass handles, naming the algorithm explicitly removes no real digest
agility.
_bytes_xorstays, because_generate_client_proofstill uses it.Fixes #1357.
Verification
The old loop and
hashlib.pbkdf2_hmacwere compared in a standalone batteryof 324 cases that reproduces the loop verbatim: published
PBKDF2-HMAC-SHA256 test vectors, iteration counts 1, 2, 3, 4096 and 10000,
empty, short, long and unicode passwords and salts, and 310 random
combinations. Every result is 32 bytes and byte-for-byte equal.
A build of the patch was then connected with a password to a local
PostgreSQL 16 server with
password_encryption = scram-sha-256andscram-sha-256inpg_hba.conf. Handshakes succeeded with storediteration counts 2, 4096 and 10000, a wrong password still failed with
InvalidPasswordError, and the server log recordedmethod=scram-sha-256.Instrumenting
hashlib.pbkdf2_hmacshowed the compiled extension calling itwith
('sha256', ..., 4096)during a live handshake, while the same check onthe pre-fix build recorded no calls. The compiled path was exercised
end-to-end but was not separately timed.
A standalone microbenchmark of the old loop against the stdlib primitive at
4096 iterations, best of 7 runs, measured 12.9 ms against 0.85 ms in one run
(15.3x) and 18.8 ms against 1.08 ms in another (17.5x) on this machine (Apple
silicon, CPython 3.11).
Tests
tests/test_scram.pyis new and runs without a server: RFC 7914 and RFC 7677vectors, byte-equality against the old Hi() loop (including a unicode
password), and a spy asserting the compiled path calls
hashlib.pbkdf2_hmac("sha256", password, salt, iterations). All 4 pass on the fix. The pbkdf2-call test fails against the pre-fix code(no call is recorded), so it pins the change. The server-free subset
(
test_scram,test_record,test__sourcecode) is 31 tests OK, 2 skipped.To make the compiled method reachable from a test without a server,
_generate_salted_passwordchanges fromcdeftocpdefinscram.pyx/scram.pxd; internal call sites keep the C fast path. If youwould rather keep the method fully private, say so and I will move the test
behind a fake-server harness or drop it; the performance fix does not depend
on the visibility change.
tests.test_connectpasses on both the pre-fix and post-fix build (51 tests,2 environment skips), including
test_auth_password_scram_sha_256against amanaged cluster.
tests.test__sourcecodeandtests.test_testpass. Thefull suite needs a PostgreSQL instance, so only the connect and auth subset
was run locally. CI runs the suite against PostgreSQL 9.5 through 18, and
test_auth_password_scram_sha_256connects with a password on 10 and above,so a wrong digest, dkLen or iteration handling fails in CI.
One edge case changes behavior. PostgreSQL never sends an iteration count of
zero and RFC 5802 does not allow it, but if a nonconforming server did, the
old loop quietly computed a one-iteration value while
pbkdf2_hmacraisesValueErrorand the connection fails. Validatingi >= 1inparse_server_first_messagewould move that error into the auth layer; leftas a follow-up here.