test(pgsql): regression tests for #5865 review asks (SCRAM verifier / md5 credential storage) - #5932
Merged
Merged
Conversation
…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.
📝 WalkthroughWalkthroughThe changes expand PostgreSQL TAP infrastructure with MD5 and SCRAM authentication support, add stepwise SCRAM handling, provision MD5 test credentials, and introduce regression tests for SCRAM parameters, verifier reloads, backend termination, and connection-pool rotation. ChangesPostgreSQL authentication regressions
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Test as pgsql-verifier_pool_rotation-t
participant ProxySQL
participant PostgreSQL
participant Admin as ProxySQL admin
Test->>ProxySQL: authenticate with password A
ProxySQL->>PostgreSQL: establish backend session
Test->>Admin: update verifier to password B and load runtime
Test->>ProxySQL: authenticate with password B
ProxySQL->>PostgreSQL: establish new backend session
Test-->>Test: compare backend identities
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 introduces comprehensive regression tests and infrastructure updates to validate PostgreSQL authentication features, specifically focusing on SCRAM-SHA-256 and MD5 authentication, verifier pass-through, mid-handshake reloads, backend connection termination, and pool isolation across password rotations. It adds several new test files and extends the mock client pg_lite_client to support stepwise SASL and MD5 authentication. The review feedback highlights critical exception-safety issues in doSASLAuth and saslFinish where raw pointers could be leaked if network or protocol exceptions are thrown, suggesting the use of RAII smart pointers to manage these resources.
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.
Comment on lines
+419
to
+506
| void PgConnection::doSASLAuth(const std::string& password, | ||
| const std::vector<uint8_t>& /*mechListMsg*/) { | ||
| ScramState* st = scram_state_init(); | ||
| PgCredentials cred; | ||
| memset(&cred, 0, sizeof(cred)); | ||
| snprintf(cred.name, sizeof(cred.name), "%s", user_.c_str()); | ||
| snprintf(cred.passwd, sizeof(cred.passwd), "%s", password.c_str()); | ||
| cred.has_scram_keys = false; | ||
|
|
||
| char type; | ||
| std::vector<uint8_t> buffer; | ||
| char* client_first = nullptr; | ||
| char* client_final = nullptr; | ||
|
|
||
| // 1) SASLInitialResponse ('p'): mechanism name + Int32 length + client-first-message. | ||
| // libscram's build_client_first_message already includes the "n,,"" GS2 header | ||
| // (it returns "n,,n=,r=<nonce>"), so we send it verbatim. | ||
| client_first = build_client_first_message(st); | ||
| if (!client_first) { free_scram_state(st); throw PgException(std::string("scram client-first: ") + scram_error()); } | ||
| { | ||
| std::vector<uint8_t> pkt; | ||
| writeStringToBuffer(pkt, "SCRAM-SHA-256"); // null-terminated mechanism name | ||
| int32_t clen = htonl((int32_t)strlen(client_first)); | ||
| const uint8_t* cp = reinterpret_cast<const uint8_t*>(&clen); | ||
| pkt.insert(pkt.end(), cp, cp + 4); // Int32 length of client-first | ||
| pkt.insert(pkt.end(), client_first, client_first + strlen(client_first)); | ||
| sendMessage('p', pkt); | ||
| } | ||
|
|
||
| // 2) Expect AuthenticationSASLContinue (authType 11) with the server-first-message. | ||
| readMessage(type, buffer); | ||
| if (type == ERROR_RESPONSE) { | ||
| free(client_first); free_scram_state(st); | ||
| throw PgException("scram: " + extractErrorMessage(buffer)); | ||
| } | ||
| if (type != AUTH_TYPE || buffer.size() < 4 || | ||
| ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 11) { | ||
| free(client_first); free_scram_state(st); | ||
| throw PgException("expected AuthenticationSASLContinue(11)"); | ||
| } | ||
| std::string server_first(reinterpret_cast<const char*>(buffer.data()) + 4, buffer.size() - 4); | ||
| char* server_nonce = nullptr; char* salt = nullptr; int saltlen = 0; int iterations = 0; | ||
| if (!read_server_first_message(st, const_cast<char*>(server_first.c_str()), | ||
| &server_nonce, &salt, &saltlen, &iterations)) { | ||
| free(client_first); free_scram_state(st); | ||
| throw PgException(std::string("scram read server-first: ") + scram_error()); | ||
| } | ||
|
|
||
| // 3) SASLResponse ('p'): client-final-message (with proof derived from plaintext passwd). | ||
| client_final = build_client_final_message(st, &cred, server_nonce, salt, saltlen, iterations); | ||
| free(salt); // read_server_first_message malloc'd salt and handed us ownership; | ||
| salt = nullptr; // build_client_final_message is its only consumer (just read above). | ||
| if (!client_final) { free(client_first); free_scram_state(st); throw PgException(std::string("scram client-final: ") + scram_error()); } | ||
| { | ||
| std::vector<uint8_t> pkt(client_final, client_final + strlen(client_final)); | ||
| sendMessage('p', pkt); | ||
| } | ||
|
|
||
| // 4) Expect AuthenticationSASLFinal (authType 12) with server-final (v=ServerSignature). | ||
| // A wrong password surfaces here as an ErrorResponse instead. | ||
| readMessage(type, buffer); | ||
| if (type == ERROR_RESPONSE) { | ||
| free(client_first); free(client_final); free_scram_state(st); | ||
| throw PgException("scram: " + extractErrorMessage(buffer)); | ||
| } | ||
| if (type != AUTH_TYPE || buffer.size() < 4 || | ||
| ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 12) { | ||
| free(client_first); free(client_final); free_scram_state(st); | ||
| throw PgException("expected AuthenticationSASLFinal(12)"); | ||
| } | ||
| { | ||
| std::string server_final(reinterpret_cast<const char*>(buffer.data()) + 4, buffer.size() - 4); | ||
| char server_sig[256] = {0}; | ||
| if (!read_server_final_message(const_cast<char*>(server_final.c_str()), server_sig) || | ||
| !verify_server_signature(st, &cred, server_sig)) { | ||
| free(client_first); free(client_final); free_scram_state(st); | ||
| throw PgException("scram server signature verification failed"); | ||
| } | ||
| } | ||
| free(client_first); free(client_final); free_scram_state(st); | ||
|
|
||
| // 5) Expect AuthenticationOk (0). | ||
| readMessage(type, buffer); | ||
| if (type == ERROR_RESPONSE) throw PgException("scram: " + extractErrorMessage(buffer)); | ||
| if (type == AUTH_TYPE && buffer.size() >= 4 && | ||
| ntohl(*reinterpret_cast<int32_t*>(buffer.data())) == 0) return; | ||
| throw PgException("scram: no AuthenticationOk after SASLFinal"); | ||
| } |
There was a problem hiding this comment.
The doSASLAuth function is not exception-safe. If sendMessage or readMessage throws a PgException, the allocated ScramState (st), client_first, and client_final will be leaked because they are raw pointers and are not managed by RAII. Using std::unique_ptr with custom deleters ensures that these resources are automatically freed under all execution paths, including when exceptions are thrown.
void PgConnection::doSASLAuth(const std::string& password,
const std::vector<uint8_t>& /*mechListMsg*/) {
std::unique_ptr<ScramState, decltype(&free_scram_state)> st(scram_state_init(), free_scram_state);
if (!st) throw PgException("Failed to initialize scram state");
PgCredentials cred;
memset(&cred, 0, sizeof(cred));
snprintf(cred.name, sizeof(cred.name), "%s", user_.c_str());
snprintf(cred.passwd, sizeof(cred.passwd), "%s", password.c_str());
cred.has_scram_keys = false;
char type;
std::vector<uint8_t> buffer;
std::unique_ptr<char, void(*)(void*)> client_first(nullptr, free);
std::unique_ptr<char, void(*)(void*)> client_final(nullptr, free);
// 1) SASLInitialResponse ('p'): mechanism name + Int32 length + client-first-message.
// libscram's build_client_first_message already includes the "n,,"" GS2 header
// (it returns "n,,n=,r=<nonce>"), so we send it verbatim.
client_first.reset(build_client_first_message(st.get()));
if (!client_first) { throw PgException(std::string("scram client-first: ") + scram_error()); }
{
std::vector<uint8_t> pkt;
writeStringToBuffer(pkt, "SCRAM-SHA-256"); // null-terminated mechanism name
int32_t clen = htonl((int32_t)strlen(client_first.get()));
const uint8_t* cp = reinterpret_cast<const uint8_t*>(&clen);
pkt.insert(pkt.end(), cp, cp + 4); // Int32 length of client-first
pkt.insert(pkt.end(), client_first.get(), client_first.get() + strlen(client_first.get()));
sendMessage('p', pkt);
}
// 2) Expect AuthenticationSASLContinue (authType 11) with the server-first-message.
readMessage(type, buffer);
if (type == ERROR_RESPONSE) {
throw PgException("scram: " + extractErrorMessage(buffer));
}
if (type != AUTH_TYPE || buffer.size() < 4 ||
ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 11) {
throw PgException("expected AuthenticationSASLContinue(11)");
}
std::string server_first(reinterpret_cast<const char*>(buffer.data()) + 4, buffer.size() - 4);
char* server_nonce = nullptr; char* salt = nullptr; int saltlen = 0; int iterations = 0;
if (!read_server_first_message(st.get(), const_cast<char*>(server_first.c_str()),
&server_nonce, &salt, &saltlen, &iterations)) {
throw PgException(std::string("scram read server-first: ") + scram_error());
}
std::unique_ptr<char, void(*)(void*)> salt_guard(salt, free);
// 3) SASLResponse ('p'): client-final-message (with proof derived from plaintext passwd).
client_final.reset(build_client_final_message(st.get(), &cred, server_nonce, salt_guard.get(), saltlen, iterations));
if (!client_final) { throw PgException(std::string("scram client-final: ") + scram_error()); }
{
std::vector<uint8_t> pkt(client_final.get(), client_final.get() + strlen(client_final.get()));
sendMessage('p', pkt);
}
// 4) Expect AuthenticationSASLFinal (authType 12) with server-final (v=ServerSignature).
// A wrong password surfaces here as an ErrorResponse instead.
readMessage(type, buffer);
if (type == ERROR_RESPONSE) {
throw PgException("scram: " + extractErrorMessage(buffer));
}
if (type != AUTH_TYPE || buffer.size() < 4 ||
ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 12) {
throw PgException("expected AuthenticationSASLFinal(12)");
}
{
std::string server_final(reinterpret_cast<const char*>(buffer.data()) + 4, buffer.size() - 4);
char server_sig[256] = {0};
if (!read_server_final_message(const_cast<char*>(server_final.c_str()), server_sig) ||
!verify_server_signature(st.get(), &cred, server_sig)) {
throw PgException("scram server signature verification failed");
}
}
// 5) Expect AuthenticationOk (0).
readMessage(type, buffer);
if (type == ERROR_RESPONSE) throw PgException("scram: " + extractErrorMessage(buffer));
if (type == AUTH_TYPE && buffer.size() >= 4 &&
ntohl(*reinterpret_cast<int32_t*>(buffer.data())) == 0) return;
throw PgException("scram: no AuthenticationOk after SASLFinal");
}
Comment on lines
+615
to
+680
| int PgConnection::saslFinish() { | ||
| if (!sasl_st_) throw PgException("saslFinish: saslBegin() was not called"); | ||
|
|
||
| PgCredentials cred; | ||
| memset(&cred, 0, sizeof(cred)); | ||
| snprintf(cred.name, sizeof(cred.name), "%s", user_.c_str()); | ||
| snprintf(cred.passwd, sizeof(cred.passwd), "%s", sasl_password_.c_str()); | ||
| cred.has_scram_keys = false; | ||
|
|
||
| char type; | ||
| std::vector<uint8_t> buffer; | ||
|
|
||
| // 3) SASLResponse ('p'): client-final-message. | ||
| char* client_final = build_client_final_message(sasl_st_, &cred, sasl_server_nonce_, | ||
| sasl_salt_, sasl_saltlen_, sasl_iterations_); | ||
| if (!client_final) { | ||
| std::string e = scram_error(); freeSaslState(); | ||
| throw PgException(std::string("scram client-final: ") + e); | ||
| } | ||
| { | ||
| std::vector<uint8_t> pkt(client_final, client_final + strlen(client_final)); | ||
| sendMessage('p', pkt); | ||
| } | ||
|
|
||
| // 4) AuthenticationSASLFinal(12) OR a clean ErrorResponse (rejected verifier). | ||
| readMessage(type, buffer); | ||
| if (type == ERROR_RESPONSE) { | ||
| last_auth_type_ = -1; | ||
| last_error_ = extractErrorMessage(buffer); // clean server rejection of the client-final | ||
| free(client_final); freeSaslState(); | ||
| return SASL_FINISH_REJECTED; | ||
| } | ||
| if (type != AUTH_TYPE || buffer.size() < 4 || | ||
| ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 12) { | ||
| free(client_final); freeSaslState(); | ||
| throw PgException("expected AuthenticationSASLFinal(12)"); | ||
| } | ||
| { | ||
| std::string server_final(reinterpret_cast<const char*>(buffer.data()) + 4, buffer.size() - 4); | ||
| char server_sig[256] = {0}; | ||
| if (!read_server_final_message(const_cast<char*>(server_final.c_str()), server_sig) || | ||
| !verify_server_signature(sasl_st_, &cred, server_sig)) { | ||
| free(client_final); freeSaslState(); | ||
| throw PgException("scram server signature verification failed"); | ||
| } | ||
| } | ||
| free(client_final); | ||
|
|
||
| // 5) AuthenticationOk(0). | ||
| readMessage(type, buffer); | ||
| if (type == ERROR_RESPONSE) { | ||
| last_auth_type_ = -1; | ||
| last_error_ = extractErrorMessage(buffer); // rejected between SASLFinal and AuthenticationOk | ||
| freeSaslState(); | ||
| return SASL_FINISH_REJECTED; | ||
| } | ||
| if (type == AUTH_TYPE && buffer.size() >= 4 && | ||
| ntohl(*reinterpret_cast<int32_t*>(buffer.data())) == 0) { | ||
| last_auth_type_ = 0; | ||
| freeSaslState(); | ||
| return 0; | ||
| } | ||
| freeSaslState(); | ||
| throw PgException("scram: no AuthenticationOk after SASLFinal"); | ||
| } | ||
|
|
There was a problem hiding this comment.
The saslFinish function has potential memory leaks. If sendMessage or readMessage throws a PgException, the allocated client_final string is leaked. Wrapping client_final in a std::unique_ptr with a custom deleter ensures it is safely and automatically freed on all execution paths.
int PgConnection::saslFinish() {
if (!sasl_st_) throw PgException("saslFinish: saslBegin() was not called");
PgCredentials cred;
memset(&cred, 0, sizeof(cred));
snprintf(cred.name, sizeof(cred.name), "%s", user_.c_str());
snprintf(cred.passwd, sizeof(cred.passwd), "%s", sasl_password_.c_str());
cred.has_scram_keys = false;
char type;
std::vector<uint8_t> buffer;
// 3) SASLResponse ('p'): client-final-message.
std::unique_ptr<char, void(*)(void*)> client_final(build_client_final_message(sasl_st_, &cred, sasl_server_nonce_,
sasl_salt_, sasl_saltlen_, sasl_iterations_), free);
if (!client_final) {
std::string e = scram_error(); freeSaslState();
throw PgException(std::string("scram client-final: ") + e);
}
{
std::vector<uint8_t> pkt(client_final.get(), client_final.get() + strlen(client_final.get()));
sendMessage('p', pkt);
}
// 4) AuthenticationSASLFinal(12) OR a clean ErrorResponse (rejected verifier).
readMessage(type, buffer);
if (type == ERROR_RESPONSE) {
last_auth_type_ = -1;
last_error_ = extractErrorMessage(buffer); // clean server rejection of the client-final
freeSaslState();
return SASL_FINISH_REJECTED;
}
if (type != AUTH_TYPE || buffer.size() < 4 ||
ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 12) {
freeSaslState();
throw PgException("expected AuthenticationSASLFinal(12)");
}
{
std::string server_final(reinterpret_cast<const char*>(buffer.data()) + 4, buffer.size() - 4);
char server_sig[256] = {0};
if (!read_server_final_message(const_cast<char*>(server_final.c_str()), server_sig) ||
!verify_server_signature(sasl_st_, &cred, server_sig)) {
freeSaslState();
throw PgException("scram server signature verification failed");
}
}
// 5) Expect AuthenticationOk(0).
readMessage(type, buffer);
if (type == ERROR_RESPONSE) {
last_auth_type_ = -1;
last_error_ = extractErrorMessage(buffer); // rejected between SASLFinal and AuthenticationOk
freeSaslState();
return SASL_FINISH_REJECTED;
}
if (type == AUTH_TYPE && buffer.size() >= 4 &&
ntohl(*reinterpret_cast<int32_t*>(buffer.data())) == 0) {
last_auth_type_ = 0;
freeSaslState();
return 0;
}
freeSaslState();
throw PgException("scram: no AuthenticationOk after SASLFinal");
}
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/pg_lite_client.cpp`:
- Around line 603-609: Update the SCRAM flow around saslBegin() and
read_server_first_message() so sasl_server_nonce_ owns a copy of the returned
server nonce rather than referencing the temporary server_first buffer. Ensure
that backing storage remains valid until saslFinish() dereferences
sasl_server_nonce_, while preserving the existing server-first parsing and error
handling.
In `@test/tap/tests/pgsql-scram_reload_midhandshake-t.cpp`:
- Around line 91-135: Ensure the test always emits the assertion counted by
plan(3) when rawConnectStartup or saslBegin throws. Update the PgException catch
block to record a failed server-first assertion before producing the existing
final contract assertion, while preserving the current timeout and
clean-rejection outcome handling.
🪄 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: 3ec04923-50d6-44df-a099-81065f2aa28e
📒 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
📜 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-md5_passthrough-t.cpptest/tap/tests/pgsql-verifier_auth-t.cpptest/tap/tests/pgsql-verifier_backend_kill-t.cpptest/tap/tests/pgsql-scram_reload_midhandshake-t.cpptest/tap/tests/pgsql-libpq_scram_params-t.cpptest/tap/tests/pgsql-verifier_pool_rotation-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-md5_passthrough-t.cpptest/tap/tests/pgsql-verifier_auth-t.cpptest/tap/tests/pgsql-verifier_backend_kill-t.cpptest/tap/tests/pgsql-scram_reload_midhandshake-t.cpptest/tap/tests/pgsql-libpq_scram_params-t.cpptest/tap/tests/pgsql-verifier_pool_rotation-t.cpptest/tap/tests/pg_lite_client.htest/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-md5_passthrough-t.cpptest/tap/tests/pgsql-verifier_auth-t.cpptest/tap/tests/pgsql-verifier_backend_kill-t.cpptest/tap/tests/pgsql-scram_reload_midhandshake-t.cpptest/tap/tests/pgsql-libpq_scram_params-t.cpptest/tap/tests/pgsql-verifier_pool_rotation-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 (15)
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!test/tap/groups/groups.json (1)
156-157: LGTM!Also applies to: 181-181, 195-197
test/tap/tests/Makefile (2)
366-384: LGTM!
380-384: 🩺 Stability & AvailabilityNo extra link flags needed for the other new tests The targets that build
pg_lite_client.cppalready add-lscram -lusual; the other new tests here don’t usepg_lite_client.cppat all.> Likely an incorrect or invalid review comment.test/tap/tests/pgsql-verifier_backend_kill-t.cpp (2)
42-115: LGTM!
117-219: LGTM!test/tap/tests/pgsql-verifier_pool_rotation-t.cpp (2)
42-131: LGTM!
133-216: LGTM!test/tap/tests/pg_lite_client.cpp (2)
373-384: The ast-grep weak-hash warning here is a false positive: MD5 is mandated by the PostgreSQL MD5 authentication protocol ("md5" + md5(md5(password+user)+salt)), so it is protocol conformance, not a security choice.
304-364: LGTM!Also applies to: 419-506
test/tap/tests/pg_lite_client.h (1)
145-162: LGTM!Also applies to: 228-246
test/tap/tests/pgsql-libpq_scram_params-t.cpp (1)
161-260: LGTM!test/tap/tests/pgsql-md5_passthrough-t.cpp (1)
77-141: LGTM!test/tap/tests/pgsql-verifier_auth-t.cpp (1)
84-94: LGTM!Also applies to: 126-127, 184-184, 201-204
Comment on lines
+603
to
+609
| std::string server_first(reinterpret_cast<const char*>(buffer.data()) + 4, buffer.size() - 4); | ||
| if (!read_server_first_message(sasl_st_, const_cast<char*>(server_first.c_str()), | ||
| &sasl_server_nonce_, &sasl_salt_, &sasl_saltlen_, &sasl_iterations_)) { | ||
| std::string e = scram_error(); freeSaslState(); | ||
| throw PgException(std::string("scram read server-first: ") + e); | ||
| } | ||
| return server_first; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate scram sources and inspect how read_server_first_message assigns *server_nonce_p.
files=$(fd -i 'scram\.(c|cpp|h)$')
echo "candidate files:"; echo "$files"
for f in $files; do
echo "==== $f ===="
rg -nP -A45 'read_server_first_message\s*\(' "$f"
doneRepository: sysown/proxysql
Length of output: 3163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '560,660p' deps/libscram/src/scram.c
echo "----"
sed -n '660,740p' deps/libscram/src/scram.c
echo "----"
sed -n '1,180p' deps/libscram/include/scram.hRepository: sysown/proxysql
Length of output: 9810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/tap/tests/pg_lite_client.cpp --view expanded | sed -n '1,220p'
echo "----"
rg -n -A80 -B40 'saslBegin|saslFinish|read_server_first_message|server_nonce_' test/tap/tests/pg_lite_client.cppRepository: sysown/proxysql
Length of output: 18592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A80 -B20 'static char \*read_attr_value|read_attr_value\s*\(' deps/libscram/src/scram.cRepository: sysown/proxysql
Length of output: 13422
Copy the server nonce before storing it for saslFinish(). read_server_first_message() returns a pointer into the server_first buffer, so sasl_server_nonce_ becomes invalid when saslBegin() returns and is later dereferenced in saslFinish(). Duplicate the nonce or keep its backing storage alive across the two phases.
🤖 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/pg_lite_client.cpp` around lines 603 - 609, Update the SCRAM
flow around saslBegin() and read_server_first_message() so sasl_server_nonce_
owns a copy of the returned server nonce rather than referencing the temporary
server_first buffer. Ensure that backing storage remains valid until
saslFinish() dereferences sasl_server_nonce_, while preserving the existing
server-first parsing and error handling.
Comment on lines
+91
to
+135
| try { | ||
| PgConnection c(4000); // 4s read timeout: a hung handshake surfaces as "Read timed out" | ||
| c.rawConnectStartup(cl.pgsql_host, cl.pgsql_port, USER /*db*/, USER); | ||
| std::string server_first = c.saslBegin(USER, PA); // ProxySQL builds this from verifier A | ||
| ok(!server_first.empty(), "server-first received for verifier A (server-first='%s')", | ||
| server_first.c_str()); | ||
|
|
||
| // --- MUTATE runtime creds mid-handshake: rotate the stored verifier A -> B. --- | ||
| setVerifier(admin.get(), USER, vB); | ||
| diag("rotated pgsql_users['%s'] to verifier B + LOAD PGSQL USERS TO RUNTIME (mid-handshake)", USER); | ||
|
|
||
| // --- Send client-final computed for the ORIGINAL verifier A. --- | ||
| int final_type = c.saslFinish(); | ||
| if (final_type == 0) { | ||
| // (A) bound-to-original: client-final for A ACCEPTED despite the rotation to B. | ||
| // Prove the post-auth protocol is in sync (ReadyForQuery) -- i.e. not a desync/unusable | ||
| // session. We deliberately do NOT run a backend query: reload_user is a frontend-only | ||
| // user with no backend role, so a query would fail at the BACKEND for reasons unrelated | ||
| // to the mid-handshake contract. ReadyForQuery from ProxySQL is the correct in-sync proof. | ||
| c.waitForReady(); | ||
| contract_held = true; | ||
| observed = "A: bound-to-original (client-final for A ACCEPTED after reload to B; " | ||
| "ReadyForQuery received, session in sync)"; | ||
| } else { | ||
| // (B) fail-closed: ProxySQL's fresh lookup of verifier B rejected the A-proof cleanly. | ||
| contract_held = true; | ||
| observed = std::string("B: fail-closed (client-final for A REJECTED after reload to B: ") | ||
| + c.getLastError() + ")"; | ||
| } | ||
| } catch (const PgException& e) { | ||
| std::string what = e.what(); | ||
| if (what.find("timed out") != std::string::npos) { | ||
| // Hang: the handshake neither completed nor was rejected -> this is the FINDING. | ||
| contract_held = false; | ||
| observed = std::string("FINDING (hang): the mid-handshake reload left the SASL exchange " | ||
| "stalled -- ") + what; | ||
| } else { | ||
| // A thrown ErrorResponse / peer-close is a clean fail-closed outcome == contract (B). | ||
| // (A libscram server-signature mismatch would also land here; still a rejection, not a | ||
| // success -- the session never becomes usable, so it is NOT the "unusable-after-success" | ||
| // third outcome.) | ||
| contract_held = true; | ||
| observed = std::string("B: fail-closed (handshake threw a clean rejection: ") + what + ")"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Plan/assertion count can mismatch on the exception path.
plan(3) counts the ok() at Line 95 (server-first received), but that call sits inside the try. If rawConnectStartup() or saslBegin() throws, control jumps to the catch (Line 120) which only emits the Line 139 assertion, so the run reports 2 tests against a plan of 3 — a TAP planning failure that masks the recorded observed outcome. Consider emitting assertion 1 on the failure path too (e.g. ok(false, ...) in the catch) or moving it out of the try.
🤖 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-scram_reload_midhandshake-t.cpp` around lines 91 - 135,
Ensure the test always emits the assertion counted by plan(3) when
rawConnectStartup or saslBegin throws. Update the PgException catch block to
record a failed server-first assertion before producing the existing final
contract assertion, while preserving the current timeout and clean-rejection
outcome handling.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the six regression tests requested in the review of #5865 (SCRAM verifier / md5 credential storage + SCRAM/md5 backend pass-through for PostgreSQL). Built and run against #5865's own head (
v3.0_pgsql-auth-5863) and its patched libpq.Intended to land WITH #5865's code fixes — see "Two RED tests" below: two of these tests are deterministic failures that document real bugs in the current #5865 credential-lifecycle handling. They turn green with no test change once those two bugs are fixed. Merging this before the fixes will show
legacy-g4(and itsmysql-*-g4variants) red — that is intentional and is the point.The six tests (one per review ask)
pgsql-verifier_backend_kill-tpgsql-scram_reload_midhandshake-tpgsql-verifier_pool_rotation-tpgsql-md5_passthrough-t(+ md5 infra user)md5_secretbackend pass-through works (first real exercise of the path)pgsql-libpq_scram_params-tpgsql-verifier_auth-t(edit)pgsql-authentication_method, not a hardcoded3Full root-cause + fix-direction analysis for the two bugs is posted as a review comment on #5865.
Notes for review
lib//src//include/changes. Thepg_lite_clienttest helper gains a stepwise SASL client (saslBegin/saslFinish) used only by test 2; all its existing consumers were re-linked with-lscram -lusual(that library is already vendored and used by the proxysql binary).md5user+ a user-scopedmd5line indocker-pgsql16-single'spg_hba.conf, above the scram catch-all (first-match-wins) so every existing scram user is unaffected. Verified: the scram-dependent pgsql tests (pgsql-verifier_auth-t11/11,pgsql-verifier_passthrough-t3/3) still pass against the extended infra.legacy-g4, each backed by a passing control assertion (e.g. the kill test'skill=false → PID survives; the rotation test'sold password rejected+B authenticates) that rules out env/infra artifacts.SIGFPEinPgSQL_Session::writeout()was seen once during off-harness manipulation; it did not recur and involves none of these test files — flagged for awareness, not chased here.Summary by CodeRabbit
Bug Fixes
Tests