sql(mysql): fix caching_sha2_password fast authentication - #33179
Conversation
With MySQL 8's caching_sha2_password, once the server's auth cache is warm (every connection after the first successful full authentication), the server answers HandshakeResponse41 with an AuthMoreData packet carrying the fast_auth_success marker (0x03) and then sends the OK packet that actually concludes authentication. handle_auth entered the command phase on the 0x03 marker alone, so the trailing OK was routed to handle_command instead. With no query in flight yet that killed the connection with ERR_MYSQL_UNEXPECTED_PACKET; with one already dispatched the OK became the query's result and it resolved with [] instead of its real rows, desynchronizing everything after it. Stay in Authenticating on fast_auth_success and let the existing OK/ERR arms finish the handshake, matching mysql2, go-sql-driver, and Connector/J. Tests: - scripted server covering both TCP framings of the 0x03 + OK pair (split and coalesced), with the AuthMoreData and text-resultset frame builders added to wire-frames.ts - container test that warms the server-side cache with a full auth and then reconnects, exercising the real MySQL 8 fast path
|
Updated 7:46 AM PT - Jul 1st, 2026
❌ @robobun, your commit 35baa39 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33179That installs a local version of the PR into your bun-33179 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe MySQL ChangesFast Auth Flow and Protocol Coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/sql/sql-mysql.auth.test.ts`:
- Around line 72-79: Trim the long explanatory comments in the MySQL auth test
to the durable invariant only, and reduce them to at most 3 lines. In the
affected block around the passworded caching_sha2_password steady-state path,
keep only the test intent in the comment near the relevant authentication flow
and remove PR-history/background details. Apply the same cleanup to the later
comment block in the same test file so the comments remain concise and reference
the behavior being asserted around the MySQL auth cache and trailing OK
handling.
In `@test/js/sql/wire-frames.ts`:
- Around line 335-338: Shorten the MySQL Textual Resultset comment in the
wire-frames test to fit within three lines while preserving the key framing
details. Update the comment near the mysqlHandshakeV10 / CLIENT_DEPRECATE_EOF
explanation so it still mentions the lenenc column_count, ColumnDefinition41
rows, and final OK packet terminator, but remove extra wording and redundancy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee61108e-7da0-4a6c-aa74-fa6d27b8b30e
📒 Files selected for processing (3)
src/sql_jsc/mysql/MySQLConnection.rstest/js/sql/sql-mysql.auth.test.tstest/js/sql/wire-frames.ts
|
I looked at both suggested issues and linked them in the description as related rather than adding
If a maintainer can confirm either against the reporters' setups, happy to add the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…a2 scramble caching_sha2_password::scramble hashed SHA256(nonce || SHA256(SHA256(password))) instead of SHA256(SHA256(SHA256(password)) || nonce), so a real MySQL 8 always rejected the fast-auth scramble and fell back to perform_full_authentication (0x04). The concatenation order was copied from mysql_native_password, which genuinely puts the nonce first; the SHA-256 plugin reverses it. The server's Generate_scramble, mysql2, go-sql-driver, and Connector/J all hash the double SHA-256 first. Without this, the fast_auth_success (0x03) path fixed in the previous commit is unreachable against a real server; with only this, that path would regress every warm-cache connection, so the two fixes have to ship together. Deliberately left out: auth_data also still includes the handshake's trailing filler byte (21 bytes instead of the spec's 20), which desynchronizes the scramble for the same reason. That is issue #26195, with an open fix in #28161, and is not duplicated here. The new scramble test accepts either nonce length so it pins the concatenation order and survives #28161 landing.
robobun
changed the title
|
Correction to my comment above about #27102 and #26235. The review below found a second, pre-existing bug I had missed: This PR now also fixes the scramble order (commit 99e0fb8), because fixing it alone would have made the 0x03 mishandling live and broken every warm-cache connection, so the two are inseparable. One further defect in the same path (the nonce includes the handshake's trailing filler byte, so it is 21 bytes instead of 20) is deliberately left to #28161 / #26195, where there is already an open fix. |
There was a problem hiding this comment.
Thanks for folding in the scramble-order fix (99e0fb8) with a byte-level test — the concatenation now matches mysql2 / go-sql-driver / Connector/J and the server's own Generate_scramble, and the 20-vs-21-byte nonce tolerance keeps it decoupled from #28161. Deferring only because both changes sit in the MySQL auth handshake, which is worth a maintainer's sign-off.
Extended reasoning...
Overview
Since my last pass the PR grew a fourth file: src/sql/mysql/protocol/Auth.rs now swaps the two copy_from_slice lines in caching_sha2_password::scramble so the third-stage hash is SHA256(SHA256(SHA256(password)) || nonce) instead of nonce || …. That is exactly the fix I suggested, and it is backed by a new scripted test that parses the client's HandshakeResponse41 off the wire and recomputes the expected scramble in JS. wire-frames.ts gained mysqlReadLenencInt, mysqlParseHandshakeResponse41, and exported MYSQL_MOCK_AUTH_DATA_PART_1/2 (a byte-identical extraction of the constants mysqlHandshakeV10 already emitted). The original two changes — the FastAuthStatus::SUCCESS arm staying in Authenticating, and the framing-parametrised test.each — are unchanged and I already found them sound.
Security risks
None introduced. The scramble is a challenge-response over a server-supplied nonce; the previous (wrong) byte order didn't leak or weaken anything, it just guaranteed a cache miss and forced the RSA full-auth fallback. Correcting the order lets the fast path be taken but changes nothing about what a MITM can observe or forge. The FastAuthStatus::SUCCESS change only defers the Connected transition by one packet; no validation is loosened.
Level of scrutiny
Moderate-to-high. The runtime deltas are tiny (a match arm reduced to a debug log; two slice copies reordered), but they sit in the connection-phase auth handshake for the default MySQL 8 plugin and change the bytes every passworded caching_sha2_password connection puts on the wire. That is squarely "auth/crypto" per the approval guidelines, so I'm deferring rather than shadow-approving even though I've cross-checked the new order against mysql2, go-sql-driver, Connector/Python, and MySQL's sha2_password_common.cc.
Other factors
- The new scramble test accepts both the 20-byte spec nonce and Bun's current 21-byte nonce (trailing NUL filler, tracked as #26195 / #28161), so it pins only the concatenation order and won't flip when that separate fix lands. The container test's comment was updated to be honest that it remains best-effort until then — good.
- The
mysqlHandshakeV10refactor is behavior-preserving: the exported constants are the same 8×0x61+ 12×0x62+0x00bytes the localauthData1/2used to build. - All CodeRabbit nits were addressed in 794d5eb; the bug-hunting pass on this revision found nothing.
There was a problem hiding this comment.
The scramble-order fix in 99e0fb8 and the new wire-level scramble test fully address my earlier note — both native changes now match the spec and every reference driver I checked; deferring only because this is auth/crypto-path code that warrants a maintainer's sign-off.
Extended reasoning...
Overview
Two tightly-coupled fixes to MySQL 8's caching_sha2_password fast-auth path, plus test infrastructure:
src/sql/mysql/protocol/Auth.rs(2-line swap):caching_sha2_password::scramblenow hashesSHA256(SHA256(password)) || nonceinstead ofnonce || SHA256(SHA256(password)). Verified against MySQL'sGenerate_scramble(sha2_password_common.cc), mysql2, go-sql-driver/mysql, Connector/J, and Connector/Python — all concatenate digest2 first. This was the pre-existing bug I flagged in my earlier inline comment; the author addressed it exactly as suggested and added a byte-level wire test.src/sql_jsc/mysql/MySQLConnection.rs(~6 lines removed): theFastAuthStatus::SUCCESSarm no longer prematurely entersConnected; it stays inAuthenticatingso the trailing OK packet the server always sends is consumed by the existingPacketType::OKarm above. Matches the documented "Fast path succeeds" exchange.- Tests: a container test (best-effort until #28161 lands), a scripted
test.eachcovering both TCP framings of0x03+OK, and a scripted test that reads theauth_responseoffHandshakeResponse41and asserts it byte-for-byte against the spec computation (accepting both 20- and 21-byte nonces so it survives #28161 landing). wire-frames.ts: new mock-server helpers (mysqlAuthMoreData,mysqlTextResultSet,mysqlParseHandshakeResponse41,mysqlReadLenencInt, exported auth-data constants, optional 0xFE header onmysqlOkPacket). All follow the file's existing spec-comment-per-helper convention.
Security risks
This is client-side authentication-proof generation and auth-state sequencing. The scramble is a client→server proof; getting it wrong causes the server to reject fast auth and fall back to full authentication (today's behavior), never to accept a bad credential. The state-machine change delays the Connected transition by one packet; if a non-conforming server never sent the trailing OK, the connection would stall in Authenticating rather than accept unauthenticated commands. Neither change weakens a trust decision, exposes credentials, or bypasses a check — the failure modes are strictly toward more rejection, not less. No new user-controlled input reaches the crypto path.
Level of scrutiny
Higher than the diff size suggests: this is the MySQL authentication state machine and a SHA-256 scramble computation, both squarely in the auth/crypto category where I do not auto-approve. That said, the actual change surface is tiny (a two-line operand swap and the removal of a premature state transition), both are cross-referenced against five independent reference implementations plus the MySQL spec, and the scripted tests pin the exact wire bytes deterministically without Docker.
Other factors
- My earlier inline concern (scramble concatenation order) was addressed in 99e0fb8 with exactly the suggested fix plus a dedicated byte-level test; the thread is genuinely resolved.
- The author correctly identified, scoped out, and documented a third defect in the same path (21-byte nonce, #26195/#28161) rather than silently duplicating an open community fix, and wrote the scramble test to keep passing once that lands.
- CodeRabbit's comment-length nits were addressed in 794d5eb.
- No CODEOWNERS entry covers
src/sql/orsrc/sql_jsc/. - The bug-hunting pass found nothing.
- CI was retriggered on 35baa39; I did not verify final CI status.
I'm confident the changes are correct, but auth-protocol code should get a maintainer's eyes before merge.
|
CI summary for a reviewing maintainer. The diff is green.
Job totals: 67552 had 283 passed / 3 failed, 67571 had 282 passed / 4 failed. I used the one retrigger (35baa39) and will not keep pushing empty commits. Nothing that is red belongs to this diff. |
2 participants
What does this PR do?
Fixes two bugs in MySQL 8's
caching_sha2_passwordfast authentication, the exchange a passworded user's second and later connections take once the server's auth cache is warm.1. The scramble concatenated its operands in the wrong order (
src/sql/mysql/protocol/Auth.rs).The client proves cache membership by sending
XOR(SHA256(password), SHA256(SHA256(SHA256(password)) || nonce)). Bun hashedSHA256(nonce || SHA256(SHA256(password)))instead, likely copied frommysql_native_password, which genuinely puts the nonce first. MySQL's ownGenerate_scramble(sha2_password_common.cc), mysql2, go-sql-driver/mysql, Connector/J, and Connector/Python all hash the double SHA-256 first, as did the comment on the line itself.Because the scramble never matched, a real MySQL 8 with a warm cache rejected every fast-auth attempt and fell back to
perform_full_authentication(0x04) instead of answeringfast_auth_success(0x03). So the fast path was dead code: every Bun connection to MySQL 8 did the slow full authentication (RSA key exchange or plaintext over TLS), warm cache or not. Credit to the review below for catching this.2. The fast-auth success marker was treated as the end of authentication (
src/sql_jsc/mysql/MySQLConnection.rs).Per the "Fast path succeeds" exchange, the server answers the handshake response with an
AuthMoreDatapacket carryingfast_auth_success(0x03) and then sends theOKpacket that actually concludes authentication.handle_authentered the command phase on the 0x03 marker alone, so that trailing OK was routed tohandle_command: with no query in flight it killed the connection withERR_MYSQL_UNEXPECTED_PACKET, and with one racing ahead it became that query's (empty) result.These two have to land together. Fixing only the scramble makes the 0x03 path reachable and turns the second bug into a regression that breaks every warm-cache connection; fixing only the 0x03 handling leaves it on a path the scramble bug keeps unreachable.
Deliberately excluded: the nonce length
auth_dataalso keeps the handshake's trailing filler byte, so the nonce Bun scrambles against is 21 bytes where the server uses 20. That is a third defect in the same path, but it is #26195 with an open fix in #28161, so it is not duplicated here. Until #28161 also lands, a real server still rejects the (now correctly ordered) scramble and falls back to full authentication, which is exactly today's behavior and is harmless.Reproduction
Scripted server replaying the warm-cache exchange (handshake advertising
caching_sha2_password, thenAuthMoreData(0x03)followed byOK, then answeringCOM_QUERYwith a one-row result set):Reproduces on 1.4.0 and current main for both TCP framings (marker and OK in separate segments, or coalesced into one). The scramble bug reproduces by reading the
auth_responsebytes off theHandshakeResponse41the client sends and comparing them to the spec computation.Tests
All in
test/js/sql/sql-mysql.auth.test.ts; every frame comes fromtest/js/sql/wire-frames.ts.0x03+OKpair: the query must return its real rows, andCOM_QUERYmust only be sent once authentication has completed.HandshakeResponse41off the wire: the scramble must beXOR(SHA256(pw), SHA256(SHA256(SHA256(pw)) || nonce)). The assertion accepts both the 20- and 21-byte nonce so that it pins the concatenation order and keeps passing once fix(sql/mysql): fix caching_sha2_password auth for passwords > 19 chars #28161 lands.caching_sha2_passworduser, connects once with full auth to warm the server's cache, then reconnects and queries. This becomes fully load-bearing for the fast path once fix(sql/mysql): fix caching_sha2_password auth for passwords > 19 chars #28161 also lands; until then it degrades to full authentication.wire-frames.ts:mysqlAuthMoreData,mysqlParseHandshakeResponse41,mysqlReadLenencInt,mysqlTextResultSet/Row, and an optional0xFEheader onmysqlOkPacket.Correction on related issues
An earlier revision of this description linked #27102 and #26235 as likely explained by the 0x03 bug. That was wrong: because of the scramble bug above, the 0x03 path has never been reachable against a real server, so neither issue can have been caused by it. Both links are withdrawn.