Add MariaDB ed25519 authentication (frontend + backend) - #6033
Conversation
Approved brainstorming outcome for supporting MariaDB's client_ed25519 / auth_ed25519 scheme on both frontend and backend: - Frontend verification gated behind PROXYSQL31 via new PROXYSQLED25519 macro; crypto reuses the connector's public-domain ref10 sources. - Backend support via flipping client_ed25519 to STATIC in the existing connector plugin patch (applies to all tiers, deps are not tier-aware). - Credentials in mysql_users.password: cleartext (full function) or new $ED$<base64> public-key format (frontend-verification-only). - Auth-switch-only protocol flow with 32-byte scramble; COM_CHANGE_USER supported; passthrough auth documented as incompatible by construction. - Unit tests with MariaDB known-answer vectors plus end-to-end TAP on infra-mariadb10.
Six-task plan executing the approved spec (docs/superpowers/specs/ 2026-08-11-ed25519-authentication-design.md): connector STATIC patch + PROXYSQLED25519 tier cascade, ref10-backed MySQL_Ed25519 wrapper with known-answer unit tests (the 'secret' vector reproduces the MariaDB KB example), frontend Auth Switch protocol flow, COM_CHANGE_USER support and credential validation warnings, end-to-end TAP test on the mariadb10-galera infra, and user documentation. All known-answer vectors were generated during planning by compiling the ref10 sources vendored in deps/ against OpenSSL SHA512.
…D25519 tier flag The connector's client_ed25519 plugin (with the full ref10 Ed25519 implementation) is flipped from DYNAMIC to STATIC in the existing plugin_auth CMakeLists patch. This transparently enables ed25519 authentication for backend connections (server-driven auth switch, no ProxySQL code involved) and exports crypto_sign_keypair / crypto_sign_open from libmariadbclient.a for the upcoming frontend verification wrapper. PROXYSQLED25519 is a new feature macro implied by PROXYSQL31, following the PROXYSQLFFTO cascade pattern. deps are intentionally NOT tier-gated (single connector build serves all tiers, per spec)."
proxysql_ed25519_{derive_public_key,verify_signature,is_pubkey_format,
decode_pubkey} wrap the ref10 symbols statically linked into
libmariadbclient.a. Key derivation must use ref10 because MariaDB
hashes an arbitrary-length password (SHA512) where standard Ed25519
hashes a fixed 32-byte seed; verification is standard Ed25519.
The 'secret' derivation vector reproduces the documented MariaDB KB
example, independently confirming scheme compatibility.
Adds AUTH_MYSQL_ED25519 to the frontend plugin registry and implements the MariaDB flow: ed25519 is never advertised in the greeting (its challenge is 32 bytes, the greeting scramble is 20); instead PPHR_verify_password decides at stage 0 -- when the stored credential is \$ED\$ or the client requested client_ed25519 -- and sends an AuthSwitchRequest with a fresh RAND_bytes nonce (PPHR_ed25519_switch). The 64-byte signature returns through PPHR_1 (native-style raw payload, no NUL terminator) and PPHR_ed25519_verify checks it against the stored public key or one derived from the stored cleartext password. All failures collapse into the generic access-denied path. Known v1 limitation: a client that triggers the early native switch (PPHR_4auth0, e.g. caching_sha2 offer against a native greeting) cannot be re-switched to ed25519 for \$ED\$ users -- the protocol allows a single switch. Standard libmariadb clients are unaffected.
PPHR_ed25519_switch() overwrites scramble_buff[0..31] with 32 raw random bytes from RAND_bytes(). That destroys the NUL terminator proxy_create_random_string() had previously written at index 20 for the native 20-byte scramble, and scramble_buff[40] is never zero-initialized by the MySQL_Connection constructor -- so bytes 32-39 are uninitialized stack/heap garbage at this point. The DEBUG-only handshake dump at __exit_process_pkt_handshake_response calls hex(scramble_buff); hex() takes a std::string_view, so the char* argument is implicitly strlen()'d. With ~88% probability RAND_bytes() produces 32 non-zero bytes, so strlen() runs past index 31 into the uninitialized tail (and potentially past the array entirely), printing garbage and risking an ASAN heap-buffer-overflow report under debug builds -- which is what the TAP harness requires (test/infra mandates a DEBUG binary). Fix: write an explicit NUL at index ED25519_NONCE_LEN (32) right after the nonce, bounding the strlen() to the intended 32 bytes. Index 32 is in-bounds for scramble_buff[40], and nothing downstream reads the native-style scramble once a connection has switched to ed25519. Found in code review of commit 3d237de (frontend client_ed25519 authentication via Auth Switch).
…ation COM_CHANGE_USER targeting a stored-$ED$ user (or naming client_ed25519) now performs the fresh-nonce Auth Switch instead of failing on unverifiable inline auth data; the signature response rides the existing change_user_auth_switch rails (#3504) back through process_pkt_handshake_response. Unlike caching_sha2 (#4618), no sub-protocol is needed, so change-user works fully. MySQL_Authentication::add() warns once at load time when a $ED$ credential is malformed, and connect_start() warns when a backend connection is attempted with a public-key-only $ED$ credential, instead of leaving admins to puzzle over generic access-denied errors.
…cramble
PPHR_ed25519_switch and the COM_CHANGE_USER ed25519 branch both
RAND_bytes()'d the 32-byte Auth Switch nonce directly into
MySQL_Connection::scramble_buff, aliasing the same 40-byte buffer that
holds the native (mysql_native_password) scramble generated once at
connection setup. That native scramble is not a one-shot value: it is
read again by every later inline-credential COM_CHANGE_USER
(verify_user_pass's proxy_scramble/proxy_scramble_sha1 calls) and by
the caching_sha2 paths in PPHR_verify_password. Both prior comments
claimed "nothing downstream consumes the native scramble after an
ed25519 switch" -- that was false.
Concretely: client authenticates as native user A (scramble S
generated); COM_CHANGE_USER to $ED$ user B overwrites scramble_buff
with the ed25519 nonce; COM_CHANGE_USER back to user A with inline
auth data computed against S now fails verification against the
mutated buffer -- a spurious access-denied on a legitimate change-user
back to the original user. Fail-closed, but it breaks the exact
change-user workload this feature exists to support.
Fix: give the ed25519 nonce its own field, MySQL_Connection::
ed25519_nonce[ED25519_NONCE_LEN], declared next to scramble_buff in
include/mysql_connection.h. Updated all four sites that touched the
nonce via scramble_buff to use the dedicated field instead:
- PPHR_ed25519_switch (RAND_bytes target)
- the COM_CHANGE_USER ed25519 branch in process_pkt_COM_CHANGE_USER
(RAND_bytes target)
- generate_pkt_auth_switch_request's AUTH_MYSQL_ED25519 case
(memcpy source when building the AuthSwitchRequest packet)
- PPHR_ed25519_verify (nonce argument to
proxysql_ed25519_verify_signature)
Because the nonce no longer aliases scramble_buff, the NUL-terminator
write at scramble_buff[ED25519_NONCE_LEN] that a prior review added to
both RAND_bytes sites is now obsolete and has been removed along with
its comment: scramble_buff is never written by the ed25519 path
anymore, so its terminator (written once by
proxy_create_random_string() during the initial handshake) is never
disturbed. That terminator-preservation was itself a correctness fix
for the DEBUG-only hex(scramble_buff) handshake dump; removing the
write it was protecting is safe precisely because the write is gone.
Runs on the mariadb10-galera infra (@proxysql_min_version:3.1): installs auth_ed25519 on the backend and covers the full matrix -- cleartext-stored user through to backend query execution, $ED$ public-key-only user (frontend OK, backend fails as documented), wrong-password 1045 for both formats, COM_CHANGE_USER via Auth Switch, and additional-password retry. The TAP client itself answers the client_ed25519 Auth Switch because the vendored connector now links the plugin statically.
The $ED$ (public-key-only) backend-query assertion in test_ed25519_auth-t.cpp only checked that the query failed (rc != 0), which a Galera blip, backend outage, or unrelated regression would also satisfy, making the row unable to prove the documented public-key-only limitation. Tighten it to assert the specific failure: errno 1045 with an 'Access denied' message, confirmed empirically against the mariadb10-galera infra (ProxySQL forwards the backend's own native 1045 access-denied response verbatim to the client when the backend connection retry fails for a $ED$ user). Also diag() the observed errno/error unconditionally so future failures are diagnosable.
Formats, MariaDB migration path, protocol behavior, tier availability, and the documented limitations ($ED$ backend connections, passthrough incompatibility, single-auth-switch edge, PARSEC out of scope).
…artext
Security gap found during a review of the ed25519 documentation task:
proxysql_ed25519_is_pubkey_format() requires an exact 47-char "$ED$" +
43-base64 credential. A stored mysql_users.password that begins with
"$ED$" but has the WRONG length (e.g. "$ED$short") is not pubkey-format,
so every routing decision that gated on is_pubkey_format() fell through
to ordinary cleartext/native-password verification -- a client that
typed the literal stored string "$ED$short" as its password would
authenticate successfully. The load-time warning added in an earlier
task already fires for any "$ED$"-prefixed value and claims "every
authentication attempt for this user will fail", which was false for
exactly this subcase.
Human ruling: fix the code, fail-closed. ANY password beginning with the
"$ED$" prefix (case-insensitive), valid or not, is a reserved marker for
ed25519 credentials and must never be treated as a cleartext password.
A malformed "$ED$..." row denies authentication with the standard
access-denied, same as any other credential mismatch -- it does not get
a distinguishable error. The existing warning wording needed no change
under this ruling, since it is now accurate.
Fix:
- New proxysql_ed25519_has_prefix() (include/MySQL_Ed25519.h,
lib/MySQL_Ed25519.cpp): NULL-safe "$ED$" marker test, independent of
length/validity. proxysql_ed25519_is_pubkey_format() is reimplemented
on top of it (has_prefix() + exact-length check) to stay DRY; its own
contract (require full 47-char validity) is unchanged, and
proxysql_ed25519_decode_pubkey() -- which calls is_pubkey_format()
internally -- is also unchanged, so decoding still requires a valid key.
- Every routing decision in lib/MySQL_Protocol.cpp that used
is_pubkey_format() to decide *which auth method to run* now routes on
has_prefix() instead, so a malformed "$ED$..." value is steered into
the ed25519 verification path (which then denies it generically via
decode_pubkey() failure) rather than falling through to cleartext
comparison:
- PPHR_verify_password's stage-0 gate (stored_is_ed)
- PPHR_verify_password's stage-2 credential-format dispatch
- PPHR_ed25519_verify's internal decode-vs-derive selector (this one
matters most: without it, a "$ED$short" row would hit the
MariaDB-variant "derive key from cleartext password" branch --
deriving from the reserved marker string itself, still a form of
treating it as cleartext)
- process_pkt_COM_CHANGE_USER's ed25519_switch_needed
verify_user_pass()'s cleartext branch (the COM_CHANGE_USER inline-data
path) now also denies outright on has_prefix() before considering any
auth_plugin_id, as defense in depth independent of caller-side gating.
mysql_connection.cpp's backend-connect warning switched from
is_pubkey_format() to has_prefix() for the same reason: a "$ED$short"
user is just as unusable against a real backend as a valid one.
process_pkt_auth_swich_response() (lib/MySQL_Protocol.cpp ~1460) has
the same unguarded cleartext branch but is dead code -- grepped for
call sites, found only its own declaration/definition, so it was left
untouched rather than edited for no reachable effect.
Edge cost, called out per the ruling: a legitimate cleartext password
that happens to start with the literal 4 characters "$ED$" is no longer
usable for that user -- the prefix is now a fully reserved marker.
Tests:
- test/tap/tests/unit/ed25519_unit-t.cpp: 5 new assertions for
proxysql_ed25519_has_prefix (valid 47-char credential, malformed
"$ED$short", case-insensitive "$ed$...", bare base64 without marker,
NULL), plan() 19 -> 24.
- test/tap/tests/test_ed25519_auth-t.cpp: new user 'ed_user_bad' with
mysql_users.password literally '$ED$short' (no backend user needed --
frontend denial happens before any backend connection); connecting
with password '$ED$short' now fails with 1045 instead of succeeding.
plan() 10 -> 11.
Verification:
- PROXYSQL31=1 make debug -j$(nproc): exit 0.
- ed25519_unit-t: 24/24 ok.
- PROXYSQL31=1 make build_tap_test_debug: exit 0.
- Isolated harness, WORKSPACE=<worktree> INFRA_ID=ed25519
TAP_GROUP=mariadb10-galera-g4 TEST_PY_TAP_INCL=test_ed25519_auth-t:
1..11, all ok, including "ok 7 - malformed $ED$ credential
('$ED$short') denied with 1045, not accepted as cleartext". SUMMARY:
PASS 1/407 FAIL 0/407, ret_rc = [0].
…o nonce
Final-review fix wave, code findings:
- lib/MySQL_Protocol.cpp: the ed25519 gates were written as "!= CLICKHOUSE"
rather than "== MYSQL", so under PROXYSQL31 they also applied to
PROXYSQL_SESSION_ADMIN and PROXYSQL_SESSION_STATS, which authenticate
through this same verify_user_pass()/PPHR_verify_password()/
process_pkt_COM_CHANGE_USER() code. A $ED$-prefixed admin/stats
password would therefore have been routed into the ed25519 switch (an
admin-port connection cannot complete a MySQL-protocol Auth Switch, so
this would have been an unrecoverable admin-port lockout) or denied
outright by the has_prefix guard in verify_user_pass(). All three gates
now check `session_type == PROXYSQL_SESSION_MYSQL` explicitly:
- the stage-0 gate in PPHR_verify_password (~L3603)
- `ed25519_switch_needed` in process_pkt_COM_CHANGE_USER (~L1891)
- the has_prefix denial in verify_user_pass (~L1571)
With the gate scoped to MYSQL, a $ED$-prefixed admin password now falls
through to the pre-existing cleartext comparison unchanged from before
this feature -- the intended conservative behavior for non-MySQL
sessions, since the feature simply doesn't apply there.
- lib/MySQL_Protocol.cpp (~L1227-1230): braced the previously-unbraced
`#ifdef`-wrapped `if` guarding the trailing-NUL write in
generate_pkt_auth_switch_request(). The guard suppresses the NUL byte
for the ed25519 packet (which must end exactly after the 32-byte nonce,
with no extra byte), and was a single unbraced statement -- a future
line inserted directly below it would have silently escaped the guard
and corrupted that one-byte heap write. No behavior change.
- lib/mysql_connection.cpp: zero-initialize `ed25519_nonce` in the
MySQL_Connection constructor. The field was previously left
uninitialized between construction and the first RAND_bytes() fill in
PPHR_ed25519_switch(); harmless in practice (nothing reads it before
that fill), but leaves ASAN/MSAN runs clean on the theoretical
uninitialized-read path.
Final-review fix wave, test-coverage finding.
The e2e test's headline assertions didn't exercise what their names
claimed. The vendored MariaDB connector only sends client_ed25519 in the
handshake response if MYSQL_DEFAULT_AUTH requests it; 'ed_user' has a
plain cleartext ProxySQL credential (not $ED$-prefixed), so with no
client-side plugin request every one of its connections silently ran
over plain mysql_native_password. Assertions 1 ("connects via ed25519
auth switch"), 8-9 (COM_CHANGE_USER "into ed25519 user"), and 10-11
("additional-password retry verifies ed25519 signature") never touched
the ed25519 code path at all -- mislabeled coverage that would not have
caught a regression in the client-requested-switch branch
(`ed25519_switch_needed`'s `client_auth_plugin` check) or in signature
verification driven by that path.
Fixes, three parts:
1. Set `mysql_options(c, MYSQL_DEFAULT_AUTH, "client_ed25519")` before
mysql_real_connect() on the two 'ed_user' cleartext-credential
connections (initial connect, and the additional-password retry).
This makes the client actually request client_ed25519, exercising
switch-policy rule 2 (client request + usable cleartext credential ->
derive key from the stored cleartext and verify the signature).
2. Added a new COM_CHANGE_USER case using 'ed_user_pk' (the $ED$-stored
user): `mysql_change_user(c, "ed_user_pk", ED_PASS, NULL)`. A $ED$
stored credential forces the ed25519 change-user switch unconditionally
(process_pkt_COM_CHANGE_USER's ed25519_switch_needed fires on the
stored-credential prefix alone), so this needs no MYSQL_DEFAULT_AUTH
option and gives coverage independent of client plugin negotiation.
The change_user call itself is asserted ok (frontend signature
verification succeeded); the following query is asserted to fail with
the same 1045/"Access denied" backend limitation as the initial-connect
$ED$ case, since the backend still can't complete auth without the
cleartext password.
3. Renamed every affected assertion description to state the path it
actually proves: the block-1 connect is now "connects via ed25519 auth
switch (client-requested, key derived from cleartext)"; the pre-existing
COM_CHANGE_USER-into-ed_user block is now explicitly labeled
"(native auth path, cleartext credential)" since it still doesn't
request client_ed25519 and was left as ordinary native-auth coverage
(the ed25519 change-user path is now covered by the new block instead);
the additional-password block is labeled "(client-requested switch)".
plan() raised from 11 to 13 to match the two new assertions (change_user
success + post-change_user query failure) from part 2.
Verified against mariadb10-galera-g4 with the corresponding lib/ fix
commit: 13/13 assertions pass, and the ProxySQL debug log confirms actual
routing through the derivation path for 'ed_user':
MySQL_Protocol.cpp:3111:PPHR_ed25519_switch(): ... user='ed_user' . Sent client_ed25519 Auth Switch
MySQL_Protocol.cpp:3150:PPHR_ed25519_verify(): ... user='ed_user' . ed25519 signature verification succeeded
…de note Final-review fix wave, doc findings. - Protocol behavior section: state plainly that frontend ed25519 requires mysql-default_authentication_plugin=mysql_native_password (ProxySQL's built-in default, so unaffected deployments need no change). If it's set to caching_sha2_password instead, ProxySQL advertises caching_sha2_password in the greeting and an ordinary client -- one that hasn't explicitly requested client_ed25519 -- switches early to caching_sha2_password before ProxySQL can route it into the ed25519 exchange; on that path a $ED$-stored user is denied unconditionally without ever reaching ed25519 verification. This was previously only implicit in the "single auth switch" limitation bullet. - New "Upgrading from 3.0" section: the $ED$ prefix becomes reserved as of this feature. A pre-existing 3.0 deployment with a cleartext password that coincidentally starts with the literal "$ED$" stops authenticating after upgrade, since that value is now parsed as an ed25519 credential and never compared as cleartext. This is intentional fail-closed behavior (human-approved during design): a parse failure denies access rather than silently falling back to cleartext comparison of an unparseable "$ED$..." value. ProxySQL logs a warning for the affected account on each connection attempt (MySQL_Protocol.cpp's PPHR_ed25519_verify(), "has a malformed $ED$ ed25519 credential; denying access").
Scoped re-review residual on the ed25519 final-fix wave. Human ruling on the finding, implemented exactly: The previous commit (5780da2) scoped all three ed25519-related session checks in lib/MySQL_Protocol.cpp down to `== PROXYSQL_SESSION_MYSQL` to fix an admin/stats lockout risk. That was correct for the two gates that control the ed25519 nonce-based Auth Switch EXCHANGE (the stage-0 gate in PPHR_verify_password and ed25519_switch_needed in process_pkt_COM_CHANGE_USER) -- only a MySQL-wire client can complete a MySQL Auth Switch, so scoping the exchange to MYSQL is a protocol necessity, not a policy choice. But it over-narrowed the fail-closed $ED$-prefix DENIAL in verify_user_pass's cleartext-password branch, which is a different kind of check: a trust decision, not a protocol capability. That denial's purpose is to guarantee a "$ED$"-prefixed stored credential (including a malformed one, e.g. "$ED$short") is NEVER compared as a literal cleartext password. Narrowing it to MYSQL-only reopened exactly the cleartext fall-through the original fail-closed fix (2b85860) was written to close, but now on PROXYSQL_SESSION_SQLITE: ProxySQL's SQLite3-server frontend (its own admin-clone SQL port) authenticates against the same mysql_users rows via the same USERNAME_FRONTEND credential scope in GloMyAuth. A stored "$ED$short" value would again be compared as a literal password on that port, and the stored string itself would authenticate. Ruling: the fail-closed $ED$ reservation applies to both PROXYSQL_SESSION_MYSQL and PROXYSQL_SESSION_SQLITE (both frontend consumers of the same credential rows); the ed25519 auth exchange itself remains MYSQL-only, unchanged. Change: verify_user_pass's has_prefix denial condition changed from `session_type == PROXYSQL_SESSION_MYSQL` to `(session_type == PROXYSQL_SESSION_MYSQL || session_type == PROXYSQL_SESSION_SQLITE)`, with an updated comment explaining the two-different-scopes distinction (reservation vs. exchange) so this isn't re-collapsed to one scope again in a future edit. The stage-0 gate and process_pkt_COM_CHANGE_USER's ed25519_switch_needed gate are untouched -- both stay MYSQL-only per the ruling. Verified: the handshake-path dispatch that also denies $ED$-prefixed credentials -- `if (auth_plugin_id == AUTH_MYSQL_ED25519 || proxysql_ed25519_has_prefix(vars1.password))` in PPHR_verify_password (~MySQL_Protocol.cpp:3678, routing into PPHR_ed25519_verify) -- already carries no session-type condition at all, so SQLite3-server handshake auth for a $ED$-stored user was already routed to PPHR_ed25519_verify and denied (that path never needed this fix); the residual was isolated to verify_user_pass's cleartext-password branch, used by the COM_CHANGE_USER / secondary verification path. Testing: - PROXYSQL31=1 make debug: exit 0 - test/tap/tests/unit/ed25519_unit-t: 24/24 (unchanged, no unit test touches this scoping) - e2e test_ed25519_auth-t against mariadb10-galera-g4: 13/13 (unchanged from before this commit -- confirms MySQL-session behavior, which is all that test exercises, is untouched by widening the SQLite3 denial)
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2026-04-11T13:17:55.508ZApplied to files:
📚 Learning: 2026-04-11T13:17:55.509ZApplied to files:
🪛 LanguageTooldocs/superpowers/specs/2026-08-11-ed25519-authentication-design.md[locale-violation] ~8-~8: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects. (AFTERWARDS_US) 🔇 Additional comments (1)
📝 WalkthroughWalkthroughProxySQL adds feature-gated MariaDB Ed25519 authentication. The change includes static connector support, credential and signature helpers, frontend Auth Switch handling, backend validation, ChangesEd25519 authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MySQL_Protocol
participant MySQL_Authentication
Client->>MySQL_Protocol: Request client_ed25519 authentication
MySQL_Protocol->>Client: Send Auth Switch with 32-byte nonce
Client->>MySQL_Protocol: Return 64-byte signature
MySQL_Protocol->>MySQL_Authentication: Obtain cleartext or $ED$ credential
MySQL_Authentication-->>MySQL_Protocol: Return derived or decoded public key
MySQL_Protocol-->>Client: Accept or reject authentication
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}
![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: 38ce7b1fea
ℹ️ 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".
| // malformed "$ED$..." stored credential must still be denied via | ||
| // PPHR_ed25519_verify()'s generic failure, never treated as a | ||
| // cleartext/native password comparison below. | ||
| if (auth_plugin_id == AUTH_MYSQL_ED25519 || proxysql_ed25519_has_prefix(vars1.password)) { |
There was a problem hiding this comment.
$ED$ routing out of admin authentication
When a v3.1 admin-admin_credentials or admin-stats_credentials password begins with $ED$, the stage-0 gate correctly avoids an ed25519 switch for the non-MySQL session, but this unconditional prefix check still sends the normal 20-byte native response to PPHR_ed25519_verify(), which requires 64 bytes and rejects it before the existing cleartext/native comparison. An upgrade can therefore lock out affected admin or stats accounts, despite ed25519 being scoped to MySQL frontends; restrict prefix-based verification here to the intended MySQL/SQLite credential scopes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 33bdf5f. The dispatch is now scoped to MYSQL||SQLITE sessions: MYSQL is the only session type the exchange is offered on, SQLITE shares the USERNAME_FRONTEND credential rows so the fail-closed
| if ((*myds)->switching_auth_stage == 0 && | ||
| (*myds)->switching_auth_sent != AUTH_MYSQL_ED25519 && | ||
| (*myds)->sess->session_type == PROXYSQL_SESSION_MYSQL) { | ||
| const bool stored_is_ed = proxysql_ed25519_has_prefix(vars1.password); |
There was a problem hiding this comment.
For a dual-password account with an ordinary primary credential and a $ED$ additional credential, a default client offers native authentication and this stage-0 decision examines only the primary password, so ProxySQL never sends the required ed25519 switch. The later additional-password retry then routes the already-received 20-byte native response into the 64-byte signature verifier and always fails, making the $ED$ additional credential unusable unless the client explicitly forces client_ed25519; include the additional credential when deciding whether the initial switch is mandatory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not implementing the auto-switch; documented instead (88a91c0, doc/ed25519_authentication.md Limitations). Deciding the switch from the ADDITIONAL credential's format would force every client of such an account through ed25519 — including clients without the client_ed25519 plugin whose primary credential is perfectly valid — turning a working primary-auth configuration into a hard failure. The documented contract is: a
![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 (1)
lib/MySQL_Protocol.cpp (1)
1919-1935: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMirror the
cred_usablecheck from the handshake gate.
ed25519_switch_neededtriggers the Auth Switch whenever the client namesclient_ed25519, regardless of the stored credential format. The handshake gate inPPHR_verify_password(Lines 3643-3653) does not do that. It computescred_usableand refuses the switch when the stored value is a*-hash or a$A$0hash, because no Ed25519 key can be derived from those.On this path a client that requests
client_ed25519against a*-hashed user gets a full Auth Switch round trip.PPHR_ed25519_verifythen callsproxysql_ed25519_derive_public_keyon the literal hash text. The result is a generic denial, so this is fail-closed and not an authentication bypass. It still wastes a round trip and diverges from the gate that the comments describe as its mirror.♻️ Proposed alignment with the handshake gate
+ // a '*SHA1' or '$A$' hash cannot derive an ed25519 key + const bool stored_is_ed = proxysql_ed25519_has_prefix(password); + const bool cred_usable = stored_is_ed || + (password[0] != '*' && + !(strlen(password) == 70 && strncasecmp(password, "$A$0", 4) == 0)); const bool ed25519_switch_needed = session_type == PROXYSQL_SESSION_MYSQL && - (proxysql_ed25519_has_prefix(password) || - (client_auth_plugin && strcmp(client_auth_plugin, plugins[AUTH_MYSQL_ED25519]) == 0)); + (stored_is_ed || + (cred_usable && client_auth_plugin && + strcmp(client_auth_plugin, plugins[AUTH_MYSQL_ED25519]) == 0));🤖 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 1919 - 1935, Update ed25519_switch_needed to mirror the cred_usable validation used by PPHR_verify_password: only request the Ed25519 auth switch when the stored credential can produce an Ed25519 key, excluding *-hash and $A$0 formats. Keep the existing session-type, password-prefix, and client_auth_plugin conditions intact while reusing the handshake gate’s credential-format logic.
🤖 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-11-ed25519-authentication.md`:
- Line 463: Correct the documented TAP assertion counts in
docs/superpowers/plans/2026-08-11-ed25519-authentication.md: at lines 463-463,
change 1..19 to 1..24 to match test/tap/tests/unit/ed25519_unit-t.cpp; at lines
1204-1204, change 1..11 to 1..10 to match the embedded plan(10).
In `@docs/superpowers/specs/2026-08-11-ed25519-authentication-design.md`:
- Around line 101-105: Update the protocol-flow fenced blocks to declare the
text language by changing the opening fences to ```text in
docs/superpowers/specs/2026-08-11-ed25519-authentication-design.md lines 101-105
and docs/superpowers/plans/2026-08-11-ed25519-authentication.md lines 497-504.
In `@include/mysql_connection.h`:
- Around line 126-132: Update the recursive lib make invocation in the unit-test
Makefile to pass through PROXYSQLED25519=$(PROXYSQLED25519), ensuring library
objects use the same MySQL_Connection layout as the unit-test objects when
rebuilding after header changes.
In `@lib/mysql_connection.cpp`:
- Around line 1034-1045: Remove the per-connection proxy_warning block guarded
by PROXYSQLED25519 from connect_start, since MySQL_Authentication::add already
reports the credential-load issue once per user. Ensure the connect path
performs no formatted warning for $ED$ credentials and avoid passing a
potentially null userinfo->username to logging.
In `@lib/MySQL_Ed25519.cpp`:
- Around line 41-50: The proxysql_ed25519_decode_pubkey function must reject
non-canonical Base64 payloads. Validate all 43 stored payload characters against
the Base64 alphabet before appending padding, decode exactly 32 key bytes,
re-encode the result without padding, and require an exact 43-character match
with the original payload; add regression coverage for embedded “=”, trailing
“=”, and non-canonical final symbols.
In `@test/tap/tests/test_ed25519_auth-t.cpp`:
- Around line 90-95: Update the fixture setup around the create_user and
create_user_pk statements to drop both dedicated accounts before recreating them
with ED_PUBKEY. Ensure the drop-and-create sequence replaces any existing
authentication strings, while preserving the current account names and ed25519
configuration.
In `@test/tap/tests/unit/Makefile`:
- Around line 283-287: Update the unit-test Makefile to derive PROXYSQLED25519=1
when PROXYSQL31=1, forward PROXYSQLED25519 through the recursive library build,
and register ed25519_unit-t based on this feature flag rather than the
parse-time nm probe of LIBPROXYSQLAR.
---
Nitpick comments:
In `@lib/MySQL_Protocol.cpp`:
- Around line 1919-1935: Update ed25519_switch_needed to mirror the cred_usable
validation used by PPHR_verify_password: only request the Ed25519 auth switch
when the stored credential can produce an Ed25519 key, excluding *-hash and $A$0
formats. Keep the existing session-type, password-prefix, and client_auth_plugin
conditions intact while reusing the handshake gate’s credential-format logic.
🪄 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: 85cabd3c-f657-481a-b240-1ea7c87bf05c
📒 Files selected for processing (19)
Makefiledeps/mariadb-client-library/plugin_auth_CMakeLists.txt.patchdoc/ed25519_authentication.mddocs/superpowers/plans/2026-08-11-ed25519-authentication.mddocs/superpowers/specs/2026-08-11-ed25519-authentication-design.mdinclude/MySQL_Ed25519.hinclude/MySQL_Protocol.hinclude/mysql_connection.hlib/Makefilelib/MySQL_Authentication.cpplib/MySQL_Ed25519.cpplib/MySQL_Protocol.cpplib/mysql_connection.cpplib/mysql_data_stream.cppsrc/Makefiletest/tap/groups/groups.jsontest/tap/tests/test_ed25519_auth-t.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/ed25519_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- 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: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: Gitar
- GitHub Check: run / trigger
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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_connection.hinclude/MySQL_Protocol.hlib/MySQL_Ed25519.cpptest/tap/tests/unit/ed25519_unit-t.cpplib/MySQL_Authentication.cpptest/tap/tests/test_ed25519_auth-t.cppinclude/MySQL_Ed25519.hlib/mysql_connection.cpplib/MySQL_Protocol.cpp
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/mysql_connection.hinclude/MySQL_Protocol.hinclude/MySQL_Ed25519.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/unit/ed25519_unit-t.cpptest/tap/tests/test_ed25519_auth-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/ed25519_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/ed25519_authentication.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/ed25519_authentication.mddocs/superpowers/specs/2026-08-11-ed25519-authentication-design.mddocs/superpowers/plans/2026-08-11-ed25519-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:
doc/ed25519_authentication.mddocs/superpowers/specs/2026-08-11-ed25519-authentication-design.mddocs/superpowers/plans/2026-08-11-ed25519-authentication.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/unit/ed25519_unit-t.cpptest/tap/tests/test_ed25519_auth-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/ed25519_unit-t.cpp
🪛 ast-grep (0.45.1)
test/tap/tests/test_ed25519_auth-t.cpp
[error] 270-270: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(hexpass + 2 * i, "%02x", (unsigned char)ED_PASS[i])
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🪛 LanguageTool
doc/ed25519_authentication.md
[style] ~23-~23: Consider an alternative for the overused word “exactly”.
Context: ...ord is unknown | The $ED$ payload is exactly the value MariaDB stores in `mysql.user...
(EXACTLY_PRECISELY)
🪛 markdownlint-cli2 (0.23.2)
docs/superpowers/specs/2026-08-11-ed25519-authentication-design.md
[warning] 101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/superpowers/plans/2026-08-11-ed25519-authentication.md
[warning] 58-58: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 69-69: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 497-497: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (23)
test/tap/groups/groups.json (1)
28-28: LGTM!Also applies to: 370-370
doc/ed25519_authentication.md (1)
1-88: LGTM!Makefile (1)
63-74: LGTM!Also applies to: 110-110, 418-432
deps/mariadb-client-library/plugin_auth_CMakeLists.txt.patch (1)
5-13: LGTM!lib/Makefile (1)
76-91: LGTM!Also applies to: 143-146
src/Makefile (1)
96-109: LGTM!include/MySQL_Ed25519.h (1)
1-48: LGTM!include/MySQL_Protocol.h (1)
40-43: LGTM!Also applies to: 221-224
include/mysql_connection.h (1)
31-34: LGTM!lib/MySQL_Protocol.cpp (10)
21-24: LGTM!Also applies to: 95-101
1558-1593: LGTM!Also applies to: 1618-1625
2200-2204: LGTM!
2453-2459: LGTM!
3100-3125: LGTM!
3135-3164: LGTM!
3673-3684: LGTM!
3827-3832: LGTM!Also applies to: 3860-3865
3638-3655: 🔒 Security & PrivacyNo change required.
PPHR_verify_passwordinitializesrettofalse, andPPHR_5passwordTruedoes not modify it. No earlier path setsrettotruebefore this branch.> Likely an incorrect or invalid review comment.
1173-1179: 🗄️ Data Integrity & IntegrationKeep the
AUTH_MYSQL_ED25519packet without a trailing NUL.client_ed25519requires exactlyNONCE_BYTES(32 bytes), so the packet length and serialization are correct.lib/mysql_data_stream.cpp (1)
1946-1950: LGTM!lib/MySQL_Authentication.cpp (2)
19-22: LGTM!
169-179: 🔒 Security & PrivacyKeep the case-insensitive prefix check.
proxysql_ed25519_has_prefixalso usesstrncasecmp, so the loader and authentication paths recognize$ed$,$Ed$, and$ED$consistently.> Likely an incorrect or invalid review comment.lib/mysql_connection.cpp (1)
19-22: LGTM!Also applies to: 506-511
| PSQLED25519 := | ||
| ifneq ($(shell nm $(LIBPROXYSQLAR) 2>/dev/null | grep -c proxysql_ed25519_verify_signature),0) | ||
| PROXYSQLED25519 := 1 | ||
| PSQLED25519 := -DPROXYSQLED25519 | ||
| endif |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Propagate the Ed25519 flag before the archive probe.
When a clean tree runs the unit-test Makefile with PROXYSQL31=1, this Makefile does not derive PROXYSQLED25519. Its recursive library build also does not forward that variable. lib/Makefile then omits MySQL_Ed25519.oo, and the parse-time nm probe leaves ed25519_unit-t out of UNIT_TESTS.
Derive PROXYSQLED25519=1 from PROXYSQL31 in this Makefile. Forward it at lines 382-386. Register the test from that feature flag instead of from symbols in a pre-existing archive.
🤖 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/Makefile` around lines 283 - 287, Update the unit-test
Makefile to derive PROXYSQLED25519=1 when PROXYSQL31=1, forward PROXYSQLED25519
through the recursive library build, and register ed25519_unit-t based on this
feature flag rather than the parse-time nm probe of LIBPROXYSQLAR.
Four review findings from PR #6033 (Codex, CodeRabbit, gitar): - Scope the $ED$ credential dispatch in PPHR_verify_password to MYSQL||SQLITE sessions (Codex P1). The dispatch was session-unscoped, so under PROXYSQL31 a $ED$-prefixed admin/stats password routed the 20-byte native response into the 64-byte signature check -- an unrecoverable admin-port lockout on upgrade. ADMIN/STATS now fall through to the pre-existing cleartext comparison; SQLITE keeps the fail-closed reservation, MYSQL keeps the exchange. - Mirror the cred_usable check in the COM_CHANGE_USER gate (CodeRabbit): an explicit client_ed25519 request against a '*SHA1'/'$A$' credential no longer wastes an Auth Switch round trip on a doomed exchange. - Strict canonical base64 in proxysql_ed25519_decode_pubkey (CodeRabbit): EVP_DecodeBlock alone treats '=' anywhere as six zero bits and accepts non-canonical trailing bits, silently decoding a corrupted credential to a DIFFERENT key; a re-encode round-trip now rejects embedded '=', trailing '=', and non-canonical final symbols. Unit tests added for all three (24 -> 27 assertions). - Rate-limit the $ED$ backend-connect warning to once per user (CodeRabbit Major + gitar): connect_start is a hot path and the pool retries failed connects, so a misconfigured user could flood the log. Also NULL-safe on username. Additionally: convert the ED25519_* macros to inline constexpr (SonarCloud S5028) and forward PROXYSQLED25519 to the unit-test Makefile's recursive libproxysql.a rebuild (CodeRabbit) so a header-triggered rebuild cannot produce a MySQL_Connection layout mismatch between the archive and unit-test objects.
- sprintf -> snprintf in the hex-encoding loop (SonarCloud S6069, the finding that failed the PR quality gate's security rating). - Drop-and-recreate the backend fixture accounts instead of CREATE USER IF NOT EXISTS (CodeRabbit): a stale account left by a previous run with a different authentication string would fail the ED_PASS assertions for the wrong reason. - ED25519_PK_BACKEND_ERRNO macro -> constexpr (SonarCloud S5028). Verified: 13/13 e2e assertions on mariadb10-galera-g4 against the rebuilt binary; 27/27 unit assertions.
…lish - doc/ed25519_authentication.md: a $ED$ public key stored as the ADDITIONAL password requires the client to explicitly request client_ed25519 (Codex P2). Deliberately documented rather than auto-switching on the additional credential's format: forcing every client of such an account through ed25519 would break clients without the plugin whose primary credential is perfectly valid. - Add language identifiers to protocol-flow fences (markdownlint MD040). - Mark the implementation plan as a historical artifact: review-driven fix rounds amended assertion counts and internals after it was written; the shipped code and tests are authoritative (CodeRabbit flagged the stale embedded counts).
|
Bot-review round addressed in 33bdf5f / 9df1b97 / 88a91c0 — per-thread replies inline. Two items handled outside threads: CodeRabbit review-body nitpick (cred_usable in the COM_CHANGE_USER gate): fixed in 33bdf5f — the change-user gate now mirrors the handshake gate and refuses the switch for an explicit client_ed25519 request against a '*SHA1'/'$A$' credential instead of spending a round trip on a doomed exchange. SonarCloud quality gate: the failure was the security rating (4 > 1), driven by the S6069
Verification after the fixes: unit |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6033 +/- ##
==========================================
+ Coverage 53.34% 53.54% +0.19%
==========================================
Files 492 494 +2
Lines 146700 146960 +260
Branches 37082 37175 +93
==========================================
+ Hits 78256 78683 +427
+ Misses 51227 50907 -320
- Partials 17217 17370 +153
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:
|
…19 arm auth_plugin is already const char*; strncmp takes const char* directly. Clears the SonarCloud M23_090 critical flagged on this new-code line (the identical pre-existing casts on the adjacent legacy arms are deliberately left untouched).
Fix where the fix is a real improvement, NOSONAR with a stated reason where the flagged pattern is deliberate: - New ed25519_cred_usable() helper shared by the initial-handshake and COM_CHANGE_USER gates: removes the duplicated inline '$A$0' format test (a deferred finding from the branch's final review) and uses a bounded strnlen scan (S5813 x2). - Compile-time ED25519_PLUGIN_NAME_LEN replaces five strlen(plugins[AUTH_MYSQL_ED25519]) calls on the fixed plugin name (S5813 x5). - proxysql_ed25519_is_pubkey_format uses a bounded strnlen (S5813). - ED_PASS in the e2e test becomes a constexpr array; the hex loop bounds on sizeof (S5813). - NOSONAR with justification on: the arbitrary-length password strlen feeding key derivation (MariaDB hashes the whole password, no bound exists); the ref10-mandated const_cast (upstream C API takes non-const pw it never modifies); the KAT-literal strlen calls in the unit test; and the TAP-convention linear main in the e2e test (S3776). Verified: 27/27 unit, 13/13 e2e on mariadb10-galera against the rebuilt binary.
A manual job-level rerun of the transient mariadb10-galera-g6 runner failure (the run-level rerun API correctly refused; the job-level API did not) superseded the run's attempt-1 results with a startup_failure attempt, hiding the ~70 green checks from the PR view. The workflow_run trigger chain (doc/GH-Actions/README.md) cannot restart mid-tree, so a fresh head SHA is the supported way to regenerate the complete matrix. No code changes. The previously green results remain visible in the attempt-1 run history; g6's single test (prepare_statement_err3024-t) was additionally verified locally against this branch (PASS 1/408).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 19 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_connection.cpp">
<violation number="1" location="lib/mysql_connection.cpp:1053">
P3: This warning dedupe cache can grow indefinitely in long-lived processes because every distinct misconfigured username is stored forever. Consider bounding or resetting the cache on user-table reload so deduplication does not create permanent per-username memory growth.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| bool ed25519_first_warning = false; | ||
| { | ||
| std::lock_guard<std::mutex> lock(ed25519_warned_mutex); | ||
| ed25519_first_warning = ed25519_warned_users.insert(ed25519_uname).second; |
There was a problem hiding this comment.
P3: This warning dedupe cache can grow indefinitely in long-lived processes because every distinct misconfigured username is stored forever. Consider bounding or resetting the cache on user-table reload so deduplication does not create permanent per-username memory growth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/mysql_connection.cpp, line 1053:
<comment>This warning dedupe cache can grow indefinitely in long-lived processes because every distinct misconfigured username is stored forever. Consider bounding or resetting the cache on user-table reload so deduplication does not create permanent per-username memory growth.</comment>
<file context>
@@ -1021,6 +1033,33 @@ void MySQL_Connection::connect_start() {
+ bool ed25519_first_warning = false;
+ {
+ std::lock_guard<std::mutex> lock(ed25519_warned_mutex);
+ ed25519_first_warning = ed25519_warned_users.insert(ed25519_uname).second;
+ }
+ if (ed25519_first_warning) {
</file context>
There was a problem hiding this comment.
Not changing this. The set's growth is bounded by the number of distinct usernames in mysql_users that carry a
From the cubic review round on PR #6033: - Plugin-name negotiation for ed25519 now uses exact strcmp in PPHR_3 and verify_user_pass (safe: both parsers reject unterminated names). The previous strncmp prefix match would have negotiated a non-standard name like 'client_ed25519_x' into the ed25519 flow; fail-closed, but sloppy, and inconsistent with the COM_CHANGE_USER gate which already matched exactly. - Makefile tier-cascade comment now names PROXYSQLED25519. - Upgrade note states the $ED$ reservation is case-insensitive and reflects the rate-limited warning behavior (once at load, once per user at backend connect - not per attempt). - Design spec gains a post-implementation note: the shipped nonce lives in MySQL_Connection::ed25519_nonce, not the data-stream location the spec described; plus the other post-review deviations. Verified: build clean, 27/27 unit, 13/13 e2e on mariadb10-galera.
Code Review ✅ Approved 1 resolved / 1 findingsAdds MariaDB ed25519 authentication support for both frontend and backend connections, including credential verification, auth switching, and comprehensive test coverage. Resolved an issue where the ✅ 1 resolved✅ Quality:
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
1 participant
Summary
Implements MariaDB's ed25519 authentication scheme (
client_ed25519/auth_ed25519) on both sides of ProxySQL, per the design spec (docs/superpowers/specs/2026-08-11-ed25519-authentication-design.md) and implementation plan committed on this branch.client_ed25519statically (plugin_auth_CMakeLists.txt.patch), so backend connections answer a MariaDB server's ed25519 auth switch transparently — backend pools, Monitor, and cluster sync included. NoMYSQL_DEFAULT_AUTHplumbing needed.PROXYSQLED25519macro implied byPROXYSQL31): ProxySQL itself verifiesclient_ed25519signatures. The exchange always runs through an Auth Switch carrying a fresh 32-byte nonce (stored in a dedicatedMySQL_Connection::ed25519_noncebuffer — deliberately notscramble_buff, which later COM_CHANGE_USER/caching_sha2 verifications still consume). Requiresmysql-default_authentication_plugin=mysql_native_password(the default).Credential formats in
mysql_users.password$ED$+ 43-char base64 public keyThe
$ED$prefix is reserved and fail-closed: any$ED$-prefixed value that is not a valid 47-char credential is denied for authentication and never treated as a cleartext password (applies to MySQL and SQLite3-server frontends; malformed values also warn atLOAD MYSQL USERS TO RUNTIME). Seedoc/ed25519_authentication.mdfor the 3.0 upgrade note.Crypto
Reuses the connector's own ref10 implementation via a thin wrapper (
lib/MySQL_Ed25519.cpp) — no new dependency, and frontend verification and backend signing share one implementation. MariaDB's variant derives the keypair fromSHA512(password)of arbitrary length, so OpenSSL's EVP Ed25519 API cannot substitute for derivation. The unit tests include the MariaDB KB's documented"secret"→ZIgUREUg5…vector, independently confirming scheme compatibility.Scope decisions
$ED$-prefixed admin password cannot cause an admin-port lockout).COM_CHANGE_USERforcaching_sha2_password#4618).caching_sha2_passwordagainst a native greeting) cannot be re-switched for$ED$users; standard clients are unaffected.Testing
ed25519_unit-t(unit-tests-g1,@proxysql_min_version:3.1): 24 known-answer assertions — derivation KATs, decode round-trips,$ED$format edge cases, signature verify/tamper/wrong-key/wrong-nonce.test_ed25519_auth-t(mariadb10-galera-g4,@proxysql_min_version:3.1): 13 e2e assertions against real MariaDB withauth_ed25519installed — cleartext user through to backend query execution via the client-requested switch (verified in the debug log to exercise the derivation path),$ED$user frontend-OK/backend-denied with pinned errno 1045, wrong passwords, forced ed25519 COM_CHANGE_USER, malformed-$ED$denial regression, additional-password retry.PROXYSQL31).All work was task-reviewed and a final whole-branch review completed; review-driven fixes are separate commits with detailed bodies.
Summary by CodeRabbit
New Features
$ED$public-key credentials, authentication switching, andCOM_CHANGE_USER.Bug Fixes
Documentation
Tests