Add caching_sha2_password RSA public-key authentication - #6017
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PROXYSQL31 RSA key management and encrypted ChangesCaching SHA-2 RSA authentication
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
<cstdlib>formkdtemp.
mkdtempis declared in<stdlib.h>. The file relies on a transitive include fromtap.hor the OpenSSL headers. Add the include so the translation unit is self-contained.♻️ Proposed include addition
`#include` <sys/stat.h> `#include` <unistd.h> +#include <cstdlib> `#include` <memory> `#include` <string> `#include` <thread> `#include` <vector>🤖 Prompt for 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. In `@test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp` around lines 5 - 11, Add the standard <cstdlib> include alongside the existing headers in the caching_sha2_rsa unit test so mkdtemp is declared directly by this translation unit, without relying on transitive includes.include/MySQL_Caching_Sha2_RSA.h (2)
52-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a pthread mutex for consistency with the repository convention.
The repository standard is pthread mutexes for synchronization.
mutex_usesstd::mutex. The lock is short-held andacquire()runs on the frontend authentication path, so the behavior is acceptable, but the choice deviates from the convention. Either switch topthread_mutex_t(orrwlock) or record the rationale in a comment.As per coding guidelines: "Use pthread mutexes for synchronization and
std::atomic<>for counters."🤖 Prompt for 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. In `@include/MySQL_Caching_Sha2_RSA.h` around lines 52 - 54, Replace the std::mutex member mutex_ in the MySQL_Caching_Sha2_RSA synchronization state with the repository-standard pthread mutex type, and update its initialization, locking, and cleanup in the associated class methods while preserving the existing short-held locking behavior.Source: Coding guidelines
10-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
MySQL_protocol prefix to the new public types.
CachingSha2RSAConfig,CachingSha2RSAKeySnapshot, andCachingSha2RSAReloadResultare MySQL-protocol types exposed in a public header. The guidelines require a protocol prefix.MySQL_Caching_Sha2_RSAalready follows the rule. Rename the three helper types for consistency, for exampleMySQL_CachingSha2RSAConfig. The rename also affectslib/MySQL_Thread.cpp,lib/MySQL_Protocol.cpp, and the unit tests.As per coding guidelines: "Class names must use
PascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_."🤖 Prompt for 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. In `@include/MySQL_Caching_Sha2_RSA.h` around lines 10 - 36, Rename the public types CachingSha2RSAConfig, CachingSha2RSAKeySnapshot, and CachingSha2RSAReloadResult to MySQL_CachingSha2RSAConfig, MySQL_CachingSha2RSAKeySnapshot, and MySQL_CachingSha2RSAReloadResult. Update all references in MySQL_Thread.cpp, MySQL_Protocol.cpp, and the unit tests while preserving their existing behavior.Source: Coding guidelines
lib/MySQL_Thread.cpp (1)
1612-1612: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftRSA key generation runs while
GloMTH->wrlock()is held.
flush_mysql_variables___database_to_runtimecallscommit()betweenGloMTH->wrlock()andGloMTH->wrunlock().reload()can generate an RSA-2048 keypair, write two files, and callfsyncthree times. Every worker thread takes the same write lock inMySQL_Thread::refresh_variables(), so all worker threads stall for the duration of the keygen and the disk syncs. The stall is worst on the firstLOAD MYSQL VARIABLES TO RUNTIMEafter an upgrade, when no keypair exists yet.Consider generating and loading the keypair outside the write lock and publishing only the resulting snapshot under the lock.
🤖 Prompt for 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. In `@lib/MySQL_Thread.cpp` at line 1612, Move the potentially expensive caching_sha2 RSA key generation and file I/O out of the GloMTH write-lock scope in flush_mysql_variables___database_to_runtime/commit. Prepare the result by calling caching_sha2_rsa_manager_->reload with rsa_config before acquiring the lock, then publish the completed snapshot under GloMTH->wrlock() while preserving existing commit behavior and error handling.
🤖 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 `@include/MySQL_Caching_Sha2_RSA.h`:
- Around line 1-2: Rename the include guard in the MySQL_Caching_Sha2_RSA header
from PROXYSQL_MYSQL_CACHING_SHA2_RSA_H to __CLASS_MYSQL_CACHING_SHA2_RSA_H,
updating both the `#ifndef` symbol and its matching `#define` while leaving the
guard’s scope unchanged.
In `@lib/Admin_FlushVariables.cpp`:
- Around line 577-581: The commit-time rejection path after GloMTH->commit()
leaves rejected RSA values in global_variables. When
commit_result.rejected_variables is nonzero, persist the restored accepted
caching_sha2_password values back to global_variables using the same write-back
behavior as flush_GENERIC_variables__process__database_to_runtime, before the
subsequent runtime-to-database flush.
In `@lib/MySQL_Caching_Sha2_RSA.cpp`:
- Around line 22-23: Rename the constants kMinimumRSAKeyBits and
kMaximumPEMFileSize to UPPER_SNAKE_CASE names, then update all three references
at the indicated use sites to match; preserve their values and behavior.
- Around line 687-690: Update the blocking flock call in the RSA key-generation
locking path to retry when it fails with EINTR, and only set the existing error
and return false for non-EINTR failures. Preserve the current error reporting
using error, errno_message, and private_path.
- Around line 857-874: In the RSA OAEP decryption flow, record the original
plaintext allocation size before plaintext.resize(plaintext_length), then use
that saved size for every OPENSSL_cleanse call in the success and
validation-failure paths. Keep the existing plaintext validation and password
assignment behavior unchanged while ensuring the entire allocated buffer is
cleansed before release.
In `@lib/MySQL_Protocol.cpp`:
- Around line 311-337: Update MySQL_Protocol::generate_auth_more_data to check
whether l_alloc(size) returns null before writing the packet header or payload.
On allocation failure, abort or fail the authentication path using the class’s
established error-handling mechanism, and only enqueue the buffer and update
pkt_sid after a successful allocation.
In `@lib/MySQL_Thread.cpp`:
- Line 1620: Replace the fixed `commit_result.rejected_variables = 3` assignment
in `commit()` with the names of the RSA variables rejected during processing.
Update `flush_mysql_variables___database_to_runtime` to intersect those names
with the resultset rows actually processed, adjusting `stats.updated` and
`stats.rejected` only for matching RSA variables.
---
Nitpick comments:
In `@include/MySQL_Caching_Sha2_RSA.h`:
- Around line 52-54: Replace the std::mutex member mutex_ in the
MySQL_Caching_Sha2_RSA synchronization state with the repository-standard
pthread mutex type, and update its initialization, locking, and cleanup in the
associated class methods while preserving the existing short-held locking
behavior.
- Around line 10-36: Rename the public types CachingSha2RSAConfig,
CachingSha2RSAKeySnapshot, and CachingSha2RSAReloadResult to
MySQL_CachingSha2RSAConfig, MySQL_CachingSha2RSAKeySnapshot, and
MySQL_CachingSha2RSAReloadResult. Update all references in MySQL_Thread.cpp,
MySQL_Protocol.cpp, and the unit tests while preserving their existing behavior.
In `@lib/MySQL_Thread.cpp`:
- Line 1612: Move the potentially expensive caching_sha2 RSA key generation and
file I/O out of the GloMTH write-lock scope in
flush_mysql_variables___database_to_runtime/commit. Prepare the result by
calling caching_sha2_rsa_manager_->reload with rsa_config before acquiring the
lock, then publish the completed snapshot under GloMTH->wrlock() while
preserving existing commit behavior and error handling.
In `@test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp`:
- Around line 5-11: Add the standard <cstdlib> include alongside the existing
headers in the caching_sha2_rsa unit test so mkdtemp is declared directly by
this translation unit, without relying on transitive includes.
🪄 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: d2a0eadf-d40c-4d1d-96df-cccd8fde319d
📒 Files selected for processing (23)
doc/caching_sha2_password_rsa.mddoc/internal/passthrough_authentication.mdinclude/MySQL_Caching_Sha2_RSA.hinclude/MySQL_Passthrough_Auth_Cache.hinclude/MySQL_Protocol.hinclude/MySQL_Thread.hinclude/mysql_connection.hlib/Admin_FlushVariables.cpplib/Makefilelib/MySQL_Authentication.cpplib/MySQL_Caching_Sha2_RSA.cpplib/MySQL_Passthrough_Auth_Cache.cpplib/MySQL_Protocol.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpplib/mysql_connection.cpplib/mysql_data_stream.cpptest/tap/groups/groups.jsontest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/protocol_unit-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/MySQL_Passthrough_Auth_Cache.hinclude/mysql_connection.hinclude/MySQL_Caching_Sha2_RSA.hinclude/MySQL_Protocol.hinclude/MySQL_Thread.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_Passthrough_Auth_Cache.hlib/mysql_connection.cppinclude/mysql_connection.hinclude/MySQL_Caching_Sha2_RSA.hlib/Admin_FlushVariables.cpplib/mysql_data_stream.cpplib/MySQL_Authentication.cpptest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpplib/MySQL_Passthrough_Auth_Cache.cppinclude/MySQL_Protocol.htest/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cppinclude/MySQL_Thread.hlib/MySQL_Session.cpptest/tap/tests/unit/protocol_unit-t.cpplib/MySQL_Caching_Sha2_RSA.cpplib/MySQL_Thread.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.cpptest/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/protocol_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/protocol_unit-t.cpp
🧠 Learnings (5)
📚 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/internal/passthrough_authentication.mddoc/caching_sha2_password_rsa.md
📚 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:
doc/internal/passthrough_authentication.mddoc/caching_sha2_password_rsa.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:
doc/internal/passthrough_authentication.mddoc/caching_sha2_password_rsa.md
📚 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.cpptest/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/protocol_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/protocol_unit-t.cpp
🪛 ast-grep (0.45.0)
lib/MySQL_Session.cpp
[error] 6515-6519: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(
_s,
"ProxySQL Error: Access denied for user '%s'@'%s': caching_sha2_password RSA key exchange is unavailable; use TLS or configure RSA keys",
client_myds->myconn->userinfo->username, client_addr
)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
[error] 6523-6523: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(_s,"ProxySQL Error: Access denied for user '%s'@'%s' (using password: %s)", client_myds->myconn->userinfo->username, client_addr, (client_myds->myconn->userinfo->password ? "YES" : "NO"))
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🪛 Cppcheck (2.21.0)
lib/MySQL_Passthrough_Auth_Cache.cpp
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/mysql_variables_unit-t.cpp
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/MySQL_Protocol.cpp
[warning] 323-323: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 324-324: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[error] 326-326: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
🪛 LanguageTool
doc/caching_sha2_password_rsa.md
[style] ~48-~48: Consider replacing this word to strengthen your wording.
Context: ...The private file must be a regular file and must not grant any group or other permi...
(AND_THAT)
🔇 Additional comments (82)
include/mysql_connection.h (1)
58-58: LGTM!include/MySQL_Protocol.h (4)
9-19: LGTM!
126-128: LGTM!
158-161: LGTM!
235-238: LGTM!lib/MySQL_Protocol.cpp (17)
17-20: LGTM!
47-75: LGTM!
96-102: LGTM!
121-122: LGTM!
133-136: LGTM!
288-290: LGTM!
1626-1629: LGTM!Also applies to: 1678-1678, 1690-1690
1775-1824: LGTM!
1841-1913: LGTM!
2374-2376: LGTM!Also applies to: 2385-2387, 2418-2419
2705-2705: LGTM!
2753-2753: LGTM!
3024-3031: LGTM!
3430-3435: LGTM!
3478-3478: LGTM!Also applies to: 3487-3487
3500-3513: LGTM!
3526-3532: LGTM!lib/MySQL_Session.cpp (4)
15-16: LGTM!
1788-1788: LGTM!Also applies to: 1920-1920, 1933-1933, 2130-2130
6464-6467: LGTM!
6500-6534: LGTM!include/MySQL_Passthrough_Auth_Cache.h (2)
40-42: LGTM!
113-113: LGTM!lib/MySQL_Passthrough_Auth_Cache.cpp (3)
8-23: LGTM!
65-65: LGTM!
134-139: LGTM!lib/MySQL_Authentication.cpp (5)
12-38: LGTM!
169-176: LGTM!
521-524: LGTM!
598-603: LGTM!
720-723: LGTM!lib/mysql_connection.cpp (4)
11-11: LGTM!
270-282: LGTM!
347-347: LGTM!
366-366: LGTM!lib/mysql_data_stream.cpp (3)
17-17: LGTM!
403-405: LGTM!
1934-1934: LGTM!doc/caching_sha2_password_rsa.md (1)
1-98: LGTM!doc/internal/passthrough_authentication.md (1)
282-289: LGTM!test/tap/groups/groups.json (1)
18-18: LGTM!Also applies to: 288-288
test/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp (4)
104-110: LGTM!Also applies to: 272-301
241-246: 🎯 Functional CorrectnessNo change needed.
lib/MySQL_Session.cpp:6518emitscaching_sha2_password RSA key exchange is unavailable, so the substring asserted intest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp:245is present.
80-102: 🎯 Functional CorrectnessNo change needed for the argv or timeout options.
wexecvpinserts thefileargument beforeexecvp, soargsshould not include an argv[0].execvprequires the full command name inhelp_args, andwexecvpcopiesoptsbefore reading its fields.> Likely an incorrect or invalid review comment.
130-134: 📐 Maintainability & Code QualityNo action needed.
no-infra-g1does not exportREGULAR_INFRA_DATADIR, butno-infra-g1/infras.lstis empty, so this test is not intended to rely on generatedtest/infraartifacts in that group.test/tap/tests/unit/Makefile (1)
434-437: LGTM!test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp (4)
246-365: LGTM!
367-513: LGTM!
515-541: LGTM!
1-16: 🩺 Stability & AvailabilityUpdate this comment to remove the globals initialization concern.
This unit test does not need
test_globals.h/test_init.hforMySQL_Caching_Sha2_RSA::reload()ordecrypt_password(), because those implementations return errors throughCachingSha2RSAReloadResult::errorand a callback without logging or accessing ProxySQL globals.> Likely an incorrect or invalid review comment.test/tap/tests/unit/mysql_variables_unit-t.cpp (3)
5-22: LGTM!Also applies to: 34-57
90-98: LGTM!Also applies to: 112-202, 204-214
109-110: 🩺 Stability & AvailabilityNo change needed.
test_globals_init()assignsGloVars.datadirwithstrdup(), andtest_globals_cleanup()frees that same heap ownership.test/tap/tests/unit/protocol_unit-t.cpp (5)
227-255: LGTM!
315-326: LGTM!
459-467: LGTM!Also applies to: 512-549
35-35: 📐 Maintainability & Code QualityNo change needed for
get_masked_pass.
get_masked_passhas external linkage and the declared signature matches the definition; moving this local declaration to a header is optional maintainability work only.
275-304: 🩺 Stability & AvailabilityNo change needed.
PPHR_1only skipssizeof(mysql_hdr)in the RSA-response branch used at stage 6 and otherwise callsstd::memchrwithpayload_lengthexactly as the caller passed it;MyProt_tmp_auth_varshaspass = NULL.> Likely an incorrect or invalid review comment.include/MySQL_Caching_Sha2_RSA.h (2)
38-50: LGTM!
8-8: 🩺 Stability & AvailabilityNo change needed. ProxySQL requires OpenSSL >= 3.0.0 in its build checks and packaging, so using
<openssl/types.h>and the OpenSSL 3.0EVP_PKEY_*_checkfunctions is consistent with the supported build target.lib/MySQL_Caching_Sha2_RSA.cpp (7)
102-173: LGTM!
183-270: LGTM!
272-301: LGTM!
398-407: 🎯 Functional Correctness | ⚡ Quick winConfirm that rejecting PKCS#1 private keys is intended.
has_single_pem_envelopeaccepts only-----BEGIN PRIVATE KEY-----, so only unencrypted PKCS#8 is loaded. MySQL Server generatesprivate_key.pemin PKCS#1 form (-----BEGIN RSA PRIVATE KEY-----). An operator who reuses an existing MySQL keypair gets a rejected configuration. If that restriction is intended, state it indoc/caching_sha2_password_rsa.md. If not, also accept the PKCS#1 envelope throughPEM_read_bio_PrivateKey.
607-724: LGTM!
735-806: LGTM!
808-856: LGTM!lib/Makefile (1)
124-127: LGTM!include/MySQL_Thread.h (2)
16-17: LGTM!Also applies to: 43-50, 440-446, 541-545, 820-822
815-815: 🎯 Functional CorrectnessNo change needed. The
MySQL_Threads_Handler::commit()return value is consumed at the production reload path that reports flush statistics. Remaining callers are shutdown/pause/resume control paths that do not use flush statistics.lib/MySQL_Thread.cpp (6)
32-34: LGTM!Also applies to: 496-500, 1470-1477
1622-1673: LGTM!
1874-1877: LGTM!Also applies to: 1980-1988
2482-2493: LGTM!
2810-2813: LGTM!
3397-3400: LGTM!lib/Admin_FlushVariables.cpp (1)
579-579: 🩺 Stability & AvailabilityNo change needed.
lib/Admin_HlushVariables.cppincludes<algorithm>directly, sostd::maxis declared here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6017 +/- ##
===========================================
+ Coverage 14.36% 53.38% +39.02%
===========================================
Files 154 487 +333
Lines 82435 145258 +62823
Branches 0 36704 +36704
===========================================
+ Hits 11841 77552 +65711
+ Misses 70594 50720 -19874
- Partials 0 16986 +16986
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-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const auto key_snapshot = caching_sha2_rsa_snapshot_; | ||
| caching_sha2_rsa_snapshot_.reset(); | ||
| const size_t ciphertext_length = | ||
| len >= sizeof(mysql_hdr) ? len - sizeof(mysql_hdr) : 0; | ||
| if (key_snapshot == nullptr || | ||
| ciphertext_length != key_snapshot->ciphertext_size() || | ||
| GloMTH == nullptr || GloMTH->caching_sha2_rsa() == nullptr) { | ||
| proxy_debug(PROXY_DEBUG_MYSQL_AUTH, 5, | ||
| "Session=%p , DS=%p , user='%s' . Invalid caching_sha2_password RSA response\n", | ||
| (*myds)->sess, (*myds), vars1.user); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
💡 Quality: Snapshot rotated mid-exchange yields generic 1045, not RSA hint
If a concurrent LOAD MYSQL VARIABLES TO RUNTIME disables/rotates the key between the public-key response (stage 5) and the RSA response (stage 6), the stage-6 guard returns failure without setting frontend_auth_error_ = CACHING_SHA2_RSA_UNAVAILABLE. The client then receives the generic access-denied message rather than the documented TLS-or-key hint. This is a minor UX inconsistency versus the stage-5 unavailable path; functionally still a correct rejection. Consider setting the RSA-unavailable error when the snapshot/manager is gone at stage 6.
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/tap/tests/unit/mysql_variables_unit-t.cpp (1)
230-236: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCleanup misses the default key lock file, so the temporary directory is not removed.
The second handler block commits with the default key paths. That path attempts generation and creates a lock file next to the private key. The unit test at
test/tap/tests/unit/caching_sha2_rsa_unit-t.cppLine 235 confirms the same<private-key>.locknaming for the explicitrsa-private.pemcase.Line 230 removes
proxysql-caching-sha2-private-key.pem, but nothing removesproxysql-caching-sha2-private-key.pem.lock. Thermdiron Line 236 then fails and leaves the directory under/tmpon every run.🧹 Proposed cleanup fix
unlink(default_private.c_str()); const std::string directory = temporary_directory; + unlink((directory + "/proxysql-caching-sha2-private-key.pem.lock").c_str()); + unlink((directory + "/proxysql-caching-sha2-public-key.pem").c_str()); unlink((directory + "/rsa-private.pem").c_str()); unlink((directory + "/rsa-public.pem").c_str()); unlink((directory + "/rsa-private.pem.lock").c_str()); rmdir(directory.c_str());🤖 Prompt for 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. In `@test/tap/tests/unit/mysql_variables_unit-t.cpp` around lines 230 - 236, Update the cleanup block after the default-key test to also unlink the default private key’s lock file, using the existing default key path and the established “.lock” suffix pattern before calling rmdir. Preserve the current cleanup of the key files and directory.
🧹 Nitpick comments (1)
test/tap/tests/unit/mysql_variables_unit-t.cpp (1)
256-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord why
adminis not deleted.Line 322 deletes
admin->admindband sets it tonullptr, butadminitself is never deleted. The partially constructedProxySQL_Adminis created withnewand onlyadmindbis populated, so callingdelete adminwould likely run a destructor against uninitialized members. Add a one-line comment that states this intent. A reader otherwise reads the missingdeleteas an oversight.Leaking the object in a short-lived TAP process is acceptable per the established convention for
test/tap/tests.Based on learnings: "In ProxySQL's TAP test suite, resource leaks ... are commonly tolerated because test processes are short-lived and OS frees resources on exit."
🤖 Prompt for 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. In `@test/tap/tests/unit/mysql_variables_unit-t.cpp` around lines 256 - 290, Add a one-line comment near the cleanup of admin->admindb in test_caching_sha2_rsa_rejection_restores_database_values explaining that the partially initialized ProxySQL_Admin is intentionally not deleted, as leaks are acceptable in short-lived TAP tests.Source: Learnings
🤖 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/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp`:
- Around line 248-266: Update the rejected-update assertion in the RSA LOAD test
around rejected_update_ok so it validates the explicit invalid boolean rejection
rather than requiring “Rejected: 3”. Alternatively, adjust the info-text
accounting to include unchanged path variables only after statistics are formed,
while preserving coverage that the invalid auto-generate setting is rejected.
In `@test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp`:
- Around line 655-665: The publication-race test can hang when flock
interposition is unavailable. In
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp:655-665, add the chrono include
in the Linux include block and replace the unbounded condition_variable wait
with a bounded wait_for that fails the test on timeout; in
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp:67-104, document that the Line
88 probe temporarily releases the intercepted descriptor lock. Also verify the
target’s test/tap/tests/unit/Makefile link flags pass -Wl,--wrap=flock.
---
Outside diff comments:
In `@test/tap/tests/unit/mysql_variables_unit-t.cpp`:
- Around line 230-236: Update the cleanup block after the default-key test to
also unlink the default private key’s lock file, using the existing default key
path and the established “.lock” suffix pattern before calling rmdir. Preserve
the current cleanup of the key files and directory.
---
Nitpick comments:
In `@test/tap/tests/unit/mysql_variables_unit-t.cpp`:
- Around line 256-290: Add a one-line comment near the cleanup of admin->admindb
in test_caching_sha2_rsa_rejection_restores_database_values explaining that the
partially initialized ProxySQL_Admin is intentionally not deleted, as leaks are
acceptable in short-lived TAP tests.
🪄 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: 331b45fe-b568-42d8-8cb9-cac5040149f7
📒 Files selected for processing (17)
include/MySQL_Caching_Sha2_RSA.hinclude/MySQL_Passthrough_Auth_Cache.hinclude/MySQL_Protocol.hinclude/MySQL_Thread.hinclude/mysql_connection.hinclude/proxysql_admin.hlib/Admin_FlushVariables.cpplib/Admin_Handler.cpplib/MySQL_Caching_Sha2_RSA.cpplib/MySQL_Protocol.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpptest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/protocol_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- include/mysql_connection.h
- test/tap/tests/unit/protocol_unit-t.cpp
- lib/MySQL_Session.cpp
- include/MySQL_Protocol.h
- lib/MySQL_Caching_Sha2_RSA.cpp
- lib/MySQL_Protocol.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: build
- GitHub Check: Gitar
🧰 Additional context used
📓 Path-based instructions (4)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/proxysql_admin.hinclude/MySQL_Caching_Sha2_RSA.hinclude/MySQL_Thread.hinclude/MySQL_Passthrough_Auth_Cache.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/proxysql_admin.hlib/Admin_FlushVariables.cppinclude/MySQL_Caching_Sha2_RSA.htest/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cppinclude/MySQL_Thread.hlib/Admin_Handler.cppinclude/MySQL_Passthrough_Auth_Cache.htest/tap/tests/unit/caching_sha2_rsa_unit-t.cpplib/MySQL_Thread.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/unit/mysql_variables_unit-t.cpptest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
🧠 Learnings (2)
📚 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/unit/mysql_variables_unit-t.cpptest/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/mysql_variables_unit-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
🪛 ast-grep (0.45.0)
lib/Admin_Handler.cpp
[error] 827-827: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(buf, "%d", admin_old_wait_timeout)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
[error] 886-886: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(buf,"%d",admin_old_wait_timeout)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🪛 checkmake (0.3.2)
test/tap/tests/unit/Makefile
[warning] 865-865: Target "caching_sha2_rsa_unit-t" should be declared PHONY.
(phonydeclared)
🪛 Cppcheck (2.21.0)
test/tap/tests/unit/mysql_variables_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (26)
test/tap/tests/unit/Makefile (2)
434-437: LGTM!
863-866: LGTM!include/MySQL_Caching_Sha2_RSA.h (1)
1-25: LGTM!Also applies to: 38-80
include/MySQL_Thread.h (1)
18-18: LGTM!Also applies to: 48-51, 446-449, 544-548, 818-836
lib/MySQL_Thread.cpp (1)
1599-1767: LGTM!Also applies to: 1896-1899, 2002-2010, 2504-2516, 2832-2835, 3419-3422
lib/Admin_FlushVariables.cpp (1)
175-272: LGTM!Also applies to: 465-617
include/proxysql_admin.h (1)
526-539: LGTM!lib/Admin_Handler.cpp (1)
824-829: LGTM!Also applies to: 880-888, 932-944
include/MySQL_Passthrough_Auth_Cache.h (3)
118-122: 🔒 Security & PrivacyReplacement cleanup is already handled.
38-48: 🎯 Functional CorrectnessNo change needed.
entries[username]constructs the mappedentry_tin place, theninsert()updates fields by reference without invoking copy or move semantics.
118-122: 🗄️ Data Integrity & IntegrationNo change needed.
The single
insertpath passes astrdup-allocated, NUL-terminated cleartext credential frompassthrough_cleartext, and reject branches preventcleartext == NULLor empty credentials before caching.test/tap/tests/reg_test_5988-caching_sha2_rsa-t.cpp (4)
119-134: LGTM!
150-161: LGTM!
268-298: LGTM!
300-354: LGTM!test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp (5)
307-343: LGTM!
117-305: LGTM!
393-565: LGTM!
567-605: LGTM!
607-654: LGTM!Also applies to: 666-684
test/tap/tests/unit/mysql_variables_unit-t.cpp (6)
334-346: LGTM!
86-88: LGTM!
240-254: LGTM!
291-331: LGTM!
124-229: LGTM!
5-43: LGTM!
| bool observed_contended_attempt = false; | ||
| { | ||
| std::unique_lock<std::mutex> lock(publication_lock_observer_mutex); | ||
| publication_lock_observer_cv.wait(lock, []() { | ||
| return publication_lock_attempted || publication_reload_finished; | ||
| }); | ||
| observed_contended_attempt = publication_lock_attempted && | ||
| publication_lock_contended && !publication_reload_finished; | ||
| } | ||
| ok(observed_contended_attempt, | ||
| "auto-generating reload waits for a locked partial-pair publisher"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The publication-race test depends on --wrap=flock interposition, and it hangs when that interposition is absent. The observer state is only updated inside __wrap_flock. The main thread holds the lock and waits for that state. If the linker does not apply -Wl,--wrap=flock, or the reload path uses LOCK_NB, neither thread makes progress.
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp#L655-L665: replacewaitwith a boundedwait_forso the test fails instead of hanging, and add#include <chrono>to the__linux__include block.test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp#L67-L104: confirm thattest/tap/tests/unit/Makefilepasses-Wl,--wrap=flockfor this target, and document that the probe on Line 88 temporarily releases the lock on the intercepted descriptor.
📍 Affects 1 file
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp#L655-L665(this comment)test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp#L67-L104
🤖 Prompt for 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.
In `@test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp` around lines 655 - 665, The
publication-race test can hang when flock interposition is unavailable. In
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp:655-665, add the chrono include
in the Linux include block and replace the unbounded condition_variable wait
with a bounded wait_for that fails the test on timeout; in
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp:67-104, document that the Line
88 probe temporarily releases the intercepted descriptor lock. Also verify the
target’s test/tap/tests/unit/Makefile link flags pass -Wl,--wrap=flock.
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 `@test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp`:
- Around line 29-30: Update the temporary-directory setup around mkdtemp and
TempDir::path_ to fail the test immediately when directory creation returns
nullptr or produces an empty path. Ensure execution returns before the
subsequent reload and datadir setup, while preserving the existing behavior for
successfully created directories.
🪄 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: 4e0e880e-e39b-4048-ac0b-8a7240a4798c
📒 Files selected for processing (8)
include/MySQL_Protocol.hinclude/MySQL_Thread.hlib/Admin_FlushVariables.cpplib/MySQL_Protocol.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/Admin_FlushVariables.cpp
- lib/MySQL_Session.cpp
- include/MySQL_Protocol.h
- include/MySQL_Thread.h
- lib/MySQL_Thread.cpp
- lib/MySQL_Protocol.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpp
**/*.{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:
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpp
🧠 Learnings (2)
📚 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/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/unit/mysql_variables_unit-t.cpp
🪛 Cppcheck (2.21.0)
test/tap/tests/unit/mysql_variables_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (2)
test/tap/tests/unit/mysql_variables_unit-t.cpp (1)
7-15: LGTM!Also applies to: 24-31, 66-91, 108-110, 112-222, 224-309, 313-322
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp (1)
647-651: The publication-lock test still needs a bounded wait.This is the same finding as the previous review. The thread created here can leave the main thread waiting at Line 662 for
__wrap_flockto updatepublication_lock_attempted. If the target does not link-Wl,--wrap=flock, the test can hang instead of failing.#!/bin/bash set -euo pipefail rg -n -- '--wrap=flock|caching_sha2_rsa_unit' test/tap/tests/unit/Makefile rg -n -C 4 '__wrap_flock|publication_lock_attempted|publication_lock_observer_cv\.wait' \ test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 855abce977
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const MySQLThreadsCommitResult commit_result = GloMTH->commit(); | ||
| if (!commit_result.rejected_variables.empty()) { |
There was a problem hiding this comment.
When a LOAD submits an invalid boolean together with a valid or generatable private/public pair, the generic pass rejects only the boolean but still stages both paths; commit() then validates those paths using the previously accepted boolean, returns no rejected_variables, and publishes the partial update. This violates the documented all-or-nothing behavior of the three RSA variables and can even generate key files for a rejected configuration. Propagate generic rejection of any RSA member into the grouped commit so all submitted members retain their prior values.
Useful? React with 👍 / 👎.
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 `@docs/superpowers/plans/2026-08-10-auth-methods-rsa-version-gate.md`:
- Line 9: Update the Tech Stack declaration in the authentication methods plan
from C++11 to C++17, while preserving the MariaDB/MySQL C API and TAP test
helpers entries.
In `@test/tap/tests/test_auth_methods-t.cpp`:
- Around line 160-174: Update the version-parsing logic around parsed_major and
parsed_minor to reject negative values after std::from_chars succeeds. Preserve
the existing boundary assertions and return false for negative major or minor
components before assigning them to major and minor.
🪄 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: 5b48e84d-2aa3-47a4-82e7-5204101b8da0
📒 Files selected for processing (3)
docs/superpowers/plans/2026-08-10-auth-methods-rsa-version-gate.mddocs/superpowers/specs/2026-08-10-auth-methods-rsa-version-gate-design.mdtest/tap/tests/test_auth_methods-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/test_auth_methods-t.cpp
**/*.{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:
test/tap/tests/test_auth_methods-t.cpp
🧠 Learnings (3)
📚 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/specs/2026-08-10-auth-methods-rsa-version-gate-design.mddocs/superpowers/plans/2026-08-10-auth-methods-rsa-version-gate.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/specs/2026-08-10-auth-methods-rsa-version-gate-design.mddocs/superpowers/plans/2026-08-10-auth-methods-rsa-version-gate.md
📚 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/test_auth_methods-t.cpp
🔇 Additional comments (1)
docs/superpowers/specs/2026-08-10-auth-methods-rsa-version-gate-design.md (1)
1-26: LGTM!
Code Review 👍 Approved with suggestions 1 resolved / 2 findingsAdds caching_sha2_password RSA public-key authentication for non-TLS frontend connections with robust key validation and atomic rotation. Consider preserving the specific RSA unavailable error hint if a concurrent reload occurs mid-exchange. 💡 Quality: Snapshot rotated mid-exchange yields generic 1045, not RSA hint📄 lib/MySQL_Protocol.cpp:1781-1792 If a concurrent ✅ 1 resolved✅ Edge Case: Unchecked malloc for recovered RSA plaintext password
🤖 Prompt for agentsOptionsAuto-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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/tap/tests/test_auth_methods-t.cpp (1)
1258-1275: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate RSA expectations on configured key availability.
supports_rsaonly reflects ProxySQL 3.1+ version; it does not verify that a complete RSA key pair is accepted. Whencaching_sha2_password_auto_generate_rsa_keysis disabled with empty paths,MySQL_Caching_Sha2_RSA::reload()clearssnapshot_, so disabled RSA configuration can disable RSA authentication despite the version gate. Set/provide a valid RSA key pair in the test setup, or derive feasibility from the runtime key state before using the expectation helpers.🤖 Prompt for 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. In `@test/tap/tests/test_auth_methods-t.cpp` around lines 1258 - 1275, Update the RSA expectation setup around supports_caching_sha2_rsa to also require a usable configured RSA key pair, rather than relying only on the ProxySQL version. Ensure the test setup provides valid RSA key paths or inspect the runtime key state after reload, then use that result when determining RSA support expectations.
🧹 Nitpick comments (1)
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp (1)
643-672: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse pthread synchronization for the publication observer.
publication_lock_observer_mutexusesstd::mutexandstd::condition_variable. Replace this observer state withpthread_mutex_tandpthread_cond_t. Preserve the bounded timed wait.As per coding guidelines, C++ synchronization must use pthread mutexes.
🤖 Prompt for 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. In `@test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp` around lines 643 - 672, Replace the publication observer’s std::mutex and std::condition_variable synchronization with pthread_mutex_t and pthread_cond_t, updating the related lock, wait, notification, and initialization/cleanup operations around publication_lock_observer_enabled and publication_lock_observer_cv. Preserve the existing bounded five-second wait and observer-state behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/tap/tests/test_auth_methods-t.cpp`:
- Around line 1258-1275: Update the RSA expectation setup around
supports_caching_sha2_rsa to also require a usable configured RSA key pair,
rather than relying only on the ProxySQL version. Ensure the test setup provides
valid RSA key paths or inspect the runtime key state after reload, then use that
result when determining RSA support expectations.
---
Nitpick comments:
In `@test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp`:
- Around line 643-672: Replace the publication observer’s std::mutex and
std::condition_variable synchronization with pthread_mutex_t and pthread_cond_t,
updating the related lock, wait, notification, and initialization/cleanup
operations around publication_lock_observer_enabled and
publication_lock_observer_cv. Preserve the existing bounded five-second wait and
observer-state behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d59538-a63e-4f2a-b77d-2014ab627c2d
📒 Files selected for processing (5)
docs/superpowers/plans/2026-08-10-auth-methods-rsa-version-gate.mddocs/superpowers/specs/2026-08-10-auth-methods-rsa-version-gate-design.mdtest/tap/groups/groups.jsontest/tap/tests/test_auth_methods-t.cpptest/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/superpowers/specs/2026-08-10-auth-methods-rsa-version-gate-design.md
- test/tap/groups/groups.json
- docs/superpowers/plans/2026-08-10-auth-methods-rsa-version-gate.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/test_auth_methods-t.cpp
**/*.{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:
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/test_auth_methods-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
🧠 Learnings (2)
📚 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/unit/caching_sha2_rsa_unit-t.cpptest/tap/tests/test_auth_methods-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp
🔇 Additional comments (2)
test/tap/tests/test_auth_methods-t.cpp (1)
154-174: LGTM!Also applies to: 1415-1473
test/tap/tests/unit/caching_sha2_rsa_unit-t.cpp (1)
14-35: LGTM!Also applies to: 309-607, 693-693
|
1 participant
Summary
caching_sha2_password, including strict key validation, secure auto-generation, atomic publication, and immutable runtime snapshotsWhy
ProxySQL previously completed
caching_sha2_passwordfull authentication only over TLS. Clients using MySQL's--get-server-public-keyflow could not authenticate over a non-TLS frontend connection because ProxySQL did not serve a public key or decrypt the RSA response.This adds that protocol path while preserving TLS-only fallback when keys are disabled or invalid.
Operator impact
The feature introduces these MySQL runtime variables:
mysql-caching_sha2_password_auto_generate_rsa_keysmysql-caching_sha2_password_private_key_pathmysql-caching_sha2_password_public_key_pathRelative paths are confined beneath ProxySQL's datadir. Invalid configuration is rejected atomically without replacing a working snapshot. An explicit disabled/empty configuration remains available for TLS-only operation.
Security properties
Validation
git diff --checkCloses #5988
Summary by CodeRabbit
New Features
caching_sha2_passwordauthentication.Security
Documentation
Tests