feat: add per-user frontend X.509 authentication - #6028
Conversation
RED runtime evidence is blocked in this worktree: the direct binary skips because REGULAR_INFRA_DATADIR is unset, and the documented isolated runner stops while configuring the current release ProxySQL binary (Unknown global variable: admin-debug). No failing TAP assertions were observed. The intended feature-absent assertions are 10-12 (plaintext, TLS without certificate, and untrusted certificate) plus 15 for the string policy type.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)include/**/*.h📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughProxySQL31 adds per-user frontend X.509 authentication through ChangesFrontend X.509 authentication
Estimated code review effort: 4 (Complex) | ~60 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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
lib/MySQL_Protocol.cpp (1)
68-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the struct type to PascalCase.
frontend_certificate_policy_resultis a struct type. Rename it toFrontendCertificatePolicyResultand update its uses.As per coding guidelines, class names must use
PascalCase.🤖 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_Protocol.cpp` around lines 68 - 71, Rename the struct type frontend_certificate_policy_result to FrontendCertificatePolicyResult and update every reference to use the new PascalCase name, preserving its fields and behavior.Source: Coding guidelines
test/tap/tests/frontend_x509_test_utils.h (2)
152-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse RAII for the extension file.
Replace the manually managed
FILE*withstd::ofstreamor a scoped file deleter. Keep an explicit write and close error check.As per coding guidelines, use RAII for resource management.
🤖 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/frontend_x509_test_utils.h` around lines 152 - 161, Update the extension-file handling in the visible helper to use RAII, preferably std::ofstream, instead of manually managing the FILE* returned by fopen. Preserve explicit checks for both writing and closing the file, and retain the existing diagnostic and false-return behavior on failure.Source: Coding guidelines
27-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
PascalCasefor class and struct names.
test/tap/tests/frontend_x509_test_utils.h#L27-L39: Renameclient_tls_materialandmysql_closertoPascalCasenames.test/tap/tests/frontend_x509_test_utils.h#L69-L97: Renametemporary_certificate_directoryto aPascalCasename.test/tap/tests/test_frontend_x509_passthrough-t.cpp#L80-L84: Renameserver_ssl_stateto aPascalCasename.As per coding guidelines, class names must use
PascalCase.🤖 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/frontend_x509_test_utils.h` around lines 27 - 39, Rename the class and struct symbols to PascalCase throughout all references: in test/tap/tests/frontend_x509_test_utils.h lines 27-39, rename client_tls_material and mysql_closer; in lines 69-97, rename temporary_certificate_directory; and in test/tap/tests/test_frontend_x509_passthrough-t.cpp lines 80-84, rename server_ssl_state. Update every declaration, alias, construction, and usage consistently.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 `@docs/superpowers/plans/2026-08-10-frontend-x509-authentication.md`:
- Around line 297-303: Update both C++ examples around the shown ssl
initialization block and the additional example near the referenced section to
use properly labeled opening and closing fenced code markers, such as a C++
language fence. Ensure each `#ifdef` block remains fully enclosed so markdownlint
does not interpret preprocessor directives as headings or report unlabeled
fences.
In `@lib/mysql_data_stream.cpp`:
- Around line 241-250: Update the GEN_URI handling in the SAN extraction loop to
use ASN1_STRING_length() and a length-bounded prefix comparison instead of
strstr(), then copy only the URI’s valid ASN.1 bytes rather than using strdup().
Preserve the first accepted SPIFFE URI by skipping further SAN entries once
x509_subject_alt_name is set, preventing later matches and replacement leaks.
In `@test/tap/tests/test_frontend_x509_passthrough-t.cpp`:
- Around line 136-137: Update the TAP plan in the test setup from 40 to 38, and
adjust the skip assertions in the trusted-certificate branch to emit 8 skips
instead of 11. Keep the successful path’s 8 assertions and all other setup,
behavior, and cleanup checks unchanged.
---
Nitpick comments:
In `@lib/MySQL_Protocol.cpp`:
- Around line 68-71: Rename the struct type frontend_certificate_policy_result
to FrontendCertificatePolicyResult and update every reference to use the new
PascalCase name, preserving its fields and behavior.
In `@test/tap/tests/frontend_x509_test_utils.h`:
- Around line 152-161: Update the extension-file handling in the visible helper
to use RAII, preferably std::ofstream, instead of manually managing the FILE*
returned by fopen. Preserve explicit checks for both writing and closing the
file, and retain the existing diagnostic and false-return behavior on failure.
- Around line 27-39: Rename the class and struct symbols to PascalCase
throughout all references: in test/tap/tests/frontend_x509_test_utils.h lines
27-39, rename client_tls_material and mysql_closer; in lines 69-97, rename
temporary_certificate_directory; and in
test/tap/tests/test_frontend_x509_passthrough-t.cpp lines 80-84, rename
server_ssl_state. Update every declaration, alias, construction, and usage
consistently.
🪄 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: 08297d6e-3dee-4104-a5eb-722353a38f15
📒 Files selected for processing (14)
doc/frontend_x509_authentication.mddoc/internal/passthrough_authentication.mddocs/superpowers/plans/2026-08-10-frontend-x509-authentication.mddocs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.mdinclude/MySQL_Data_Stream.hinclude/MySQL_Protocol.hlib/MySQL_Authentication.cpplib/MySQL_Protocol.cpplib/mysql_data_stream.cpptest/tap/groups/groups.jsontest/tap/tests/frontend_x509_test_utils.htest/tap/tests/test_frontend_x509_auth-t.cpptest/tap/tests/test_frontend_x509_passthrough-t.cpptest/tap/tests/test_frontend_x509_tier_gate-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Gitar
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
lib/mysql_data_stream.cppinclude/MySQL_Protocol.hlib/MySQL_Authentication.cppinclude/MySQL_Data_Stream.htest/tap/tests/test_frontend_x509_tier_gate-t.cpptest/tap/tests/test_frontend_x509_auth-t.cpptest/tap/tests/frontend_x509_test_utils.hlib/MySQL_Protocol.cpptest/tap/tests/test_frontend_x509_passthrough-t.cpp
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/MySQL_Protocol.hinclude/MySQL_Data_Stream.h
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_frontend_x509_tier_gate-t.cpptest/tap/tests/test_frontend_x509_auth-t.cpptest/tap/tests/test_frontend_x509_passthrough-t.cpp
🧠 Learnings (4)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_frontend_x509_tier_gate-t.cpptest/tap/tests/test_frontend_x509_auth-t.cpptest/tap/tests/test_frontend_x509_passthrough-t.cpp
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.mddoc/internal/passthrough_authentication.mddoc/frontend_x509_authentication.mddocs/superpowers/plans/2026-08-10-frontend-x509-authentication.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-frontend-x509-proxysql31-gating-design.mddoc/internal/passthrough_authentication.mddoc/frontend_x509_authentication.mddocs/superpowers/plans/2026-08-10-frontend-x509-authentication.md
📚 Learning: 2026-04-11T13:16:05.854Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:16:05.854Z
Learning: When validating GitHub-rendered Markdown in this repository (e.g., links that use heading anchors), account for GitHub slug behavior for headings containing an em-dash (—) surrounded by spaces: GitHub strips the em-dash and converts each surrounding space into a hyphen independently, which can produce a double hyphen (--) in the generated anchor. Therefore, do NOT flag as broken links any anchors whose expected slug contains a double hyphen specifically attributable to an em-dash surrounded by spaces in the source heading. (Example: `...vocabulary — read...` -> `...vocabulary--read...`.)
Applied to files:
doc/internal/passthrough_authentication.mddoc/frontend_x509_authentication.md
🪛 Cppcheck (2.21.0)
lib/MySQL_Protocol.cpp
[warning] 86-86: 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)
🪛 LanguageTool
docs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.md
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... the require_x509 policy evaluator. - Do not inspect the require_x509 key. - P...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~82-~82: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rement for unknown-user pass-through. - No PostgreSQL frontend authentication chan...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...reSQL frontend authentication change. - No TLS renegotiation or backend certificat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~84-~84: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...on or backend certificate forwarding. - No change to the per-user behavior of a st...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/superpowers/plans/2026-08-10-frontend-x509-authentication.md
[style] ~649-~649: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...pted`, and leaves the cache empty. 6. TLS with the trusted no-SAN certificate and...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.23.2)
docs/superpowers/plans/2026-08-10-frontend-x509-authentication.md
[warning] 297-297: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 301-301: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 303-303: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 382-382: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 383-383: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 392-392: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 394-394: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 395-395: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OpenGrep (1.26.0)
test/tap/tests/frontend_x509_test_utils.h
[ERROR] 57-57: Dynamic command passed to system(). Use execve() or posix_spawn() with an explicit argument array instead.
(coderabbit.command-injection.c-system)
🔇 Additional comments (11)
doc/frontend_x509_authentication.md (1)
1-37: LGTM!include/MySQL_Data_Stream.h (1)
134-138: LGTM!lib/MySQL_Authentication.cpp (1)
17-33: LGTM!Also applies to: 212-215, 274-277
lib/mysql_data_stream.cpp (1)
353-357: LGTM!include/MySQL_Protocol.h (1)
257-259: LGTM!lib/MySQL_Protocol.cpp (1)
73-167: LGTM!Also applies to: 203-219, 1599-1703, 1819-1867, 2910-2938, 3582-3676
doc/internal/passthrough_authentication.md (1)
80-83: LGTM!Also applies to: 182-183, 257-272
docs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.md (1)
1-84: LGTM!test/tap/groups/groups.json (1)
332-334: LGTM!test/tap/tests/test_frontend_x509_auth-t.cpp (1)
1-299: LGTM!test/tap/tests/test_frontend_x509_tier_gate-t.cpp (1)
1-111: LGTM!
| /* 10 setup + 28 behavior checks + 2 cleanup checks. */ | ||
| plan(40); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the TAP assertion count.
The normal path emits 38 assertions, not 40. Lines 281-283 emit 11 skips, but the successful trusted-certificate path emits 8 assertions. That branch emits 41 assertions.
Set the plan to 38. Replace the 11 skip assertions with 8 skip assertions.
Proposed fix
- /* 10 setup + 28 behavior checks + 2 cleanup checks. */
- plan(40);
+ /* 10 setup + 26 behavior checks + 2 cleanup checks. */
+ plan(38);
...
- for (int i = 0; i != 11; ++i) ok(true, "Trusted pass-through control # SKIP trusted certificate fixture unavailable");
+ for (int i = 0; i != 8; ++i) ok(true, "Trusted pass-through control # SKIP trusted certificate fixture unavailable");Also applies to: 281-283
🤖 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_frontend_x509_passthrough-t.cpp` around lines 136 - 137,
Update the TAP plan in the test setup from 40 to 38, and adjust the skip
assertions in the trusted-certificate branch to emit 8 skips instead of 11. Keep
the successful path’s 8 assertions and all other setup, behavior, and cleanup
checks unchanged.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/MySQL_Protocol.cpp (2)
88-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the new types to
PascalCasewith a protocol prefix.
frontend_auth_contextandfrontend_certificate_policy_resultare class-like types. The coding guidelines requirePascalCasewith a protocol prefix such asMySQL_for class names. Rename them, for example toMySQL_FrontendAuthContextandMySQL_FrontendCertificatePolicyResult.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 `@lib/MySQL_Protocol.cpp` around lines 88 - 99, Rename the class-like types frontend_auth_context and frontend_certificate_policy_result to PascalCase names with the MySQL_ protocol prefix, such as MySQL_FrontendAuthContext and MySQL_FrontendCertificatePolicyResult. Update all references to these types consistently while preserving their existing members and behavior.Source: Coding guidelines
1747-1761: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
has_spiffe_idchecks.
evaluate_frontend_certificate_policyalready setsallowed = falsewhen the context isCOM_CHANGE_USERand the target has aspiffe_id(lines 142-147). Thereforetarget_policy.has_spiffe_idon line 1747 never adds a rejection, and the gate on lines 1747-1752 already returns before line 1760 can observe a SPIFFE target. Line 1760 is unreachable-effect code. Keep one check to state the intent, and drop the other to avoid future confusion about which layer enforces the rule.🤖 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_Protocol.cpp` around lines 1747 - 1761, Remove the redundant target_policy.has_spiffe_id checks in the COM_CHANGE_USER handling around evaluate_frontend_certificate_policy: retain a single check that clearly expresses the SPIFFE-target rejection intent, and remove the duplicate condition from the passthrough-auth gate. Preserve the existing allowed-policy rejection and cleanup behavior.test/tap/tests/frontend_x509_test_utils.h (1)
27-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse PascalCase for the new C++ class types.
The new class-type names do not follow the repository naming rule.
test/tap/tests/frontend_x509_test_utils.h#L27-L39: Renameclient_tls_materialandmysql_closerto PascalCase names, and updatemysql_ptr.test/tap/tests/frontend_x509_test_utils.h#L69-L97: Renametemporary_certificate_directorytoTemporaryCertificateDirectoryand update its uses.As per coding guidelines, class names must use
PascalCase.🤖 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/frontend_x509_test_utils.h` around lines 27 - 39, Rename the class types client_tls_material and mysql_closer to PascalCase names in test/tap/tests/frontend_x509_test_utils.h:27-39, and update the mysql_ptr alias accordingly. Rename temporary_certificate_directory to TemporaryCertificateDirectory in test/tap/tests/frontend_x509_test_utils.h:69-97 and update every use of the type.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 `@lib/MySQL_Protocol.cpp`:
- Around line 1740-1752: Update the policy-rejection cleanup in the PROXYSQL31
branch of the frontend certificate policy flow to call
cleanse_and_free_password(password) instead of freeing password directly. Keep
the existing pass cleanup, account-details cleanup, and rejection return
unchanged.
In `@test/tap/tests/frontend_x509_test_utils.h`:
- Around line 6-7: Rename the header guard macro __FRONTEND_X509_TEST_UTILS_H to
the non-reserved FRONTEND_X509_TEST_UTILS_H consistently across all three guard
references.
---
Nitpick comments:
In `@lib/MySQL_Protocol.cpp`:
- Around line 88-99: Rename the class-like types frontend_auth_context and
frontend_certificate_policy_result to PascalCase names with the MySQL_ protocol
prefix, such as MySQL_FrontendAuthContext and
MySQL_FrontendCertificatePolicyResult. Update all references to these types
consistently while preserving their existing members and behavior.
- Around line 1747-1761: Remove the redundant target_policy.has_spiffe_id checks
in the COM_CHANGE_USER handling around evaluate_frontend_certificate_policy:
retain a single check that clearly expresses the SPIFFE-target rejection intent,
and remove the duplicate condition from the passthrough-auth gate. Preserve the
existing allowed-policy rejection and cleanup behavior.
In `@test/tap/tests/frontend_x509_test_utils.h`:
- Around line 27-39: Rename the class types client_tls_material and mysql_closer
to PascalCase names in test/tap/tests/frontend_x509_test_utils.h:27-39, and
update the mysql_ptr alias accordingly. Rename temporary_certificate_directory
to TemporaryCertificateDirectory in
test/tap/tests/frontend_x509_test_utils.h:69-97 and update every use of the
type.
🪄 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: 49821287-f435-46df-97b5-7a710ec4fae3
📒 Files selected for processing (14)
doc/frontend_x509_authentication.mddoc/internal/passthrough_authentication.mddocs/superpowers/plans/2026-08-10-frontend-x509-authentication.mddocs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.mdinclude/MySQL_Data_Stream.hinclude/MySQL_Protocol.hlib/MySQL_Authentication.cpplib/MySQL_Protocol.cpplib/mysql_data_stream.cpptest/tap/groups/groups.jsontest/tap/tests/frontend_x509_test_utils.htest/tap/tests/test_frontend_x509_auth-t.cpptest/tap/tests/test_frontend_x509_passthrough-t.cpptest/tap/tests/test_frontend_x509_tier_gate-t.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- test/tap/groups/groups.json
- include/MySQL_Data_Stream.h
- doc/frontend_x509_authentication.md
- test/tap/tests/test_frontend_x509_auth-t.cpp
- lib/MySQL_Authentication.cpp
- doc/internal/passthrough_authentication.md
- test/tap/tests/test_frontend_x509_passthrough-t.cpp
- test/tap/tests/test_frontend_x509_tier_gate-t.cpp
- include/MySQL_Protocol.h
- lib/mysql_data_stream.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: Gitar
- GitHub Check: run / trigger
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
lib/MySQL_Protocol.cpptest/tap/tests/frontend_x509_test_utils.h
🧠 Learnings (2)
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/plans/2026-08-10-frontend-x509-authentication.mddocs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/plans/2026-08-10-frontend-x509-authentication.mddocs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.md
🪛 Cppcheck (2.21.0)
lib/MySQL_Protocol.cpp
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 LanguageTool
docs/superpowers/plans/2026-08-10-frontend-x509-authentication.md
[style] ~649-~649: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...pted`, and leaves the cache empty. 6. TLS with the trusted no-SAN certificate and...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.md
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... the require_x509 policy evaluator. - Do not inspect the require_x509 key. - P...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~82-~82: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rement for unknown-user pass-through. - No PostgreSQL frontend authentication chan...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...reSQL frontend authentication change. - No TLS renegotiation or backend certificat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~84-~84: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...on or backend certificate forwarding. - No change to the per-user behavior of a st...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.23.2)
docs/superpowers/plans/2026-08-10-frontend-x509-authentication.md
[warning] 297-297: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 301-301: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 303-303: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 382-382: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 383-383: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 392-392: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 394-394: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 395-395: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OpenGrep (1.26.0)
test/tap/tests/frontend_x509_test_utils.h
[ERROR] 57-57: Dynamic command passed to system(). Use execve() or posix_spawn() with an explicit argument array instead.
(coderabbit.command-injection.c-system)
🔇 Additional comments (10)
lib/MySQL_Protocol.cpp (8)
141-172: LGTM!
174-195: LGTM!
1657-1666: LGTM!
1874-1886: LGTM!Also applies to: 1918-1922
3751-3776: LGTM!
3824-3845: 🩺 Stability & AvailabilityNo header-gating change is needed. The declaration is already inside
#ifndef PROXYSQL31, and no call sites were found in C++ sources.> Likely an incorrect or invalid review comment.
3070-3098: 🔒 Security & Privacy
require_x509remains enforced when pass-through is disabled.process_pkt_handshake_response()callsverify_user_attributes()after successful password verification, and thePROXYSQL31evaluator rejects rows requiring X.509 without a valid client certificate.
126-137: 🩺 Stability & AvailabilityKeep
%ldforclient_cert_verify_result. The member islong, and the conditional expression converts the fallback value tolong.docs/superpowers/plans/2026-08-10-frontend-x509-authentication.md (1)
1-1115: LGTM!docs/superpowers/specs/2026-08-10-frontend-x509-proxysql31-gating-design.md (1)
1-85: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6028 +/- ##
==========================================
+ Coverage 53.40% 53.45% +0.04%
==========================================
Files 487 491 +4
Lines 145258 145947 +689
Branches 36704 36879 +175
==========================================
+ Hits 77579 78011 +432
- Misses 50732 50859 +127
- Partials 16947 17077 +130
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:
|
![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: 73e0ba28a2
ℹ️ 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".
| ok(trusted_warm_err == 0, "Warm trusted certificate succeeds (errno=%u)", trusted_warm_err); | ||
| ok(trusted_hits_before >= 0 && trusted_hits_after == trusted_hits_before + 1, "Warm trusted certificate increments cache_hits exactly once (%ld -> %ld)", trusted_hits_before, trusted_hits_after); | ||
| } else { | ||
| for (int i = 0; i != 11; ++i) ok(true, "Trusted pass-through control # SKIP trusted certificate fixture unavailable"); |
There was a problem hiding this comment.
When the frontend CA private key is unavailable or does not match the CA certificate, trusted_ready is false and this branch emits 11 TAP results, while the corresponding success branch contains only 10 assertions. The test consequently produces 41 results against plan(40) and fails in the custom-CA environment that this fallback is intended to support; change the loop bound to 10.
Useful? React with 👍 / 👎.
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/frontend_x509_test_utils.h (1)
69-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the class to PascalCase.
Rename
temporary_certificate_directorytoTemporaryCertificateDirectory. Update its uses in the TAP tests.As per coding guidelines, class names must use
PascalCase.🤖 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/frontend_x509_test_utils.h` around lines 69 - 99, Rename the class temporary_certificate_directory to TemporaryCertificateDirectory and update every constructor, destructor, and TAP test usage to match the new PascalCase name. Preserve the class behavior and member naming otherwise.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/frontend_x509_test_utils.h`:
- Around line 69-99: Rename the class temporary_certificate_directory to
TemporaryCertificateDirectory and update every constructor, destructor, and TAP
test usage to match the new PascalCase name. Preserve the class behavior and
member naming otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eb370455-4c24-434a-a8c2-a63d09380dcd
📒 Files selected for processing (9)
docs/superpowers/plans/2026-08-10-frontend-x509-authentication.mddocs/superpowers/plans/2026-08-10-frontend-x509-review-remediation.mdinclude/MySQL_Data_Stream.hlib/MySQL_Protocol.cpplib/mysql_data_stream.cpptest/tap/groups/groups.jsontest/tap/tests/frontend_x509_test_utils.htest/tap/tests/test_frontend_x509_auth-t.cpptest/tap/tests/test_frontend_x509_passthrough-t.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- test/tap/groups/groups.json
- test/tap/tests/test_frontend_x509_auth-t.cpp
- docs/superpowers/plans/2026-08-10-frontend-x509-authentication.md
- include/MySQL_Data_Stream.h
- test/tap/tests/test_frontend_x509_passthrough-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Gitar
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/frontend_x509_test_utils.hlib/mysql_data_stream.cpplib/MySQL_Protocol.cpp
🧠 Learnings (2)
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/plans/2026-08-10-frontend-x509-review-remediation.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/plans/2026-08-10-frontend-x509-review-remediation.md
🪛 Cppcheck (2.21.0)
lib/MySQL_Protocol.cpp
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (4)
docs/superpowers/plans/2026-08-10-frontend-x509-review-remediation.md (1)
1-137: LGTM!lib/mysql_data_stream.cpp (1)
221-244: LGTM!Also applies to: 252-276, 507-517, 550-550
lib/MySQL_Protocol.cpp (1)
62-69: LGTM!Also applies to: 109-202, 1647-1648, 1650-1675, 1688-1695, 1771-1801, 1850-1850, 1902-1914, 1946-1950
test/tap/tests/frontend_x509_test_utils.h (1)
6-63: LGTM!Also applies to: 120-128, 137-141, 166-175, 231-231
|
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
1 participant
Summary
mysql_users.attributes.require_x509frontend authentication policyCOM_CHANGE_USER, and row-backed pass-through authenticationRelated to #5928 and #4582.
Behavior
require_x509: trueis additive to password/auth-plugin authentication and requires frontend TLS, a presented peer certificate, andX509_V_OKCOM_CHANGE_USERdoes not renegotiate TLS; it evaluates the target user against the certificate evidence captured for the original physical connectionCOM_CHANGE_USERis rejected for a SPIFFE-authenticated source, a SPIFFE target, or a pass-through target; a pass-through source may change to an ordinary password-authenticated rowrequire_x509; the feature exists only underPROXYSQL31(and therefore PROXYSQL40)mysql-passthrough_auth_require_tlscheck remains miss-only: after the cache lookup and before requesting cleartext credentials/backend probingValidation
Stable DEBUG:
COM_CHANGE_USER: 96/96TAP_HOST: 968/968require_x509implementation is present in stable objectsPROXYSQL31 DEBUG:
COM_CHANGE_USER: 96/96Both tiers were clean-built independently. The build configuration also confirms PROXYSQL40 implies PROXYSQL31.
Test infrastructure note
The raw-socket helper in
reg_test_4556requires a numericTAP_HOST. In the stable isolated Docker runner, the standard hostname form causes its knowninet_ptonharness failure at assertion 967; rerunning the unchanged binary with the resolved numeric address passes 968/968. PROXYSQL31 passes in both the standard and numeric-address runners.Summary by CodeRabbit
COM_CHANGE_USER.