fix(auth): allow mysql-monitor_* to authenticate under caching_sha2_password - #5991
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 (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe change corrects a caching-SHA2 RSA authentication log call and adds Bash, TAP, and documentation coverage for Admin monitor and full authentication scenarios. ChangesAuthentication regressions
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 5
🤖 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 1710-1712: In the RSA public-key diagnostic’s proxy_debug call,
swap the first two pointer arguments so the Session label receives (*myds)->sess
and the DS label receives (*myds). Keep the message and remaining arguments
unchanged.
In `@test/repro/README.md`:
- Around line 39-42: Update the fenced block in README.md that documents
COLD_START values to specify the text language, preserving its existing content
and formatting.
In `@test/repro/reg_test_5363_admin_monitor_caching_sha2.bash`:
- Around line 146-150: The regression tests must preserve and restore the exact
authentication state. In
test/repro/reg_test_5363_admin_monitor_caching_sha2.bash lines 146-150, update
restore() and its setup to save and restore the original plugin,
mysql-monitor_username, and mysql-monitor_password values, and remove the
admin-admin_credentials reset. In
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp lines 158-175,
save mysql-monitor_username alongside the existing saved values; in lines
198-220, restore that username and preserve an originally empty password instead
of substituting "monitor".
In `@test/repro/reg_test_5985_admin_caching_sha2_full_auth.bash`:
- Around line 138-144: Update the test setup around restore() to capture the
existing values of admin-admin_credentials and
mysql-default_authentication_plugin before modifying them, including when
COLD_START=0. Make restore() use those saved values in its UPDATE statements
before reloading both variable groups, rather than hard-coded defaults.
In `@test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp`:
- Around line 110-116: Update set_var to escape value before building the UPDATE
query’s quoted SQL literal, using the existing MySQL escaping utility or
established test helper. Preserve the current variable-name and load_stmt
behavior while ensuring quote and escape characters in value produce a valid SQL
query.
🪄 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: 6fe061c8-fcdd-448d-8c85-40e7d2f583e5
📒 Files selected for processing (6)
lib/MySQL_Protocol.cpptest/repro/README.mdtest/repro/reg_test_5363_admin_monitor_caching_sha2.bashtest/repro/reg_test_5985_admin_caching_sha2_full_auth.bashtest/tap/groups/groups.jsontest/tap/tests/reg_test_5363_admin_monitor_caching_sha2-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/reg_test_5363_admin_monitor_caching_sha2-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/reg_test_5363_admin_monitor_caching_sha2-t.cpplib/MySQL_Protocol.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:
test/repro/README.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:
test/repro/README.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_5363_admin_monitor_caching_sha2-t.cpp
🪛 Cppcheck (2.21.0)
lib/MySQL_Protocol.cpp
[warning] 2328-2328: Uninitialized variable
(uninitvar)
🪛 markdownlint-cli2 (0.23.2)
test/repro/README.md
[warning] 39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
test/repro/reg_test_5985_admin_caching_sha2_full_auth.bash
[warning] 92-92: This assignment is only seen by the forked process.
(SC2097)
[warning] 93-93: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 98-98: This assignment is only seen by the forked process.
(SC2097)
[warning] 99-99: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 103-103: This assignment is only seen by the forked process.
(SC2097)
[warning] 104-104: This expansion will not see the mentioned assignment.
(SC2098)
[info] 138-143: This function is never invoked. Check usage (or ignored if invoked indirectly).
(SC2329)
[info] 148-148: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 151-151: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 164-164: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 193-193: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 197-197: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 203-203: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 208-208: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 213-213: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 217-217: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 221-221: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
test/repro/reg_test_5363_admin_monitor_caching_sha2.bash
[warning] 109-109: This assignment is only seen by the forked process.
(SC2097)
[warning] 110-110: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 113-113: This assignment is only seen by the forked process.
(SC2097)
[warning] 114-114: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 118-118: This assignment is only seen by the forked process.
(SC2097)
[warning] 119-119: This expansion will not see the mentioned assignment.
(SC2098)
[info] 146-150: This function is never invoked. Check usage (or ignored if invoked indirectly).
(SC2329)
[info] 156-156: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 159-159: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 166-166: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 173-173: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 176-176: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 183-183: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 187-187: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 190-190: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 198-198: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 201-201: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🔇 Additional comments (3)
test/tap/groups/groups.json (1)
276-276: LGTM!lib/MySQL_Protocol.cpp (1)
2075-2180: LGTM!Also applies to: 2280-2341
test/repro/reg_test_5985_admin_caching_sha2_full_auth.bash (1)
1-136: LGTM!Also applies to: 146-242
| static int set_var(MYSQL* admin, const char* name, const char* value, const char* load_stmt) { | ||
| const string q { | ||
| string("UPDATE global_variables SET variable_value='") + value + | ||
| "' WHERE variable_name='" + name + "'" | ||
| }; | ||
| MYSQL_QUERY_T(admin, q.c_str()); | ||
| MYSQL_QUERY_T(admin, load_stmt); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Escape value before constructing the SQL literal.
Line 112 inserts value directly into a quoted SQL literal. An existing mysql-monitor_password can contain a quote or escape character. The restore query can then fail and leave shared authentication state modified.
Use a correctly escaped SQL literal for value.
🤖 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/reg_test_5363_admin_monitor_caching_sha2-t.cpp` around lines
110 - 116, Update set_var to escape value before building the UPDATE query’s
quoted SQL literal, using the existing MySQL escaping utility or established
test helper. Preserve the current variable-name and load_stmt behavior while
ensuring quote and escape characters in value produce a valid SQL query.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5991 +/- ##
==========================================
- Coverage 52.41% 52.38% -0.04%
==========================================
Files 472 472
Lines 143270 143270
Branches 36196 36196
==========================================
- Hits 75102 75052 -50
- Misses 51322 51331 +9
- Partials 16846 16887 +41
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:
|
…pros Addresses the CodeRabbit review on #5991. The repro scripts and the TAP test run against a SHARED instance, and test/repro/README.md already requires that they "restore any global state that was changed". They did not: - reg_test_5363_*.bash reset 'admin-admin_credentials' to a hardcoded BASE_CREDS even though it never modifies that variable, reset the authentication plugin and monitor password to hardcoded defaults, and never restored 'mysql-monitor_username' at all -- which it does mutate. - reg_test_5985_*.bash reset 'admin-admin_credentials' and the authentication plugin to hardcoded values, discarding whatever the instance had. - reg_test_5363_*-t.cpp never saved 'mysql-monitor_username', and substituted "monitor" / "mysql_native_password" whenever the saved value was empty, overwriting a legitimately empty value with a guess. All three now capture the actual values up front and restore exactly those. The TAP test distinguishes "read an empty value" from "could not read" via a new read_var() helper, and leaves an unreadable variable alone rather than guessing. The 5985 retry loop now rebuilds on the instance's real credential list instead of a hardcoded one. Verified by A/B against a seeded non-default state (an extra 'canary:canarypw' admin credential plus non-default monitor username/password): - committed version: canary credential destroyed, both monitor variables reset - this version: all three preserved byte-for-byte reg_test_5363_admin_monitor_caching_sha2-t passes 13/13, including the new "mysql-monitor_username restored" assertion (plan updated 12 -> 13). Also from the same review: - swap the Session/DS arguments in the RSA public-key proxy_debug - give the COLD_START fenced block in test/repro/README.md a language (MD040) Sonar (65 new issues on this PR, 64 of them in these two new scripts): use [[ over [, assign positional parameters to locals, add explicit returns, and hoist the repeated LOAD MYSQL VARIABLES literal into a constant. shellcheck -S warning is now clean on both scripts (the env-prefix SC2097/SC2098 pairs are gone: the three variables are exported once instead).
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp (1)
127-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse RAII for
MYSQL_RES.Automate
mysql_free_result()with astd::unique_ptrdeleter so this query result cannot be skipped by future control-flow changes.Proposed change
+#include <memory> + - MYSQL_RES* r = mysql_store_result(admin); - if (r == NULL) { return false; } - MYSQL_ROW row = mysql_fetch_row(r); + std::unique_ptr<MYSQL_RES, decltype(&mysql_free_result)> r( + mysql_store_result(admin), mysql_free_result); + if (!r) { return false; } + MYSQL_ROW row = mysql_fetch_row(r.get()); const bool found = (row != NULL && row[0] != NULL); if (found) { out = row[0]; } - mysql_free_result(r); return found;🤖 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/reg_test_5363_admin_monitor_caching_sha2-t.cpp` around lines 127 - 138, Update read_var to wrap the MYSQL_RES returned by mysql_store_result in a std::unique_ptr with a mysql_free_result deleter, replacing the manual cleanup while preserving the existing found check and output assignment.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.
Inline comments:
In `@test/repro/reg_test_5363_admin_monitor_caching_sha2.bash`:
- Line 69: Preserve the caller-provided COLD_START value instead of
unconditionally assigning “-e” near the script’s initialization. Update the
cold-start branching logic around COLD_START and its usage message so the branch
remains reachable when the caller requests a cold start and the displayed
invocation matches the actual accepted value.
- Around line 91-94: Update the setvar() helper to SQL-escape or otherwise
safely serialize both value and name before interpolating them into the UPDATE
statement, preserving exact restoration of values containing quotes or
backslashes; ensure the same encoding is used for all calls that restore
ORIG_MON_USER and ORIG_MON_PASS.
In `@test/repro/reg_test_5985_admin_caching_sha2_full_auth.bash`:
- Line 57: Preserve any caller-provided COLD_START value instead of
unconditionally assigning "-e" in the test setup. Update the related logic
around the condition at line 100 so COLD_START=1 still triggers instance
recreation and the cold-cache test runs, while retaining the existing default
behavior when no value is supplied.
- Around line 151-158: Make the configuration snapshot and restore flow
lossless: require both adm reads for ORIG_ADMIN_CREDS and ORIG_AUTH_PLUGIN to
succeed before modifying any state, and capture each value as HEX. Update
restore() and the later admin-admin_credentials reconstruction to restore and
reuse values through UNHEX, escaping no raw quoted data, and calculate
EXPECT_LEN from the original byte length rather than the encoded representation.
In `@test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp`:
- Around line 190-193: The diagnostic at
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp:190-193 must not
output the monitor password; replace its value with a fixed redacted status
while retaining the other diagnostic details. At
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp:263-267, preserve
the password equality assertion but remove both expected and actual password
values from the TAP message.
---
Nitpick comments:
In `@test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp`:
- Around line 127-138: Update read_var to wrap the MYSQL_RES returned by
mysql_store_result in a std::unique_ptr with a mysql_free_result deleter,
replacing the manual cleanup while preserving the existing found check and
output assignment.
🪄 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: e6ace944-95c1-4cbc-92c6-f058b927e430
📒 Files selected for processing (5)
lib/MySQL_Protocol.cpptest/repro/README.mdtest/repro/reg_test_5363_admin_monitor_caching_sha2.bashtest/repro/reg_test_5985_admin_caching_sha2_full_auth.bashtest/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/MySQL_Protocol.cpp
- test/repro/README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: build
🧰 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/reg_test_5363_admin_monitor_caching_sha2-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/reg_test_5363_admin_monitor_caching_sha2-t.cpp
🧠 Learnings (1)
📚 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_5363_admin_monitor_caching_sha2-t.cpp
🪛 GitHub Check: SonarCloud Code Analysis
test/repro/reg_test_5985_admin_caching_sha2_full_auth.bash
[warning] 70-70: Add an explicit return statement at the end of the function.
test/repro/reg_test_5363_admin_monitor_caching_sha2.bash
[warning] 83-83: Add an explicit return statement at the end of the function.
🪛 Shellcheck (0.11.0)
test/repro/reg_test_5985_admin_caching_sha2_full_auth.bash
[info] 154-160: This function is never invoked. Check usage (or ignored if invoked indirectly).
(SC2329)
[info] 165-165: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 210-210: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 238-238: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
test/repro/reg_test_5363_admin_monitor_caching_sha2.bash
[info] 161-166: This function is never invoked. Check usage (or ignored if invoked indirectly).
(SC2329)
[info] 172-172: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 175-175: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 182-182: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🔇 Additional comments (6)
test/repro/reg_test_5985_admin_caching_sha2_full_auth.bash (4)
58-86: LGTM!
88-99: LGTM!Also applies to: 101-143
163-169: LGTM!Also applies to: 180-180
224-224: LGTM!Also applies to: 237-259
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp (1)
169-171: LGTM!test/repro/reg_test_5363_admin_monitor_caching_sha2.bash (1)
70-90: LGTM!Also applies to: 96-107, 129-130, 143-143, 152-160, 169-183, 197-198, 223-228
| WORKSPACE="${WORKSPACE:-$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)}" | ||
| INFRA_ID="${INFRA_ID:-dev-$USER}" | ||
| TAP_GROUP="${TAP_GROUP:-no-infra-g1}" | ||
| COLD_START="-e" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the caller-provided COLD_START value.
Line 69 overwrites COLD_START=1 with -e. The cold-start branch at Line 116 can never run. The usage message at Line 124 is therefore incorrect.
Proposed fix
-COLD_START="-e"
+COLD_START="${COLD_START:-0}"Also applies to: 116-126
🤖 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/repro/reg_test_5363_admin_monitor_caching_sha2.bash` at line 69,
Preserve the caller-provided COLD_START value instead of unconditionally
assigning “-e” near the script’s initialization. Update the cold-start branching
logic around COLD_START and its usage message so the branch remains reachable
when the caller requests a cold start and the displayed invocation matches the
actual accepted value.
| setvar() { # setvar <variable_name> <value> <LOAD statement> | ||
| local name="$1" value="$2" load_stmt="$3" | ||
| adm "UPDATE global_variables SET variable_value='$value' WHERE variable_name='$name'; $load_stmt" >/dev/null | ||
| return $? |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize saved variable values safely before restoring them.
Line 93 embeds ORIG_MON_USER and ORIG_MON_PASS directly in an SQL literal. A pre-existing value containing a quote or backslash can produce invalid SQL or restore a different value. This defeats the exact-state restoration added in Lines 157-164.
Encode SQL literal values before constructing the UPDATE statement. Apply the same encoding to name if this helper remains generic.
Also applies to: 161-167
🤖 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/repro/reg_test_5363_admin_monitor_caching_sha2.bash` around lines 91 -
94, Update the setvar() helper to SQL-escape or otherwise safely serialize both
value and name before interpolating them into the UPDATE statement, preserving
exact restoration of values containing quotes or backslashes; ensure the same
encoding is used for all calls that restore ORIG_MON_USER and ORIG_MON_PASS.
| WORKSPACE="${WORKSPACE:-$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)}" | ||
| INFRA_ID="${INFRA_ID:-dev-$USER}" | ||
| TAP_GROUP="${TAP_GROUP:-no-infra-g1}" | ||
| COLD_START="-e" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the cold-start setting.
Line 57 overwrites a caller-provided COLD_START=1 with -e. The condition at Line 100 can then never recreate the instance. A warm cache fails preflight instead of running the required cold-cache test.
Proposed fix
-COLD_START="-e"
+COLD_START="${COLD_START:-0}"Also applies to: 100-110
🤖 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/repro/reg_test_5985_admin_caching_sha2_full_auth.bash` at line 57,
Preserve any caller-provided COLD_START value instead of unconditionally
assigning "-e" in the test setup. Update the related logic around the condition
at line 100 so COLD_START=1 still triggers instance recreation and the
cold-cache test runs, while retaining the existing default behavior when no
value is supplied.
| ORIG_ADMIN_CREDS="$(adm 'SELECT @@admin-admin_credentials;')" | ||
| ORIG_AUTH_PLUGIN="$(adm 'SELECT @@mysql-default_authentication_plugin;')" | ||
|
|
||
| restore() { | ||
| adm "UPDATE global_variables SET variable_value='${ORIG_ADMIN_CREDS}' WHERE variable_name='admin-admin_credentials'; | ||
| LOAD ADMIN VARIABLES TO RUNTIME; | ||
| UPDATE global_variables SET variable_value='${ORIG_AUTH_PLUGIN}' WHERE variable_name='mysql-default_authentication_plugin'; | ||
| ${LOAD_MYSQL_VARS}" >/dev/null |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make configuration capture and restoration lossless.
If either adm query fails, command substitution stores an empty value and the script continues. If an existing value contains a single quote, direct SQL interpolation produces invalid SQL. Lines 202-205 or the EXIT handler can then replace shared Admin configuration with empty or partial data.
Require successful reads before changing state. Capture values as HEX(...), restore them with UNHEX(...), and use the same representation when rebuilding admin-admin_credentials. Calculate EXPECT_LEN from the original byte length.
Also applies to: 198-205
🤖 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/repro/reg_test_5985_admin_caching_sha2_full_auth.bash` around lines 151
- 158, Make the configuration snapshot and restore flow lossless: require both
adm reads for ORIG_ADMIN_CREDS and ORIG_AUTH_PLUGIN to succeed before modifying
any state, and capture each value as HEX. Update restore() and the later
admin-admin_credentials reconstruction to restore and reuse values through
UNHEX, escaping no raw quoted data, and calculate EXPECT_LEN from the original
byte length rather than the encoded representation.
| diag("Saved mysql-default_authentication_plugin='%s'%s, mysql-monitor_username='%s'%s, mysql-monitor_password='%s'%s", | ||
| orig_plugin.c_str(), have_plugin ? "" : " (UNREADABLE - will not be restored)", | ||
| orig_mon_user.c_str(), have_mon_user ? "" : " (UNREADABLE - will not be restored)", | ||
| orig_mon_pass.c_str(), have_mon_pass ? "" : " (UNREADABLE - will not be restored)"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not write mysql-monitor_password to TAP output.
Line 190 logs the existing monitor password. Line 266 logs it again in the restore assertion. Shared-instance credentials can then persist in CI logs and test artifacts.
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp#L190-L193: Replace the password value with a fixed redacted status.test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp#L263-L267: Keep the equality check, but remove expected and actual password values from the TAP message.
📍 Affects 1 file
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp#L190-L193(this comment)test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp#L263-L267
🤖 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/reg_test_5363_admin_monitor_caching_sha2-t.cpp` around lines
190 - 193, The diagnostic at
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp:190-193 must not
output the monitor password; replace its value with a fixed redacted status
while retaining the other diagnostic details. At
test/tap/tests/reg_test_5363_admin_monitor_caching_sha2-t.cpp:263-267, preserve
the password equality assertion but remove both expected and actual password
values from the TAP message.
Brings the branch up to date with v3.0 (14 commits, including the #5993 credential-scope work that already carried this branch's ca03c8d), and re-triggers a full CI run after the CI-mysql84-g9 job on 9421d44 wedged for 6 hours in test_ssl_fast_forward-3_libmariadb-t. That stall was not reproducible: the same commit ran mysql84-g9 locally in 26 minutes and the test passed 5/5 in isolation, four other g9 runs completed successfully inside the same window, and no GitHub Actions incident overlapped it. Cause still unknown; investigation paused. Merge verified: lib/MySQL_Protocol.cpp auto-merged keeping both v3.0's cred_scope_for_session() call sites and this branch's Session/DS argument fix; the repro scripts and TAP test keep their restore-exact-state versions. Builds clean under PROXYSQL31.
|
Brings the branch up to date with v3.0 (37 commits, including #5991 and the #5999 salt-length/NUL fixes) and re-triggers a full CI run from a clean state. Merge verified: lib/MySQL_Protocol.cpp auto-merged keeping all six auth_response_has() bounds checks from this branch, with the reverted set_SHA1 const_cast staying reverted; the CACHING_SHA2_PASSWORD() salt-length and NUL rejections and the deterministic salt sweep come across from v3.0 intact. sqlite3 dep rebuilt against the merged patch; builds clean under PROXYSQL31.
TEST_TAP_TIMEOUT could not catch the failure it exists for. The read loop
was:
line = fop.stdout.readline() # blocks
...
if tap_timeout > 0 and (time.time() - start_time) > tap_timeout:
readline() blocks until a full line arrives, so a test that hangs while
producing no output never reached the deadline check at all. Verified
directly: with tap_timeout=3 against 'sleep 300', the old loop was still
blocked after 25 seconds; the new one raises at 3.0s.
That is the exact profile of the CI-mysql84-g9 stall on sysown#5991, where
test_ssl_fast_forward-3_libmariadb-t ran for hours.
Replaces the blocking readline() with select() on a bounded wait plus raw
os.read() chunking. select() guarantees the deadline check runs even when
the child is silent; chunking rather than line-reading means a test that
stops mid-line cannot wedge the loop either. Output order and content are
unchanged, a trailing partial line is now flushed instead of dropped, and
decoding uses errors='replace' so a stray non-UTF-8 byte no longer throws.
Verified against four cases: normal chatty test (all lines, in order),
silent hang (timeout fires), partial-line-then-hang (timeout fires), and
clean exit.
Also flips the default from 0 (disabled) to 1800s. 1800 is ~2.4x the
slowest single test measured across 47 groups:
reg_test_3765_ssl_pollout-t 12.5 min
test_cluster_sync-t 10.5 min
set_testing-240-t 7.8 min
test_auth_methods-t 7.7 min
Only 4 of 401 tests exceed 5 minutes, so this cannot fire on a merely slow
test, while still stopping a hang well inside the 90-minute step budget
added in the companion CI PRs -- and, unlike a step or job timeout, it
identifies WHICH test hung and lets the run continue to its archive steps.
1 participant
Fixes #5363. Implements #5986.
Problem
mysql-monitor_username/mysql-monitor_passwordcannot authenticate on the Admin interface (:6032) whenmysql-default_authentication_plugin = 'caching_sha2_password'. It works undermysql_native_password. TLS does not help. This is what breaks k8s liveness/readiness probes and metrics exporters.That credential is not in
GloMyAuth, soPPHR_verify_password()reaches it via itsvars1.password == NULLbranch andPPHR_5passwordFalse_0()(MySQL_Protocol.cpp) is the only code that authenticates it. It was hardcoded to the native SHA1 scramble with a 20-byte compare; under caching_sha2 the client sends a 32-byte SHA256-derived response, so it could never match. It returnedfalsewithout ever sendingperform full authentication, which is why the transport was irrelevant.The pre-existing
// FIXME: does this work only for mysql_native_password ?on the line above was this bug.Fix
Dispatch on
auth_plugin_id. The caching_sha2 fast-auth verification already existed inPPHR_6auth2(); it's extracted intocaching_sha2_fast_auth_verify()so there is exactly one implementation, withPPHR_6auth2()now calling it. Successful caching_sha2 fast auth also emits thefast_auth_success(0x03) marker before the OK packet, mirroring thePPHR_6auth2()call site.Also: a 1-byte
0x02atswitching_auth_stage == 5is the caching_sha2request_public_keypacket, not a disconnect. It was logged as "client is disconnecting during switch auth", which sends operators to the wrong place. The log line now names the real cause. Client-visible messaging needs the session-level error path and is left to #5988.Deliberately not length-gated — please keep it that way
Both comparisons carry a comment explaining this, because it is genuinely counter-intuitive:
vars1.pass_lenis not the amount of valid data invars1.pass.PPHR_2(MySQL_Protocol.cpp:1884) strips a trailing NUL — "remove the extra 0 if present" — so a legitimate 32-byte caching_sha2 response whose last byte is0x00(p = 1/256) arrives withpass_len == 31while all 32 bytes are present in the buffer.An earlier revision of this branch gated on
pass_lenand caused intermittentAccess deniedon the frontend. Measured with an instrumented build: 20 spurious denials across ~6520 connections (0.31%, predicted 0.39%), every one reportingclient_response_len=31. The same mistake was also present in the monitor's native branch, where it would have intermittently broken the very credential this PR fixes. Both are removed;PPHR_6auth2()is behaviourally identical tov3.0.There is a real out-of-bounds read here for a deliberately short response (allocation is
malloc(N_original+1), so a client declaringpass_len=1gets a 2-byte buffer against a 32-bytememcmp). That is pre-existing, not introduced or worsened by this PR, and is being filed separately — a length guard on this comparison needs its own verification run, which is exactly the lesson above.Verification
test/repro/reg_test_5363_admin_monitor_caching_sha2.bashreg_test_5363_admin_monitor_caching_sha2-ttest_auth_methods-t(40194 assertions)reg_test_4935-caching_sha2-tno-infra-g1groupThe revert check matters: a regression test that doesn't fail on the bug is worthless, so it was verified against a rebuilt pre-fix binary rather than assumed.
How the TAP test asserts this in CI
The monitor credential has a locality restriction, so an off-box connection is refused even with the right password — which is what makes it testable without running a client inside the container:
1040"can only connect locally" → authentication succeeded, rejected afterwards1045"Access denied" → authentication failedThe regression is precisely "correct password yields 1045 instead of 1040 under caching_sha2". The repro script does connect locally and asserts a real login; the two are complementary.
Also lands:
test/repro/Two developer-facing reproductions plus a README. Not CI artifacts — the TAP tests are. Teardown is opt-in (
COLD_START=1, destructive), both scripts refuse to run against a release build, and both abort if the running container predatessrc/proxysqlso a rebuilt fix can't silently go untested. Exit codes separate "bug reproduced" (1) from "environment wrong" (2).Note for the reviewer
reg_test_5363_admin_monitor_caching_sha2-tis registered inno-infra-g1, alongsidetest_passthrough_auth_admin-t, since it needs ProxySQL but no backend. I could not find any workflow referencing that group —grep -rn "no-infra" .github/workflows/returns nothing on this branch. If there's noci-no-infra-g1caller onGH-Actions, this test (and the four already in the group) never run in CI, and it should move or the group should be wired up.Summary by CodeRabbit
Bug Fixes
caching_sha2_password, including monitor and full-authentication scenarios over secure connections.Tests
Documentation