SCRAM verifier & md5 credential storage with SCRAM/md5 backend pass-through - PostgreSQL - #5865
SCRAM verifier & md5 credential storage with SCRAM/md5 backend pass-through - PostgreSQL#5865rahim-kanji wants to merge 28 commits into
Conversation
…oor auth selection, anti-enumeration Store a SCRAM-SHA-256 verifier or md5 hash (or plaintext) in pgsql_users.password, detected by prefix. Pick the client auth method from the stored type via a minimum-strength floor (pgsql-authentication_method): a SCRAM verifier always uses SCRAM (no per-auth PBKDF2), an md5 hash uses md5, plaintext follows the floor; a secret too weak for the floor is rejected. Unknown / too-weak users run the existing SCRAM mock to a generic failure (no 'User not found' enumeration leak). Malformed SCRAM verifiers are rejected at LOAD. Pure B-floor logic lives in a dependency-free PgSQL_AuthReconcile.cpp. No libpq/backend changes (that is Phase 2).
…d5 auth pgsql_reconcile_unit-t: 9 cases over the B-floor reconcile matrix (unit-tests-g1). pgsql-verifier_auth-t: 8 integration assertions (legacy-g4) — verifier SCRAM auth, wrong-password reject, md5-hash auth, anti-enumeration, PLUS unselectable, malformed-verifier rejected at load.
…tication ProxySQL now authenticates to PostgreSQL backends using the stored verifier instead of a plaintext password. During the client's frontend SCRAM login the recovered ClientKey and the verifier's ServerKey are harvested onto the session's backend userinfo (skipped for adhoc/plaintext-derived keys, whose salt would not match the backend's rolpassword). On the backend connection PgSQL_Connection::connect_start injects them into the bundled libpq via new conninfo params (scram_client_key/scram_server_key), or md5_secret for md5-stored users; plaintext users keep the original password path unchanged. The bundled libpq is patched (deps/postgresql/scram_verifier_auth.patch, applied by deps/Makefile) to accept the new params: scram_client_key skips saslprep+PBKDF2 and derives StoredKey=SHA256(ClientKey); scram_server_key verifies the backend ServerSignature and fails closed if absent; md5_secret is used as the inner md5 hash. The keys are excluded from PgSQL_Connection_userinfo::compute_hash so connection-pool reuse is unchanged. Precondition (spec section 9): the verifier stored in ProxySQL must be byte-identical to the backend's pg_authid.rolpassword (same salt), and all backends a user reaches must share that rolpassword. No frontend/auth-selection changes (that is Phase 1).
pgsql-verifier_passthrough-t: 3 integration assertions (legacy-g4) — the backend role's exact SCRAM verifier is stored in pgsql_users and a query reaches the backend (proving the harvested ClientKey authenticates the backend leg, no plaintext/PBKDF2), plus a salt-mismatch negative case (a fresh verifier with a different salt is rejected). LOAD ... TO RUNTIME only; backend roles dropped and runtime restored at the end.
|
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:
📝 WalkthroughWalkthroughThis PR adds PostgreSQL SCRAM verifier and MD5 secret handling, patches bundled libpq for injected authentication material, reconciles frontend authentication with stored credential types, propagates SCRAM keys through backend connections, and adds lifecycle and integration coverage. ChangesPostgreSQL verifier pass-through authentication
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PgSQL_Protocol
participant PgSQL_Connection
participant libpq
participant BackendPostgres
Client->>PgSQL_Protocol: submit authentication response
PgSQL_Protocol->>PgSQL_Protocol: reconcile stored credential and auth floor
PgSQL_Protocol->>PgSQL_Connection: propagate SCRAM keys or MD5 secret
PgSQL_Connection->>libpq: build credential-specific conninfo
libpq->>BackendPostgres: authenticate with injected material
BackendPostgres->>PgSQL_Connection: return authentication result
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements PostgreSQL frontend authentication using stored SCRAM verifiers and MD5 secrets, along with backend SCRAM pass-through authentication to avoid sending plaintext passwords. It introduces anti-enumeration mechanisms to prevent user discovery, adds validation for SCRAM verifiers at load time, and includes comprehensive integration and unit tests. A security review identified that base64-encoded sensitive key material in local stack buffers (ck_b64 and sk_b64) should be explicitly cleared before exiting the block to prevent potential memory-disclosure vulnerabilities.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| char ck_b64[64] = { 0 }; | ||
| char sk_b64[64] = { 0 }; | ||
| int n1 = pg_b64_encode((const char*)userinfo->scram_ClientKey, | ||
| (int)sizeof(userinfo->scram_ClientKey), ck_b64, (int)sizeof(ck_b64) - 1); | ||
| int n2 = pg_b64_encode((const char*)userinfo->scram_ServerKey, | ||
| (int)sizeof(userinfo->scram_ServerKey), sk_b64, (int)sizeof(sk_b64) - 1); | ||
| if (n1 > 0) ck_b64[n1] = '\0'; | ||
| if (n2 > 0) sk_b64[n2] = '\0'; | ||
| append_conninfo_param(conninfo, "scram_client_key", ck_b64); | ||
| append_conninfo_param(conninfo, "scram_server_key", sk_b64); |
There was a problem hiding this comment.
Security Issue: The local buffers ck_b64 and sk_b64 contain base64-encoded sensitive key material (scram_ClientKey is password-equivalent). Leaving these buffers on the stack without clearing them can expose sensitive credentials to memory scraping or other memory-disclosure attacks.
Recommendation: Explicitly clear ck_b64 and sk_b64 using memset before exiting the block.
char ck_b64[64] = { 0 };
char sk_b64[64] = { 0 };
int n1 = pg_b64_encode((const char*)userinfo->scram_ClientKey,
(int)sizeof(userinfo->scram_ClientKey), ck_b64, (int)sizeof(ck_b64) - 1);
int n2 = pg_b64_encode((const char*)userinfo->scram_ServerKey,
(int)sizeof(userinfo->scram_ServerKey), sk_b64, (int)sizeof(sk_b64) - 1);
if (n1 > 0) ck_b64[n1] = '\0';
if (n2 > 0) sk_b64[n2] = '\0';
append_conninfo_param(conninfo, "scram_client_key", ck_b64);
append_conninfo_param(conninfo, "scram_server_key", sk_b64);
memset(ck_b64, 0, sizeof(ck_b64));
memset(sk_b64, 0, sizeof(sk_b64));![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
include/PgSQL_Connection.h (1)
243-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse snake_case for the new member fields.
Rename the new members to
scram_client_keyandscram_server_key, updating the.cppcall sites at the same time. As per coding guidelines, “Member variables usesnake_case.”♻️ Proposed rename
- uint8_t scram_ClientKey[32]; - uint8_t scram_ServerKey[32]; + uint8_t scram_client_key[32]; + uint8_t scram_server_key[32];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/PgSQL_Connection.h` around lines 243 - 244, The new PgSQL_Connection member fields use mixed-case names instead of the project’s snake_case convention. Rename the fields in PgSQL_Connection to scram_client_key and scram_server_key, and update every corresponding use site in the implementation files that reads or writes these members so the class and its .cpp references stay consistent.Source: Coding guidelines
test/tap/tests/unit/pgsql_reconcile_unit-t.cpp (1)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the returned method on the reject path too.
These lines only verify
rj. A regression that still setsreject=truebut stops returningAM_SCRAMwould pass this test even though the frontend challenge behavior changed.Suggested fix
- pgsql_reconcile_auth_method(3, PT_MD5, &rj); - ok(rj, "md5 secret, scram floor -> REJECT"); + ok(pgsql_reconcile_auth_method(3, PT_MD5, &rj) == AM_SCRAM && rj, + "md5 secret, scram floor -> SCRAM + REJECT");🤖 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/pgsql_reconcile_unit-t.cpp` around lines 31 - 32, The reject-path test for pgsql_reconcile_auth_method only checks the `rj` flag, so it can miss regressions in the returned auth method. Update the assertion near `pgsql_reconcile_auth_method` in `pgsql_reconcile_unit-t` to verify both the reject result and that the returned method is still `AM_SCRAM`, matching the existing non-reject expectations and covering the frontend challenge behavior.test/tap/tests/pgsql-verifier_auth-t.cpp (1)
79-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the same client-visible failure for anti-enumeration.
The linked requirement is that unknown users, wrong passwords, and floor/secret mismatches look identical to clients. These checks only ban two substrings, so a different user-specific error would still pass. Reuse the wrong-password failure as the baseline and compare the unknown-user and md5-under-SCRAM-floor paths against that same visible error surface.
Also applies to: 94-101, 111-121
🤖 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/pgsql-verifier_auth-t.cpp` around lines 79 - 82, The anti-enumeration checks in the pgsql verifier auth tests currently only exclude a couple of substrings, so they may still allow user-specific failures to differ. In the verifier test cases around frontendConn and PQstatus, reuse the wrong-password case as the baseline client-visible failure, then assert the unknown-user path and the md5-under-SCRAM-floor path produce the same visible error surface as that baseline rather than just banning specific substrings.
🤖 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/PgSQL_Connection.cpp`:
- Around line 50-52: Replace the destructor’s plain memset-based clears for
scram_ClientKey and scram_ServerKey with a non-elidable secure wipe such as
explicit_bzero() or the project’s equivalent secure-zero helper. Update the
cleanup logic in the PgSQL_Connection destructor so the SCRAM key material is
always overwritten in a way the compiler cannot optimize away.
In `@lib/PgSQL_Protocol.cpp`:
- Around line 1002-1013: The handshake logic in
PgSQL_Protocol::generate_pkt_initial_handshake and the later password-packet
handling are using different sources of truth for auth selection, which can flip
a login into the mock-fail path if pgsql_thread___authentication_method changes
mid-flight. Stop recomputing mock from the live floor in the password handling
block and instead derive it from the stored handshake decision in
(*myds)->auth_method, or persist the reject bit when the initial handshake is
chosen so both halves use the same auth contract.
In `@test/tap/tests/pgsql-verifier_auth-t.cpp`:
- Around line 43-56: The helper functions addUser(), delUser(), and setFloor()
are swallowing execAdmin() failures, which hides setup errors and makes later
auth checks misleading. Update these test helpers so each admin command result
is checked and failures are surfaced immediately, either by returning a checked
status from addUser()/setFloor() and handling it at the call sites, or by
calling BAIL_OUT with a clear message from addUser(), delUser(), and setFloor()
when execAdmin() fails. Also update the call sites that invoke addUser() to stop
ignoring its return value.
In `@test/tap/tests/pgsql-verifier_passthrough-t.cpp`:
- Around line 61-67: The negative passthrough test in select1ThroughProxySQL is
currently too broad because it treats any connection failure as a pass, which
can hide frontend SCRAM auth regressions. Update the logic so the connection
step explicitly asserts PQstatus(c.get()) == CONNECTION_OK, then separately
assert that execScalar(c.get(), "SELECT 1") fails for the negative case; apply
the same split to the related assertions around the other referenced checks so
frontend-auth success and backend-query failure are validated independently.
- Around line 100-102: The negative passthrough test is allowing an empty
verifier when PQencryptPasswordConn fails, which lets the assertion pass without
actually testing the salt-mismatch path. Update the pgsql-verifier_passthrough-t
test around the PQencryptPasswordConn call and storeUser helper usage to fail
closed unless a SCRAM verifier is returned, so the test aborts or explicitly
fails when mismatch generation returns nullptr instead of falling back to an
empty string.
---
Nitpick comments:
In `@include/PgSQL_Connection.h`:
- Around line 243-244: The new PgSQL_Connection member fields use mixed-case
names instead of the project’s snake_case convention. Rename the fields in
PgSQL_Connection to scram_client_key and scram_server_key, and update every
corresponding use site in the implementation files that reads or writes these
members so the class and its .cpp references stay consistent.
In `@test/tap/tests/pgsql-verifier_auth-t.cpp`:
- Around line 79-82: The anti-enumeration checks in the pgsql verifier auth
tests currently only exclude a couple of substrings, so they may still allow
user-specific failures to differ. In the verifier test cases around frontendConn
and PQstatus, reuse the wrong-password case as the baseline client-visible
failure, then assert the unknown-user path and the md5-under-SCRAM-floor path
produce the same visible error surface as that baseline rather than just banning
specific substrings.
In `@test/tap/tests/unit/pgsql_reconcile_unit-t.cpp`:
- Around line 31-32: The reject-path test for pgsql_reconcile_auth_method only
checks the `rj` flag, so it can miss regressions in the returned auth method.
Update the assertion near `pgsql_reconcile_auth_method` in
`pgsql_reconcile_unit-t` to verify both the reject result and that the returned
method is still `AM_SCRAM`, matching the existing non-reject expectations and
covering the frontend challenge behavior.
🪄 Autofix (Beta)
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
Run ID: 15a25310-334d-4fd9-bc3a-e54b4c48819e
📒 Files selected for processing (11)
deps/Makefiledeps/postgresql/scram_verifier_auth.patchinclude/PgSQL_Connection.hinclude/PgSQL_Protocol.hlib/PgSQL_Authentication.cpplib/PgSQL_Connection.cpplib/PgSQL_Protocol.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-verifier_auth-t.cpptest/tap/tests/pgsql-verifier_passthrough-t.cpptest/tap/tests/unit/pgsql_reconcile_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: run / trigger
⚠️ CI failures not shown inline (2)
GitHub Actions: CI-lint-groups-json / lint: SCRAM verifier & md5 credential storage with SCRAM/md5 backend pass-through - PostgreSQL
Conclusion: failure
##[group]Run python3 test/tap/groups/lint_groups_json.py
�[36;1mpython3 test/tap/groups/lint_groups_json.py�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
groups.json format lint: 1 error(s) found:
Keys not sorted: 'basic-t' should come before 'pgsql_reconcile_unit-t'
Hint: run 'python3 /home/runner/work/proxysql/proxysql/test/tap/groups/lint_groups_json.py --fix' to auto-fix
##[error]Process completed with exit code 1.
GitHub Actions: CI-lint-groups-json / 0_lint.txt: SCRAM verifier & md5 credential storage with SCRAM/md5 backend pass-through - PostgreSQL
Conclusion: failure
##[group]Run python3 test/tap/groups/lint_groups_json.py
�[36;1mpython3 test/tap/groups/lint_groups_json.py�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
groups.json format lint: 1 error(s) found:
Keys not sorted: 'basic-t' should come before 'pgsql_reconcile_unit-t'
Hint: run 'python3 /home/runner/work/proxysql/proxysql/test/tap/groups/lint_groups_json.py --fix' to auto-fix
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers are controlled via flags:PROXYSQL31=1for v3.1.x features (FFTO, TSDB),PROXYSQL40=1for v4.0.x features (plugin loader).PROXYSQL40=1implies bothPROXYSQL31=1andPROXYSQLFFTO=1andPROXYSQLTSDB=1. Use conditional compilation with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Class names usePascalCasewith protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session).
Member variables usesnake_case.
Constants and macros useUPPER_SNAKE_CASE.
Use C++17; conditional compilation for feature tiers via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Use RAII for resource management; use jemalloc for memory allocation.
Use pthread mutexes for synchronization; usestd::atomic<>for counters.
Files:
include/PgSQL_Connection.hinclude/PgSQL_Protocol.hlib/PgSQL_Authentication.cpptest/tap/tests/unit/pgsql_reconcile_unit-t.cpplib/PgSQL_Connection.cpptest/tap/tests/pgsql-verifier_passthrough-t.cpptest/tap/tests/pgsql-verifier_auth-t.cpplib/PgSQL_Protocol.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in headers use
#ifndef __CLASS_*_Hformat (e.g.,#ifndef __MYSQL_PROTOCOL_H).
Files:
include/PgSQL_Connection.hinclude/PgSQL_Protocol.h
{lib,src}/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
GenAI/MCP/RAG/LLM features live entirely in
plugins/genai/and load as a.soat runtime viadlopen. Do not guard withPROXYSQLGENAIin core code — that flag no longer guards any core code as of Step 7 of the GenAI plugin carve-out.
Files:
lib/PgSQL_Authentication.cpplib/PgSQL_Connection.cpplib/PgSQL_Protocol.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/. Test binaries are built via pattern rulemake <testname>-twhich compiles<testname>-t.cppinto<testname>-t. Register new tests ingroups.json.
Files:
test/tap/tests/unit/pgsql_reconcile_unit-t.cpptest/tap/tests/pgsql-verifier_passthrough-t.cpptest/tap/tests/pgsql-verifier_auth-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must use
test_globals.handtest_init.hand link againstlibproxysql.avia the custom test harness defined indoc/agents/project-conventions.md.
Files:
test/tap/tests/unit/pgsql_reconcile_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/pgsql_reconcile_unit-t.cpptest/tap/tests/pgsql-verifier_passthrough-t.cpptest/tap/tests/pgsql-verifier_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/pgsql_reconcile_unit-t.cpp
🪛 ast-grep (0.44.0)
lib/PgSQL_Protocol.cpp
[error] 1050-1050: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(&md5_string[i * 2], "%02x", (unsigned int)md5_digest[i])
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🔇 Additional comments (14)
deps/Makefile (1)
398-398: LGTM!deps/postgresql/scram_verifier_auth.patch (2)
7-84: LGTM!Also applies to: 101-156
166-188: 🎯 Functional CorrectnessThe MD5 bypass belongs in the later MD5 branch, not this guard. This check is for SCRAM (
scram_client_key);md5_secretis handled separately later, so widening this condition here is misplaced.> Likely an incorrect or invalid review comment.lib/PgSQL_Connection.cpp (1)
39-41: LGTM!Also applies to: 133-136, 956-958, 987-1014
lib/PgSQL_Authentication.cpp (1)
11-11: LGTM!Also applies to: 91-98
include/PgSQL_Protocol.h (1)
55-59: LGTM!lib/PgSQL_Protocol.cpp (2)
381-469: LGTM!Also applies to: 931-932, 1075-1075, 1102-1103, 1156-1196
1041-1045: 🩺 Stability & AvailabilityThe MD5 fast-path is length-checked in the classifier.
get_password_type()only returnsPASSWORD_TYPE_MD5for secrets matchingmd5+ 32 hex chars, so shorter or malformed values fall through to the plaintext path and never reach thismemcpy.> Likely an incorrect or invalid review comment.test/tap/tests/unit/pgsql_reconcile_unit-t.cpp (1)
1-30: LGTM!Also applies to: 34-40
test/tap/groups/groups.json (1)
16-16: LGTM!Also applies to: 180-181
test/tap/tests/pgsql-verifier_passthrough-t.cpp (4)
1-39: LGTM!
40-58: LGTM!
70-92: LGTM!
104-111: LGTM!
|
I think this PR would benefit from a few more targeted regression tests around verifier pass-through and lifecycle edge cases. The current tests cover the main SCRAM verifier path, floor reconciliation, anti-enumeration behavior, and malformed verifier rejection, but there are a couple of paths where verifier/hash-only credentials behave differently from plaintext passwords. Suggested coverage, roughly in priority order:
These are not all necessarily blockers, but I would at least cover the backend termination path and the mid-SCRAM credential reload behavior before merging, because both exercise state transitions outside the main happy path. |
…view ask 2) Adds pgsql-scram_reload_midhandshake-t: drives a raw SCRAM-SHA-256 handshake stepwise, rotates pgsql_users.password + LOAD PGSQL USERS TO RUNTIME between server-first and client-final, then sends the client-final computed for the ORIGINAL verifier. libpq cannot express this (it drives SASL atomically). Brings the SCRAM/MD5-capable pg_lite_client from test/pgsql-protocol-testing-design (wholesale copy of the two files -- strictly additive: getLastAuthType, MD5/SASL handleAuthentication branches, doSASLAuth) and adds a stepwise wrapper on top: rawConnectStartup() + saslBegin() (client-first -> server-first) + saslFinish() (client-final -> AuthenticationOk / clean-reject sentinel). The library stays decoupled from TAP (no diag()). Every Makefile consumer of pg_lite_client.cpp now links -lscram -lusual (relinked+verified) since the shared source uses libscram unconditionally. Maintainer decision (2026-07-11): PIN OBSERVED BEHAVIOR. The test asserts the observed contract rather than choosing one, and reports it via diag for blessing. Either outcome (bound-to-original OR fail-closed) is a PASS; a hang/crash/desync/ unusable session would be a FINDING. OBSERVED (legacy-g4 / docker-pgsql16-single, PR #5865 head): contract (A) bound-to-original -- the client-final for verifier A is ACCEPTED after the runtime verifier is rotated to B (LOAD completes before client-final is sent), ReadyForQuery is received (session in sync), and a fresh login with the current verifier B still succeeds (ProxySQL healthy, no lasting damage). Deterministic by construction (no race). 3/3 PASS.
…_auth-t (#5865 review ask 6, test isolation) pgsql-verifier_auth-t.cpp mutated the pgsql-authentication_method floor across several scenarios and restored it to a hardcoded "3" instead of whatever value the suite actually started with, risking a silent floor change leaking into sibling legacy-g4 tests if the suite default ever differs from 3. Snapshot the original value from runtime_global_variables right after connecting, BAIL_OUT if the read comes back empty, and restore that exact value at every restore point (including a final unconditional restore before cleanup). No assertions changed; all 11 existing assertions still pass.
Regression tests for the review asks — results (tests in #5932)I implemented the six regression tests requested in the review, built against this PR's head and its patched libpq. Summary: 4 of the credential paths hold up cleanly; 2 have real, reproducible bugs that should be fixed before merge. Full tests are in #5932 (targeted at this branch so they ride in with the code). 🔴 Bug 1 — backend termination fails for verifier/md5-stored users (
|
PgSQL_backend_kill_thread()'s TERMINATE_CONNECTION path built its conninfo with an unconditional password=<stored secret>, so for a user stored as a SCRAM verifier or an md5 hash it shipped the literal "SCRAM-SHA-256$..."/"md5..." text as if it were a plaintext password. libpq applies no prefix detection to 'password' — it runs SASLprep+PBKDF2 over whatever it is handed — so the auxiliary connection failed to authenticate and pg_terminate_backend never ran. With pgsql-kill_backend_connection_when_disconnect=true (the default), dropping a frontend left the backend query running, failing silently with only a proxy_error in the log. connect_start() already selected the mechanism matching the stored secret's type; the kill path never learned it. Extract that selection into append_conninfo_credentials() and call it from both paths, so the pooled and auxiliary backend connections cannot drift apart again — mirroring the branching would have left the same failure mode open to a third caller. PgSQL_Backend_Kill_Args now takes the PgSQL_Connection_userinfo and deep-copies the harvested ClientKey/ServerKey, since the kill runs on a detached thread that outlives the source connection; the copies are wiped with OPENSSL_cleanse on destruction, as PgSQL_Connection_userinfo does. CANCEL_QUERY is unaffected — it reuses PQgetCancel() on the already-authenticated connection — and the plaintext path is unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/PgSQL_Connection.h`:
- Around line 243-248: Update the SCRAM credential state associated with
scram_client_key, scram_server_key, and has_scram_keys to include a credential
generation or fingerprint used during pool matching. During credential reload,
advance or replace that identifier and evict idle pooled backends whose stored
identifier no longer matches, while preserving reuse for connections using the
current credentials.
🪄 Autofix (Beta)
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
Run ID: 1745b49a-24b7-434b-a582-6435447eaefa
📒 Files selected for processing (4)
include/PgSQL_Connection.hlib/PgSQL_Connection.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Session.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/PgSQL_HostGroups_Manager.cpplib/PgSQL_Session.cppinclude/PgSQL_Connection.hlib/PgSQL_Connection.cpp
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/PgSQL_Connection.h
🔇 Additional comments (5)
include/PgSQL_Connection.h (1)
227-229: LGTM!Also applies to: 733-739, 756-759
lib/PgSQL_Connection.cpp (2)
6-6: LGTM!Also applies to: 40-42, 51-54, 135-138, 958-961, 1033-1034, 3048-3064, 3108-3111, 3152-3154
986-1001: 🔒 Security & PrivacyAvoid suggesting
PQconnectdbParamshere libpq copies the provided values intoPGconneither way, so parameter arrays do not remove the secret-lifetime issue. The current stack buffers are already cleansed; the remainingconninfo/PGconnstorage is inherent to this connection flow.> Likely an incorrect or invalid review comment.lib/PgSQL_HostGroups_Manager.cpp (1)
2567-2567: LGTM!lib/PgSQL_Session.cpp (1)
1266-1267: LGTM!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tap/tests/pgsql-verifier_backend_kill-t.cpp (1)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
storeUser()idempotent (DELETE-then-INSERT) for test isolation. Both helpersINSERTintopgsql_userswithout a precedingDELETE. A stale row left by a prior crashed run would make theINSERTfail (onlydiag'd, not fatal), soLOAD PGSQL USERS TO RUNTIMEkeeps the old verifier and the SCRAM/md5 assertions run against stale credentials.pgsql-md5_passthrough-t.cppalready uses the safe DELETE-then-INSERT pattern.
test/tap/tests/pgsql-verifier_backend_kill-t.cpp#L63-L67: prepend aDELETE FROM pgsql_users WHERE username='<user>'before theINSERTinstoreUser().test/tap/tests/pgsql-verifier_pool_rotation-t.cpp#L63-L67: apply the same DELETE-then-INSERT change instoreUser().🤖 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/pgsql-verifier_backend_kill-t.cpp` around lines 63 - 67, Make storeUser() idempotent in both test/tap/tests/pgsql-verifier_backend_kill-t.cpp:63-67 and test/tap/tests/pgsql-verifier_pool_rotation-t.cpp:63-67 by executing DELETE FROM pgsql_users for the target username before the existing INSERT, then retain the LOAD PGSQL USERS TO RUNTIME step.
🤖 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.
Nitpick comments:
In `@test/tap/tests/pgsql-verifier_backend_kill-t.cpp`:
- Around line 63-67: Make storeUser() idempotent in both
test/tap/tests/pgsql-verifier_backend_kill-t.cpp:63-67 and
test/tap/tests/pgsql-verifier_pool_rotation-t.cpp:63-67 by executing DELETE FROM
pgsql_users for the target username before the existing INSERT, then retain the
LOAD PGSQL USERS TO RUNTIME step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8359b39-c283-4874-8a60-89146fd01ef5
📒 Files selected for processing (12)
test/infra/docker-pgsql16-single/bin/docker-pgsql-post.bashtest/infra/docker-pgsql16-single/conf/pgsql/pgsql1/pg_hba.conftest/tap/groups/groups.jsontest/tap/tests/Makefiletest/tap/tests/pg_lite_client.cpptest/tap/tests/pg_lite_client.htest/tap/tests/pgsql-libpq_scram_params-t.cpptest/tap/tests/pgsql-md5_passthrough-t.cpptest/tap/tests/pgsql-scram_reload_midhandshake-t.cpptest/tap/tests/pgsql-verifier_auth-t.cpptest/tap/tests/pgsql-verifier_backend_kill-t.cpptest/tap/tests/pgsql-verifier_pool_rotation-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tap/tests/pgsql-verifier_auth-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/pgsql-scram_reload_midhandshake-t.cpptest/tap/tests/pgsql-verifier_pool_rotation-t.cpptest/tap/tests/pgsql-md5_passthrough-t.cpptest/tap/tests/pgsql-libpq_scram_params-t.cpptest/tap/tests/pgsql-verifier_backend_kill-t.cpptest/tap/tests/pg_lite_client.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
test/tap/tests/pgsql-scram_reload_midhandshake-t.cpptest/tap/tests/pgsql-verifier_pool_rotation-t.cpptest/tap/tests/pgsql-md5_passthrough-t.cpptest/tap/tests/pgsql-libpq_scram_params-t.cpptest/tap/tests/pg_lite_client.htest/tap/tests/pgsql-verifier_backend_kill-t.cpptest/tap/tests/pg_lite_client.cpp
🧠 Learnings (1)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/pgsql-scram_reload_midhandshake-t.cpptest/tap/tests/pgsql-verifier_pool_rotation-t.cpptest/tap/tests/pgsql-md5_passthrough-t.cpptest/tap/tests/pgsql-libpq_scram_params-t.cpptest/tap/tests/pgsql-verifier_backend_kill-t.cpptest/tap/tests/pg_lite_client.cpp
🪛 ast-grep (0.44.1)
test/tap/tests/pg_lite_client.cpp
[warning] 374-374: This hashing algorithm is insecure. If this hash is used in a security context, such as password hashing, it should be converted to a stronger hashing algorithm.
Context: MD5(reinterpret_cast<const unsigned char*>(in.data()), in.size(), digest);
Note: [CWE-328] Use of Weak Hash.
(insecure-hash-cpp)
🔇 Additional comments (13)
test/tap/tests/pg_lite_client.cpp (3)
373-384: The static-analysis weak-hash warning onMD5(...)is a false positive here: PostgreSQL'smd5authentication method ("md5" + md5(md5(password+user)+salt)) is protocol-mandated, not a security choice. No change needed.
415-506: LGTM!
508-679: LGTM!test/tap/tests/Makefile (1)
366-384: LGTM!test/tap/tests/pg_lite_client.h (1)
145-162: LGTM!Also applies to: 228-246
test/tap/groups/groups.json (1)
156-157: LGTM!Also applies to: 181-181, 195-197
test/tap/tests/pgsql-md5_passthrough-t.cpp (1)
58-141: LGTM!test/tap/tests/pgsql-libpq_scram_params-t.cpp (1)
95-260: LGTM!test/tap/tests/pgsql-scram_reload_midhandshake-t.cpp (1)
70-162: LGTM!test/tap/tests/pgsql-verifier_backend_kill-t.cpp (1)
117-219: LGTM!test/tap/tests/pgsql-verifier_pool_rotation-t.cpp (1)
133-216: LGTM!test/infra/docker-pgsql16-single/bin/docker-pgsql-post.bash (1)
28-40: LGTM!test/infra/docker-pgsql16-single/conf/pgsql/pgsql1/pg_hba.conf (1)
19-24: LGTM!
PR sysown#5865 already adds pgsql-verifier_auth-t / -verifier_passthrough-t / pgsql_reconcile_unit-t covering the credential-storage x floor matrix, anti-enumeration, and backend pass-through via libpq (connect success/fail only, no queries, no wire challenge-type assertion). Reposition SP-1's auth-matrix test as the wire-level complement: assert the actual auth CHALLENGE type ProxySQL presents per floor (3/5/10) via a new pg_lite_client getLastAuthType() accessor, plus run a query to prove the session is usable. De-dup the storage-type/floor success/fail (owned by sysown#5865). Add merge-order note and an optional sysown#5865-gated no-downgrade wire assertion. MD5/SCRAM enabler tasks unchanged.
Wire-level complement to PR sysown#5865: assert the auth CHALLENGE type ProxySQL presents to the client (cleartext=3) and run a query to prove the session is usable — neither observable through libpq. Adds PgConnection::getLastAuthType() to pg_lite_client and the test scaffold (2 assertions); md5/scram land in the next tasks.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| int floor = pgsql_thread___authentication_method; | ||
| AUTHENTICATION_METHOD selected = (AUTHENTICATION_METHOD)floor; | ||
| { | ||
| const char* user = (const char*)(*myds)->myconn->conn_params.get_value(PG_USER); | ||
| if (user && *user) { | ||
| bool _ssl = false, _tp = true, _ff = false; int _hg = -1, _mc = 0; void* _sha = NULL; char* _attr = NULL; | ||
| // Same credential scope as the response-time lookup in process_handshake_response() | ||
| // (#5987): ADMIN/STATS resolve against the Admin scope, everything else against | ||
| // USERNAME_FRONTEND. The two lookups must agree or the challenge method and the | ||
| // verification would be chosen from different credentials. | ||
| char* stored = GloPgAuth->lookup((char*)user, cred_scope_for_session((*myds)->sess->session_type), | ||
| &_ssl, &_hg, &_tp, &_ff, &_mc, &_sha, &_attr); | ||
| if (stored) { | ||
| bool reject = false; // on reject we still challenge with the floor method; the response handler mocks | ||
| selected = (AUTHENTICATION_METHOD) pgsql_reconcile_auth_method( |
There was a problem hiding this comment.
💡 Security: Challenge method leaks stored credential type / user existence
generate_pkt_initial_handshake() now selects the auth challenge (cleartext/md5/SCRAM) from the connecting user's stored secret type, while an unknown user falls back to the configured floor. When the floor is below a known user's stored strength (e.g. floor=cleartext with a SCRAM-verifier or md5 user), that user is challenged with SCRAM/md5 whereas a non-existent user is challenged with the floor method. An attacker can therefore enumerate valid usernames and distinguish credential types purely from the challenge type, before sending any password — partially undermining the PR's stated 'same handshake shape' anti-enumeration goal. This is fully mitigated only when the floor is set to scram (uniform SCRAM challenge for all). This appears intentional per the PR's behavior matrix, so flagging as minor: consider documenting the enumeration exposure of sub-scram floors, or always issuing the floor-method challenge for the initial packet and reconciling at response time.
Was this helpful? React with 👍 / 👎
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5865 +/- ##
==========================================
+ Coverage 60.76% 62.10% +1.33%
==========================================
Files 613 623 +10
Lines 175976 177991 +2015
Branches 44535 45010 +475
==========================================
+ Hits 106936 110539 +3603
+ Misses 47580 45457 -2123
- Partials 21460 21995 +535
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:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 24 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/PgSQL_Authentication.cpp">
<violation number="1" location="lib/PgSQL_Authentication.cpp:118">
P2: A SCRAM verifier with iteration count `0` passes this guard because `get_password_type` does not enforce the positive iteration bound. Reject non-positive (and out-of-range) iteration counts before storing the credential, otherwise malformed verifiers are admitted as SCRAM and can fail later during authentication.</violation>
</file>
<file name="lib/PgSQL_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Protocol.cpp:1175">
P1: When a SCRAM verifier changes from A to B during an in-flight exchange, this condition labels A's `scram_state` keys as belonging to B. A frontend login can therefore succeed, but backend pass-through sends A's keys to a backend using B and rejects the first backend connection; bind the exchange to its original verifier and harvest only matching keys, or fail the handshake on a mismatch.</violation>
</file>
<file name="lib/PgSQL_Connection.cpp">
<violation number="1" location="lib/PgSQL_Connection.cpp:988">
P2: SCRAM pass-through leaves password-equivalent keys in ordinary `std::string` heap buffers after this helper returns. Wipe the complete conninfo temporaries after `PQconnectStart()`, or avoid serializing key material through `std::ostringstream` and `std::string`.</violation>
</file>
<file name="test/tap/tests/pgsql-libpq_scram_params-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-libpq_scram_params-t.cpp:151">
P3: The `derivedServerMatchesStored` sanity result is computed and only emitted via `diag()`, never turned into an `ok()` assertion. If the derived ServerKey diverged from the verifier's stored key, the test would still silently proceed (and the cause would only show up as a downstream failure in test (1)), rather than pinpointing the derivation as the failing leg. Assert the cross-check explicitly so a mismatch is diagnosed at the right step.</violation>
</file>
<file name="test/tap/tests/pgsql-verifier_backend_kill-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-verifier_backend_kill-t.cpp:189">
P2: The md5 kill section never lowers the frontend auth-method floor, so on any md5-capable infrastructure this assertion fails instead of exercising the kill path. The stored md5 secret is reconciled via pgsql_reconcile_auth_method (lib/PgSQL_Protocol.cpp:393-404): with the default pgsql-authentication_method=3 (SCRAM), an md5 secret sets reject=true and the response handler mocks a generic failure (lines ~1022-1030), so openConn(...kill_md5...) through the ProxySQL frontend cannot reach CONNECTION_OK. start_query_and_drop_frontend then returns an empty pid and ok(terminated) is false. The md5_backend_ok probe only tests a DIRECT backend connection (pg_hba), not the frontend floor, so it gates nothing against this. This is why the companion test pgsql-md5_passthrough-t.cpp explicitly snapshots orig_floor, SET pgsql-authentication_method='2', and restores it. Do the same here (snapshot before mutation, restore on every path), otherwise the md5 kill path is either skipped in scram-only infra or falsely red in md5-capable infra.</violation>
</file>
<file name="deps/postgresql/scram_verifier_auth.patch">
<violation number="1" location="deps/postgresql/scram_verifier_auth.patch:113">
P3: These new error strings bypass libpq's `libpq_gettext()` localization used by every surrounding error message (`scram_ServerKey`, `could not encrypt password`, etc.). Wrap them with `libpq_gettext()` so they are translated consistently with the rest of fe-auth-scram.c / fe-auth.c.</violation>
</file>
<file name="include/PgSQL_Connection.h">
<violation number="1" location="include/PgSQL_Connection.h:248">
P2: The new SCRAM-key state (has_scram_keys plus the key arrays) is only reset in the constructor and the copy-override set(PgSQL_Connection_userinfo*). The base four-argument set(char*, char*, char*, char*) — which the copy-override internally calls and which is the only other credential-mutation path — leaves has_scram_keys true and retains the harvested ClientKey (password-equivalent) after the username/password are changed or reset through it. Because these fields were deliberately excluded from compute_hash() and are not reset alongside the credentials, a reused userinfo that once carried keys keeps them until destruction, so connect_start()/kill-path credential selection can emit stale keys for a different credential set. Current PgSQL call sites invoke the four-arg setter only with NULLs (hash recompute), so there is no immediate wrong-login path today; this is a latent robustness/security-hygiene gap in the state added by this PR.</violation>
</file>
<file name="test/tap/tests/pgsql-verifier_passthrough-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-verifier_passthrough-t.cpp:54">
P2: storeUser() ignores the return of both execOk() calls, so a failed INSERT into pgsql_users or a failed LOAD PGSQL USERS TO RUNTIME is silently swallowed. The same-PR sibling test pgsql-verifier_auth-t.cpp's addUser() deliberately BAIL_OUTs on these failures because a swallowed INSERT/LOAD makes the later assertion fail for the wrong reason and stops the test from being diagnostic. Apply the same here (and to the restore DELETE/LOAD at the end) so a setup/teardown failure surfaces as such rather than as a confusing auth failure.</violation>
</file>
<file name="test/tap/tests/pgsql-md5_passthrough-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-md5_passthrough-t.cpp:137">
P2: The final restore in the teardown ignores the return of execOk for both the SET and LOAD that put pgsql-authentication_method back to its original value. If that restore fails, the global floor is left at MD5 and weakens auth for every subsequent pgsql test in the suite, silently. The comment block above takes great care to guard the pre-mutation read (BAIL_OUT if orig_floor cannot be read) specifically to avoid a silently-skipped restore, but the actual restore has no failure handling. Check the return values and emit a WARNING (like pgsql-auth_method_matrix-t.cpp does on restore failure) or BAIL_OUT so a leaked MD5 floor is not silent.</violation>
</file>
<file name="test/tap/tests/unit/pgsql_reconcile_unit-t.cpp">
<violation number="1" location="test/tap/tests/unit/pgsql_reconcile_unit-t.cpp:15">
P3: This unit test duplicates the PasswordType and AUTHENTICATION_METHOD enum values as local anonymous enums tied to nothing at compile time, then passes those magic ints to pgsql_reconcile_auth_method(). If either source enum shifts, the assertions silently compare against stale values with no compile error, causing both false-positives and false-negatives because the function signature is only (int,int,bool*).</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| * backend's rolpassword, so those keys must never be reused on the backend leg. | ||
| * (Gating on scram_state->adhoc is insufficient — it stays false on a plaintext | ||
| * user's 2nd+ login when the verifier cache hits.) */ | ||
| if (password && get_password_type(password) == PASSWORD_TYPE_SCRAM_SHA_256) { |
There was a problem hiding this comment.
P1: When a SCRAM verifier changes from A to B during an in-flight exchange, this condition labels A's scram_state keys as belonging to B. A frontend login can therefore succeed, but backend pass-through sends A's keys to a backend using B and rejects the first backend connection; bind the exchange to its original verifier and harvest only matching keys, or fail the handshake on a mismatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_Protocol.cpp, line 1175:
<comment>When a SCRAM verifier changes from A to B during an in-flight exchange, this condition labels A's `scram_state` keys as belonging to B. A frontend login can therefore succeed, but backend pass-through sends A's keys to a backend using B and rejects the first backend connection; bind the exchange to its original verifier and harvest only matching keys, or fail the handshake on a mismatch.</comment>
<file context>
@@ -1091,15 +1165,21 @@ EXECUTION_STATE PgSQL_Protocol::process_handshake_response_packet(unsigned char*
+ * backend's rolpassword, so those keys must never be reused on the backend leg.
+ * (Gating on scram_state->adhoc is insufficient — it stays false on a plaintext
+ * user's 2nd+ login when the verifier cache hits.) */
+ if (password && get_password_type(password) == PASSWORD_TYPE_SCRAM_SHA_256) {
+ memcpy(userinfo->scram_client_key,
(*myds)->scram_state->ClientKey,
</file context>
| // mistyped verifier is never silently stored as a literal plaintext password. (md5 follows the | ||
| // PostgreSQL convention: "md5"+32hex is md5, anything else is plaintext — so no md5 rejection here.) | ||
| if (password && strncmp(password, "SCRAM-SHA-256$", 14) == 0 | ||
| && get_password_type(password) != PASSWORD_TYPE_SCRAM_SHA_256) { |
There was a problem hiding this comment.
P2: A SCRAM verifier with iteration count 0 passes this guard because get_password_type does not enforce the positive iteration bound. Reject non-positive (and out-of-range) iteration counts before storing the credential, otherwise malformed verifiers are admitted as SCRAM and can fail later during authentication.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_Authentication.cpp, line 118:
<comment>A SCRAM verifier with iteration count `0` passes this guard because `get_password_type` does not enforce the positive iteration bound. Reject non-positive (and out-of-range) iteration counts before storing the credential, otherwise malformed verifiers are admitted as SCRAM and can fail later during authentication.</comment>
<file context>
@@ -110,6 +111,14 @@ creds_group_t& PgSQL_Authentication::creds_for(enum cred_username_type usertype)
+ // mistyped verifier is never silently stored as a literal plaintext password. (md5 follows the
+ // PostgreSQL convention: "md5"+32hex is md5, anything else is plaintext — so no md5 rejection here.)
+ if (password && strncmp(password, "SCRAM-SHA-256$", 14) == 0
+ && get_password_type(password) != PASSWORD_TYPE_SCRAM_SHA_256) {
+ proxy_error("pgsql_users: user '%s' has a malformed SCRAM-SHA-256 verifier; skipping\n", username);
+ return false;
</file context>
| PQclear(r); | ||
| return v; | ||
| } | ||
| static void storeUser(PGconn* admin, const char* user, const std::string& secret) { |
There was a problem hiding this comment.
P2: storeUser() ignores the return of both execOk() calls, so a failed INSERT into pgsql_users or a failed LOAD PGSQL USERS TO RUNTIME is silently swallowed. The same-PR sibling test pgsql-verifier_auth-t.cpp's addUser() deliberately BAIL_OUTs on these failures because a swallowed INSERT/LOAD makes the later assertion fail for the wrong reason and stops the test from being diagnostic. Apply the same here (and to the restore DELETE/LOAD at the end) so a setup/teardown failure surfaces as such rather than as a confusing auth failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/pgsql-verifier_passthrough-t.cpp, line 54:
<comment>storeUser() ignores the return of both execOk() calls, so a failed INSERT into pgsql_users or a failed LOAD PGSQL USERS TO RUNTIME is silently swallowed. The same-PR sibling test pgsql-verifier_auth-t.cpp's addUser() deliberately BAIL_OUTs on these failures because a swallowed INSERT/LOAD makes the later assertion fail for the wrong reason and stops the test from being diagnostic. Apply the same here (and to the restore DELETE/LOAD at the end) so a setup/teardown failure surfaces as such rather than as a confusing auth failure.</comment>
<file context>
@@ -0,0 +1,128 @@
+ PQclear(r);
+ return v;
+}
+static void storeUser(PGconn* admin, const char* user, const std::string& secret) {
+ execOk(admin, std::string("INSERT INTO pgsql_users (username,password,active,default_hostgroup) "
+ "VALUES ('") + user + "','" + secret + "',1,0)");
</file context>
| execOk(admin.get(), std::string("DELETE FROM pgsql_users WHERE username='") + U + "'"); | ||
| execOk(admin.get(), "LOAD PGSQL USERS TO RUNTIME"); | ||
| if (!orig_floor.empty()) { | ||
| execOk(admin.get(), std::string("SET pgsql-authentication_method='") + orig_floor + "'"); |
There was a problem hiding this comment.
P2: The final restore in the teardown ignores the return of execOk for both the SET and LOAD that put pgsql-authentication_method back to its original value. If that restore fails, the global floor is left at MD5 and weakens auth for every subsequent pgsql test in the suite, silently. The comment block above takes great care to guard the pre-mutation read (BAIL_OUT if orig_floor cannot be read) specifically to avoid a silently-skipped restore, but the actual restore has no failure handling. Check the return values and emit a WARNING (like pgsql-auth_method_matrix-t.cpp does on restore failure) or BAIL_OUT so a leaked MD5 floor is not silent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/pgsql-md5_passthrough-t.cpp, line 137:
<comment>The final restore in the teardown ignores the return of execOk for both the SET and LOAD that put pgsql-authentication_method back to its original value. If that restore fails, the global floor is left at MD5 and weakens auth for every subsequent pgsql test in the suite, silently. The comment block above takes great care to guard the pre-mutation read (BAIL_OUT if orig_floor cannot be read) specifically to avoid a silently-skipped restore, but the actual restore has no failure handling. Check the return values and emit a WARNING (like pgsql-auth_method_matrix-t.cpp does on restore failure) or BAIL_OUT so a leaked MD5 floor is not silent.</comment>
<file context>
@@ -0,0 +1,141 @@
+ execOk(admin.get(), std::string("DELETE FROM pgsql_users WHERE username='") + U + "'");
+ execOk(admin.get(), "LOAD PGSQL USERS TO RUNTIME");
+ if (!orig_floor.empty()) {
+ execOk(admin.get(), std::string("SET pgsql-authentication_method='") + orig_floor + "'");
+ execOk(admin.get(), "LOAD PGSQL VARIABLES TO RUNTIME");
+ }
</file context>
| // reuse semantics are unchanged. | ||
| uint8_t scram_client_key[PGSQL_SCRAM_KEY_LEN]; | ||
| uint8_t scram_server_key[PGSQL_SCRAM_KEY_LEN]; | ||
| bool has_scram_keys; |
There was a problem hiding this comment.
P2: The new SCRAM-key state (has_scram_keys plus the key arrays) is only reset in the constructor and the copy-override set(PgSQL_Connection_userinfo*). The base four-argument set(char*, char*, char*, char*) — which the copy-override internally calls and which is the only other credential-mutation path — leaves has_scram_keys true and retains the harvested ClientKey (password-equivalent) after the username/password are changed or reset through it. Because these fields were deliberately excluded from compute_hash() and are not reset alongside the credentials, a reused userinfo that once carried keys keeps them until destruction, so connect_start()/kill-path credential selection can emit stale keys for a different credential set. Current PgSQL call sites invoke the four-arg setter only with NULLs (hash recompute), so there is no immediate wrong-login path today; this is a latent robustness/security-hygiene gap in the state added by this PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At include/PgSQL_Connection.h, line 248:
<comment>The new SCRAM-key state (has_scram_keys plus the key arrays) is only reset in the constructor and the copy-override set(PgSQL_Connection_userinfo*). The base four-argument set(char*, char*, char*, char*) — which the copy-override internally calls and which is the only other credential-mutation path — leaves has_scram_keys true and retains the harvested ClientKey (password-equivalent) after the username/password are changed or reset through it. Because these fields were deliberately excluded from compute_hash() and are not reset alongside the credentials, a reused userinfo that once carried keys keeps them until destruction, so connect_start()/kill-path credential selection can emit stale keys for a different credential set. Current PgSQL call sites invoke the four-arg setter only with NULLs (hash recompute), so there is no immediate wrong-login path today; this is a latent robustness/security-hygiene gap in the state added by this PR.</comment>
<file context>
@@ -237,7 +240,12 @@ class PgSQL_Connection_userinfo {
+ // reuse semantics are unchanged.
+ uint8_t scram_client_key[PGSQL_SCRAM_KEY_LEN];
+ uint8_t scram_server_key[PGSQL_SCRAM_KEY_LEN];
+ bool has_scram_keys;
PgSQL_Connection_userinfo();
~PgSQL_Connection_userinfo();
</file context>
| } | ||
| ck_b64 = b64key(ClientKey); | ||
| sk_b64 = b64key(ServerKey); | ||
| // sanity: our derived ServerKey must equal the verifier's stored ServerKey. |
There was a problem hiding this comment.
P3: The derivedServerMatchesStored sanity result is computed and only emitted via diag(), never turned into an ok() assertion. If the derived ServerKey diverged from the verifier's stored key, the test would still silently proceed (and the cause would only show up as a downstream failure in test (1)), rather than pinpointing the derivation as the failing leg. Assert the cross-check explicitly so a mismatch is diagnosed at the right step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/pgsql-libpq_scram_params-t.cpp, line 151:
<comment>The `derivedServerMatchesStored` sanity result is computed and only emitted via `diag()`, never turned into an `ok()` assertion. If the derived ServerKey diverged from the verifier's stored key, the test would still silently proceed (and the cause would only show up as a downstream failure in test (1)), rather than pinpointing the derivation as the failing leg. Assert the cross-check explicitly so a mismatch is diagnosed at the right step.</comment>
<file context>
@@ -0,0 +1,260 @@
+ }
+ ck_b64 = b64key(ClientKey);
+ sk_b64 = b64key(ServerKey);
+ // sanity: our derived ServerKey must equal the verifier's stored ServerKey.
+ derivedServerMatchesStored = (sk_b64 == storedServerb64) ? "yes" : "NO";
+ if (sk_b64 != storedServerb64) {
</file context>
| + | ||
| + if (dec != state->key_length) | ||
| + { | ||
| + *errstr = "invalid scram_client_key"; |
There was a problem hiding this comment.
P3: These new error strings bypass libpq's libpq_gettext() localization used by every surrounding error message (scram_ServerKey, could not encrypt password, etc.). Wrap them with libpq_gettext() so they are translated consistently with the rest of fe-auth-scram.c / fe-auth.c.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deps/postgresql/scram_verifier_auth.patch, line 113:
<comment>These new error strings bypass libpq's `libpq_gettext()` localization used by every surrounding error message (`scram_ServerKey`, `could not encrypt password`, etc.). Wrap them with `libpq_gettext()` so they are translated consistently with the rest of fe-auth-scram.c / fe-auth.c.</comment>
<file context>
@@ -0,0 +1,205 @@
++
++ if (dec != state->key_length)
++ {
++ *errstr = "invalid scram_client_key";
++ pg_hmac_free(ctx);
++ return false;
</file context>
| + *errstr = "invalid scram_client_key"; | |
| *errstr = libpq_gettext("invalid scram_client_key"); |
| int pgsql_reconcile_auth_method(int floor, int stored, bool* reject); | ||
|
|
||
| // libscram PasswordType values (deps/libscram/include/scram.h): | ||
| enum { PT_PLAINTEXT = 0, PT_MD5 = 1, PT_SCRAM = 2 }; |
There was a problem hiding this comment.
P3: This unit test duplicates the PasswordType and AUTHENTICATION_METHOD enum values as local anonymous enums tied to nothing at compile time, then passes those magic ints to pgsql_reconcile_auth_method(). If either source enum shifts, the assertions silently compare against stale values with no compile error, causing both false-positives and false-negatives because the function signature is only (int,int,bool*).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/pgsql_reconcile_unit-t.cpp, line 15:
<comment>This unit test duplicates the PasswordType and AUTHENTICATION_METHOD enum values as local anonymous enums tied to nothing at compile time, then passes those magic ints to pgsql_reconcile_auth_method(). If either source enum shifts, the assertions silently compare against stale values with no compile error, causing both false-positives and false-negatives because the function signature is only (int,int,bool*).</comment>
<file context>
@@ -0,0 +1,39 @@
+int pgsql_reconcile_auth_method(int floor, int stored, bool* reject);
+
+// libscram PasswordType values (deps/libscram/include/scram.h):
+enum { PT_PLAINTEXT = 0, PT_MD5 = 1, PT_SCRAM = 2 };
+// AUTHENTICATION_METHOD values (include/PgSQL_Thread.h):
+enum { AM_CLEARTEXT = 1, AM_MD5 = 2, AM_SCRAM = 3 };
</file context>
Signed-off-by: René Cannaò <rene@proxysql.com>
append_conninfo_credentials() logged an error and returned when it could not build a credential, and both callers connected anyway. A conninfo with no credential is not inert: libpq falls back to PGPASSWORD and then ~/.pgpass from the ProxySQL process environment, so the backend leg could authenticate as whoever owns the host instead of the configured user. password='' does not fix it -- libpq still reads the password file when the password is empty (fe-connect.c:1261) -- so the helper now reports whether a credential was emitted and both callers abandon the attempt on false. connect_start() fails with SQLSTATE 28000 through the existing ASYNC_CONNECT_FAILED path; the kill thread skips the terminate. Also closes two arms with the same hole: a NULL password (an empty string is still sent as password='', which trust-auth backends need), and a base64 encode failure that emitted scram_client_key='' -- which libpq treats as absent.
A SCRAM login takes two round trips. ProxySQL looks the user up again on the second one, but checks the client's proof against the data it prepared during the first. If the user is deleted in between -- a LOAD PGSQL USERS TO RUNTIME while a client pauses between the two messages -- the lookup finds nothing while the proof still verifies against the old data. ProxySQL treats the login as successful and then stores the user's password, which no longer exists, crashing the whole process on a NULL pointer and taking down every other session with it. A client can trigger this deliberately. The fix refuses the login when the proof verified but the credential is gone: it logs an error and returns the same generic "Access denied" a wrong password gets, so a deleted user stays indistinguishable from a bad one. The check deliberately keys on "proof verified" rather than "no password found", because an unknown username also arrives with no password -- that is the normal path for a login attempt with a made-up user, and it must keep failing the ordinary way instead of logging an error every time. Adds pgsql-scram_user_removed_midhandshake-t to cover the crash and the normal logins the check must not break, plus pgsql-scram_rotate_midhandshake_backend-t, which checks the related case of a password being changed mid-handshake and finds nothing wrong. Also fixes pgsql-scram_reload_midhandshake-t, which treated a dropped connection as a clean rejection and so passed on exactly the failures it was written to detect.
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsAdds PostgreSQL SCRAM verifier and md5 credential storage with backend auth pass-through, alongside unit tests for auth reconciliation and handshake safety. Consider addressing the minor challenge method leak where the initial handshake reveals the stored credential type. 💡 Security: Challenge method leaks stored credential type / user existence📄 lib/PgSQL_Protocol.cpp:428-442 📄 lib/PgSQL_Protocol.cpp:452-466 generate_pkt_initial_handshake() now selects the auth challenge (cleartext/md5/SCRAM) from the connecting user's stored secret type, while an unknown user falls back to the configured floor. When the floor is below a known user's stored strength (e.g. floor=cleartext with a SCRAM-verifier or md5 user), that user is challenged with SCRAM/md5 whereas a non-existent user is challenged with the floor method. An attacker can therefore enumerate valid usernames and distinguish credential types purely from the challenge type, before sending any password — partially undermining the PR's stated 'same handshake shape' anti-enumeration goal. This is fully mitigated only when the floor is set to scram (uniform SCRAM challenge for all). This appears intentional per the PR's behavior matrix, so flagging as minor: consider documenting the enumeration exposure of sub-scram floors, or always issuing the floor-method challenge for the initial packet and reconciling at response time. 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 2 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
|
auth_type was parsed into the Config and then ignored: the converter never read it and check_unmappable() never mentioned it. The frontend authentication method of the pooler being replaced simply vanished from the import, with no error even in strict mode -- which contradicts the converter's own strict-by-default contract, and does so for the most security-relevant setting in pgbouncer.ini. It maps cleanly onto the existing pgsql-authentication_method variable (PgSQL_Thread.cpp, range 1-3): plain / password -> 1 (cleartext) md5 -> 2 scram-sha-256 -> 3 The values ProxySQL cannot express are now reported (fatal in strict mode): trust and any (ProxySQL always verifies the user against pgsql_users), hba (pgsql-authentication_method is global, so per-rule pg_hba.conf methods cannot select the frontend method), and cert / pam. Also records in doc/PGBOUNCER_COMPAT.md that the pre-hashed-password limitation is expected to lift with #5865 / #5863, which teaches pgsql_users.password to hold a SCRAM verifier or md5 hash directly -- the same formats userlist.txt already stores -- together with the constraints that come with it (md5 secret needs an md5 backend, verifier needs a scram-sha-256 backend and must be byte-identical to the backend's rolpassword). Converter unit tests: 52 -> 68.
2 participants
Problem
ProxySQL stores PostgreSQL user passwords as plaintext in
pgsql_users.password.That has two costs: cleartext secrets at rest in the admin DB and in memory, and a
PBKDF2 (4096 HMAC-SHA256 iterations) run for every SCRAM client — paid again per
backend connection, since libpq re-derives the keys from the plaintext on each new
server connection.
PostgreSQL itself never stores the password; it stores a verifier in
pg_authid.rolpassword:SCRAM-SHA-256$<iterations>:<salt>$<StoredKey>:<ServerKey>What this PR does
Lets
pgsql_users.passwordhold the backend's actual secret — a SCRAM verifier, anmd5 hash, or (as today) plaintext — auto-detected by prefix. Clients are then
verified with no per-auth PBKDF2, and backends are authenticated by reusing key
material instead of re-deriving it from a plaintext password.
pgsql-authentication_method:1=cleartext,2=md5,3=scram) selects the challenge method from the stored secrettype. A SCRAM verifier is parsed directly (no PBKDF2); a plaintext user pays PBKDF2 once
per thread (cached thereafter).
ClientKeyas a by-product of the proof check, and replays it (with the verifier'sServerKey) on the backend leg via a bundled-libpq patch — authenticating to the backendwith no plaintext and no PBKDF2.
md5…hash is reused directly on both legs.identically (same handshake shape, one generic error; real reason logged server-side only).
Behavior matrices
Frontend (client → ProxySQL) — challenge method / outcome:
Backend (ProxySQL → PostgreSQL) — depends only on the stored type:
md5_secretA session succeeds only if both legs in its row succeed.
libpq patch (
deps/postgresql/scram_verifier_auth.patch)New conninfo params
scram_client_key/scram_server_key/md5_secret(markedsensitive). In
fe-auth-scram.c: when a ClientKey is injected, skip SASLprep + PBKDF2 andderive
StoredKey = SHA256(ClientKey); verify the backend'sServerSignatureagainst theinjected ServerKey. Fail-closed fences: ClientKey and ServerKey must both be present or
both absent; invalid/short keys are rejected. Injected key material is
explicit_bzero'd infreePGconn.Preconditions & limitations
backend's
rolpassword(same salt and iterations), and the connecting user to map to thesame backend user (no user remapping). All backends a user reaches in a hostgroup must
share the same verifier — automatic for physical/streaming replicas; for logical
replication / independent clusters, pin one identical verifier or use per-hostgroup
credentials.
Closes #5863
Summary by CodeRabbit