feat(auth): support client-pinned caching SHA-2 RSA authentication - #6032
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (3)test/tap/tests/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/tap/tests/unit/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-01-20T09:34:19.124ZApplied to files:
📚 Learning: 2026-04-01T21:27:00.297ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe change adds client-pinned RSA key support for non-TLS ChangesCaching SHA-2 RSA authentication
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MySQLCLI
participant ProxySQL
participant MySQLProtocol
participant RSAKeySnapshot
MySQLCLI->>ProxySQL: Start caching_sha2_password authentication
ProxySQL->>MySQLProtocol: Process 0x04 challenge
MySQLProtocol->>RSAKeySnapshot: Capture active RSA key
MySQLProtocol-->>MySQLCLI: Send requested public key when requested
MySQLCLI->>MySQLProtocol: Send key-request or pinned-key ciphertext
MySQLProtocol->>RSAKeySnapshot: Decrypt response with snapshot
MySQLProtocol-->>ProxySQL: Advance authentication or report failure
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/MySQL_Protocol.cpp`:
- Around line 2883-2885: Move capture_caching_sha2_rsa_snapshot() before
generate_one_byte_pkt() in both authentication paths at
lib/MySQL_Protocol.cpp:2883-2885 and 2953-2955, and reset the snapshot whenever
packet generation fails. Update the challenge-ordering requirement in
docs/superpowers/specs/2026-08-11-caching-sha2-server-public-key-path-design.md:63-72
and the implementation sequence and failure-cleanup guidance in
docs/superpowers/plans/2026-08-11-caching-sha2-server-public-key-path.md:255-277.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fc9a5e1-a8ab-4366-b2cc-354c4e1bfaba
📒 Files selected for processing (6)
doc/caching_sha2_password_rsa.mddocs/superpowers/plans/2026-08-11-caching-sha2-server-public-key-path.mddocs/superpowers/specs/2026-08-11-caching-sha2-server-public-key-path-design.mdinclude/MySQL_Protocol.hlib/MySQL_Protocol.cpptest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Gitar
- GitHub Check: run / trigger
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (3)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/MySQL_Protocol.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
include/MySQL_Protocol.htest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpplib/MySQL_Protocol.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp
🧠 Learnings (4)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/plans/2026-08-11-caching-sha2-server-public-key-path.mddoc/caching_sha2_password_rsa.mddocs/superpowers/specs/2026-08-11-caching-sha2-server-public-key-path-design.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/plans/2026-08-11-caching-sha2-server-public-key-path.mddoc/caching_sha2_password_rsa.mddocs/superpowers/specs/2026-08-11-caching-sha2-server-public-key-path-design.md
📚 Learning: 2026-04-11T13:16:05.854Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:16:05.854Z
Learning: When validating GitHub-rendered Markdown in this repository (e.g., links that use heading anchors), account for GitHub slug behavior for headings containing an em-dash (—) surrounded by spaces: GitHub strips the em-dash and converts each surrounding space into a hyphen independently, which can produce a double hyphen (--) in the generated anchor. Therefore, do NOT flag as broken links any anchors whose expected slug contains a double hyphen specifically attributable to an em-dash surrounded by spaces in the source heading. (Example: `...vocabulary — read...` -> `...vocabulary--read...`.)
Applied to files:
doc/caching_sha2_password_rsa.md
🪛 LanguageTool
docs/superpowers/plans/2026-08-11-caching-sha2-server-public-key-path.md
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-keyand--server-public-key-path. - ProxySQL 4.0 must support both paths because PR...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/superpowers/specs/2026-08-11-caching-sha2-server-public-key-path-design.md
[style] ~16-~16: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...PROXYSQL40=1impliesPROXYSQL31=1`. - ProxySQL Stable 3.0 must not expose or execute t...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (12)
docs/superpowers/specs/2026-08-11-caching-sha2-server-public-key-path-design.md (1)
1-60: LGTM!Also applies to: 74-200
docs/superpowers/plans/2026-08-11-caching-sha2-server-public-key-path.md (1)
1-254: LGTM!Also applies to: 279-550
include/MySQL_Protocol.h (1)
232-241: LGTM!lib/MySQL_Protocol.cpp (1)
1954-2025: LGTM!Also applies to: 2027-2031, 2051-2066, 2098-2110
test/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp (5)
11-32: LGTM!
79-114: LGTM!
132-167: LGTM!Also applies to: 222-264
366-371: LGTM!
331-363: 🎯 Functional CorrectnessRemove this comment.
test/tap/groups/groups.jsonexcludesreg_test_5988-caching_sha2_rsa-tbelow ProxySQL 3.1 through@proxysql_min_version:3.1.> Likely an incorrect or invalid review comment.doc/caching_sha2_password_rsa.md (3)
3-8: LGTM!Also applies to: 18-18
73-88: LGTM!
90-138: LGTM!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6032 +/- ##
==========================================
+ Coverage 53.39% 53.42% +0.02%
==========================================
Files 492 492
Lines 146280 146307 +27
Branches 36965 36972 +7
==========================================
+ Hits 78113 78159 +46
+ Misses 50995 50968 -27
- Partials 17172 17180 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
Gitar fix CI |
On it — working on fixing the CI failure now. |
Code Review ✅ Approved 1 resolved / 1 findingsAdds client-pinned caching SHA-2 RSA authentication support with isolated key snapshots for safe rotation, addressing the inconsistent RSA-unavailable error handling. No issues found. ✅ 1 resolved✅ Quality: RSA-unavailable error set inconsistently across reject paths
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
Summary
--server-public-key-pathfor non-TLScaching_sha2_passwordfull authentication--get-server-public-keyrequest/response flowPROXYSQL31, making it available in ProxySQL 3.1 and 4.0 but not 3.0This is a follow-up to #6017. That work supports clients that request ProxySQL's public key in-band. Oracle MySQL clients configured with
--server-public-key-pathskip that request and send the RSA ciphertext immediately, which previously caused ProxySQL to reject the authentication packet.Validation
The focused test checks both
--get-server-public-keyand--server-public-key-path, rejects an incorrect pinned-key password with 1045, and verifies that the recovered password is not exposed through internal-session output.Summary by CodeRabbit
New Features
caching_sha2_passwordRSA authentication with requested or client-pinned public keys.Bug Fixes
Documentation
Tests