fix(mysql): require TLS for SPIFFE users (#5929) - #6059
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 (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/tap/tests/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/tap/tests/unit/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2026-08-11T12:56:13.170ZApplied to files:
📚 Learning: 2026-04-01T21:27:00.297ZApplied to files:
📚 Learning: 2026-07-08T13:19:04.649ZApplied to files:
📚 Learning: 2026-01-20T09:34:19.124ZApplied to files:
📚 Learning: 2026-01-20T07:40:34.938ZApplied to files:
📚 Learning: 2026-08-12T05:27:01.785ZApplied to files:
🪛 Cppcheck (2.21.0)lib/MySQL_Authentication.cpp[warning] 310-310: If memory allocation fails, then there is a possible null pointer dereference (nullPointerOutOfMemory) 🔇 Additional comments (4)
📝 WalkthroughWalkthrough
ChangesSPIFFE TLS authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change ensures SPIFFE-authenticated MySQL users use TLS while preserving existing validation behavior, with focused authentication coverage added; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Code Review ✅ ApprovedForces the runtime MySQL use_ssl flag when user attributes contain a spiffe_id, ensuring TLS is required for SPIFFE users. No issues found. OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/MySQL_Authentication.cpp">
<violation number="1" location="lib/MySQL_Authentication.cpp:312">
P3: This re-parses `ad->attributes` that was just parsed and validated a few lines above in the same `add()` call (in both the new-account and attribute-change branches). Every `add()` for a user with non-empty attributes now parses the same JSON twice. The spiffe check could reuse the already-validated JSON instead of parsing again; as-is it's redundant work done on every user load (e.g. `LOAD MYSQL USERS TO RUNTIME` iterating many users).</violation>
<violation number="2" location="lib/MySQL_Authentication.cpp:313">
P2: The TLS-forcing path runs for every credential type, not only frontend users. In ProxySQL_Admin.cpp the LOAD MYSQL USERS handler calls `add()` once per usertype in `usertypes`, and a single `mysql_users` row flagged with both `frontend_=1` and `backend_=1` passes the same `attributes` (including `spiffe_id`) for the USERNAME_BACKEND credential too. As a result the backend account also gets `use_ssl=true`, forcing TLS on the proxy→MySQL backend connection for that user. This contradicts the PR scope ("require TLS for MySQL frontend users") and can break backend connectivity for a SPIFFE user that is also a backend user. Guard the spiffe check with `usertype == USERNAME_FRONTEND`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| if (ad->attributes && strlen(ad->attributes)) { | ||
| try { | ||
| nlohmann::json valid=nlohmann::json::parse(ad->attributes); | ||
| if (valid.find("spiffe_id") != valid.end()) { |
There was a problem hiding this comment.
P2: The TLS-forcing path runs for every credential type, not only frontend users. In ProxySQL_Admin.cpp the LOAD MYSQL USERS handler calls add() once per usertype in usertypes, and a single mysql_users row flagged with both frontend_=1 and backend_=1 passes the same attributes (including spiffe_id) for the USERNAME_BACKEND credential too. As a result the backend account also gets use_ssl=true, forcing TLS on the proxy→MySQL backend connection for that user. This contradicts the PR scope ("require TLS for MySQL frontend users") and can break backend connectivity for a SPIFFE user that is also a backend user. Guard the spiffe check with usertype == USERNAME_FRONTEND.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/MySQL_Authentication.cpp, line 313:
<comment>The TLS-forcing path runs for every credential type, not only frontend users. In ProxySQL_Admin.cpp the LOAD MYSQL USERS handler calls `add()` once per usertype in `usertypes`, and a single `mysql_users` row flagged with both `frontend_=1` and `backend_=1` passes the same `attributes` (including `spiffe_id`) for the USERNAME_BACKEND credential too. As a result the backend account also gets `use_ssl=true`, forcing TLS on the proxy→MySQL backend connection for that user. This contradicts the PR scope ("require TLS for MySQL frontend users") and can break backend connectivity for a SPIFFE user that is also a backend user. Guard the spiffe check with `usertype == USERNAME_FRONTEND`.</comment>
<file context>
@@ -306,7 +307,19 @@ bool MySQL_Authentication::add(char * username, char * password, enum cred_usern
+ if (ad->attributes && strlen(ad->attributes)) {
+ try {
+ nlohmann::json valid=nlohmann::json::parse(ad->attributes);
+ if (valid.find("spiffe_id") != valid.end()) {
+ effective_use_ssl = true;
+ }
</file context>
| ad->use_ssl=use_ssl; | ||
| if (ad->attributes && strlen(ad->attributes)) { | ||
| try { | ||
| nlohmann::json valid=nlohmann::json::parse(ad->attributes); |
There was a problem hiding this comment.
P3: This re-parses ad->attributes that was just parsed and validated a few lines above in the same add() call (in both the new-account and attribute-change branches). Every add() for a user with non-empty attributes now parses the same JSON twice. The spiffe check could reuse the already-validated JSON instead of parsing again; as-is it's redundant work done on every user load (e.g. LOAD MYSQL USERS TO RUNTIME iterating many users).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/MySQL_Authentication.cpp, line 312:
<comment>This re-parses `ad->attributes` that was just parsed and validated a few lines above in the same `add()` call (in both the new-account and attribute-change branches). Every `add()` for a user with non-empty attributes now parses the same JSON twice. The spiffe check could reuse the already-validated JSON instead of parsing again; as-is it's redundant work done on every user load (e.g. `LOAD MYSQL USERS TO RUNTIME` iterating many users).</comment>
<file context>
@@ -306,7 +307,19 @@ bool MySQL_Authentication::add(char * username, char * password, enum cred_usern
- ad->use_ssl=use_ssl;
+ if (ad->attributes && strlen(ad->attributes)) {
+ try {
+ nlohmann::json valid=nlohmann::json::parse(ad->attributes);
+ if (valid.find("spiffe_id") != valid.end()) {
+ effective_use_ssl = true;
</file context>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6059 +/- ##
==========================================
- Coverage 53.58% 53.46% -0.13%
==========================================
Files 504 503 -1
Lines 148005 147279 -726
Branches 37488 37286 -202
==========================================
- Hits 79314 78737 -577
- Misses 51115 51116 +1
+ Partials 17576 17426 -150
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:
|
1 participant
Summary
use_sslflag when the final retained user attributes containspiffe_id.Root cause
SPIFFE identity verification requires the client certificate SAN, but the authentication runtime could retain
spiffe_idwhile acceptinguse_ssl=0.Validation
PROXYSQL31=1 make build_lib -j"$(nproc)"make -C test/tap/tests/unit auth_unit-ttest/tap/tests/unit/auth_unit-t(69 assertions)git diff --check origin/v3.0..HEADFixes #5929
Summary by cubic
Enforces TLS for MySQL frontend users with SPIFFE identities. Previously the runtime could retain a
spiffe_idwhile acceptinguse_ssl=0; nowuse_sslis forced to true when retained attributes includespiffe_id, preventing connections without certificate SAN verification.ad->use_sslvia aneffective_use_sslthat becomes true when valid JSON attributes containspiffe_id; otherwise preserves the supplied flag. Attribute validation is unchanged; invalid JSON or unknown keys leave TLS as provided. Retained validspiffe_idcontinues to require TLS across updates.Operational impact
spiffe_idor with invalid attributes.Written for commit deb0722. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes