Add SSL/TLS traffic decryption for PostgreSQL backend connections#5567
Conversation
) Extends the existing NSS keylog-based SSL decryption to PostgreSQL backend connections by patching libpq with a global PQsetSSLKeyLogCallback() API. When admin-ssl_keylog_file is configured, TLS secrets from all PgSQL backend SSL handshakes are written to the keylog file in NSS Key Log Format, enabling traffic decryption with Wireshark/tshark. Approach: - Patch libpq with global callback following the PQsslKeyPassHook pattern - Single startup call covers all backend paths (regular, monitor, kill, harvester) - No new admin variables — existing admin-ssl_keylog_file covers PgSQL backends - Zero changes to ProxySQL PgSQL connection code Includes 9 integration tests (pgsql-ssl_keylog-t) and user guide updates.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR enables NSS-style SSL/TLS key logging for PostgreSQL backend connections by patching libpq to expose a global keylog callback, wiring that callback into ProxySQL's keylog initialization, updating docs for PostgreSQL scenarios, and adding integration tests that validate behavior across connection types and edge cases. Changes
Sequence DiagramsequenceDiagram
participant Init as ProxySQL Init
participant libpq as libpq
participant OpenSSL as OpenSSL
participant PG as PostgreSQL
Init->>Init: process_opts_pre()
Init->>Init: proxysql_keylog_init()
Init->>libpq: PQsetSSLKeyLogCallback(proxysql_keylog_write_line_callback)
libpq->>libpq: store global callback
Init->>PG: open backend SSL connection
PG->>libpq: SSL handshake
libpq->>OpenSSL: SSL_CTX_set_keylog_callback(callback)
OpenSSL->>libpq: invoke keylog callback with secret line
libpq->>Init: proxysql_keylog_write_line_callback(line)
Init->>Init: write NSS KeyLog formatted line to file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 SSL key logging support for PostgreSQL backend connections in ProxySQL. This involves patching the libpq library to provide a global keylog callback, integrating this callback into ProxySQL's existing keylog writing functionality, and updating the user guide to reflect the new feature. A new integration test suite has also been added to validate the PostgreSQL SSL keylog behavior. A minor issue was noted in the user guide regarding incorrect numbering in the "Solutions" section.
| ``` | ||
| 2. Make sure clients are connecting with SSL/TLS | ||
| 4. Make sure clients are connecting with SSL/TLS | ||
| 3. Check that `admin-ssl_keylog_file` is loaded into runtime: |
There was a problem hiding this comment.
The numbering in the "Solutions" list is incorrect. It currently goes from 4 back to 3. This should be updated to 5 to maintain the correct sequence.
| 3. Check that `admin-ssl_keylog_file` is loaded into runtime: | |
| 5. Check that `admin-ssl_keylog_file` is loaded into runtime: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doc/ssl_keylog/ssl_keylog_user_guide.md (1)
427-435:⚠️ Potential issue | 🟡 MinorFix list item numbering inconsistency.
The troubleshooting steps have inconsistent numbering. After adding steps 2-4 for PostgreSQL verification, line 431 starts with "4." and line 432 reuses "3." The markdown renderer may handle this, but the source should be consistent.
📝 Suggested fix for list numbering
-3. Keylog entries are only written during **new** SSL handshakes. Existing pooled connections won't generate entries. To force new handshakes, reload servers: +5. Keylog entries are only written during **new** SSL handshakes. Existing pooled connections won't generate entries. To force new handshakes, reload servers: ```sql LOAD PGSQL SERVERS TO RUNTIME; ``` -4. Make sure clients are connecting with SSL/TLS -3. Check that `admin-ssl_keylog_file` is loaded into runtime: +6. Make sure clients are connecting with SSL/TLS +7. Check that `admin-ssl_keylog_file` is loaded into runtime:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/ssl_keylog/ssl_keylog_user_guide.md` around lines 427 - 435, The ordered list in ssl_keylog_user_guide.md is inconsistent: after the "LOAD PGSQL SERVERS TO RUNTIME;" item the next lines use "4." then "3." — renumber the troubleshooting steps so they are sequential (e.g., continue 4., 5., 6.) and ensure the items referencing "Make sure clients are connecting with SSL/TLS" and "Check that `admin-ssl_keylog_file` is loaded into runtime (LOAD ADMIN VARIABLES TO RUNTIME;)" follow the correct order; update the numbering for those list entries and any adjacent items so the markdown source uses a consistent ascending sequence.
🧹 Nitpick comments (2)
deps/postgresql/sslkeylogfile.patch (2)
17-19: Cast hides type mismatch between callback signatures.The callback typedef uses
const void *ssl(line 57) to avoid exposing OpenSSL types in the public header, butSSL_CTX_set_keylog_callbackexpectsvoid (*)(const SSL*, const char*). The cast on line 19 works but silently accepts any function pointer. Consider adding a static assertion or compile-time check that the callback signature matches expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/postgresql/sslkeylogfile.patch` around lines 17 - 19, The direct cast from PQsslKeyLogCB to the type expected by SSL_CTX_set_keylog_callback hides a signature mismatch; replace the blind cast by either (a) adding a compile-time check using a static assertion (e.g. using _Static_assert or __builtin_types_compatible_p) that verifies PQsslKeyLogCB has the signature void (*)(const SSL*, const char*), or (b) implement a thin wrapper function with the exact signature void wrapper(const SSL *ssl, const char *line) that calls PQsslKeyLogCB(ssl, line) after casting ssl to const void*, then pass that wrapper to SSL_CTX_set_keylog_callback(SSL_context, wrapper); reference PQsslKeyLogCB, SSL_CTX_set_keylog_callback and SSL_context when applying the change.
34-44: Global callback is set without synchronization.
PQsslKeyLogCBis accessed without thread synchronization. While the PR description states this is called once during single-threaded startup (matchingPQsslKeyPassHookpattern), a brief comment in the implementation noting this assumption would help maintainers understand the thread-safety contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/postgresql/sslkeylogfile.patch` around lines 34 - 44, PQsslKeyLogCB is accessed without synchronization; follow the existing PQsslKeyPassHook pattern by documenting the thread-safety contract instead of adding locks: update the implementation of PQsetSSLKeyLogCallback, PQgetSSLKeyLogCallback (and near the PQsslKeyLogCB declaration) to include a short comment stating that the global callback is expected to be set once during single-threaded startup and is intentionally not synchronized, so callers must not install/change the callback after threads are running; reference the symbols PQsslKeyLogCB, PQsetSSLKeyLogCallback, and PQgetSSLKeyLogCallback in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/pgsql-ssl_keylog-t.cpp`:
- Around line 102-108: The SQL built in set_keylog_file uses the raw path and
can break if the path contains a single quote; change set_keylog_file to escape
the path via libpq (use PQescapeLiteral or equivalent) before concatenating into
the SQL string, handle the NULL case by passing 'NULL' (or empty literal)
appropriately, and then call admin_exec with the escaped string; reference
set_keylog_file, PGconn*, and admin_exec so you update that function to call
PQescapeLiteral(admin, path, strlen(path)) (and free the returned string)
instead of appending path directly.
---
Outside diff comments:
In `@doc/ssl_keylog/ssl_keylog_user_guide.md`:
- Around line 427-435: The ordered list in ssl_keylog_user_guide.md is
inconsistent: after the "LOAD PGSQL SERVERS TO RUNTIME;" item the next lines use
"4." then "3." — renumber the troubleshooting steps so they are sequential
(e.g., continue 4., 5., 6.) and ensure the items referencing "Make sure clients
are connecting with SSL/TLS" and "Check that `admin-ssl_keylog_file` is loaded
into runtime (LOAD ADMIN VARIABLES TO RUNTIME;)" follow the correct order;
update the numbering for those list entries and any adjacent items so the
markdown source uses a consistent ascending sequence.
---
Nitpick comments:
In `@deps/postgresql/sslkeylogfile.patch`:
- Around line 17-19: The direct cast from PQsslKeyLogCB to the type expected by
SSL_CTX_set_keylog_callback hides a signature mismatch; replace the blind cast
by either (a) adding a compile-time check using a static assertion (e.g. using
_Static_assert or __builtin_types_compatible_p) that verifies PQsslKeyLogCB has
the signature void (*)(const SSL*, const char*), or (b) implement a thin wrapper
function with the exact signature void wrapper(const SSL *ssl, const char *line)
that calls PQsslKeyLogCB(ssl, line) after casting ssl to const void*, then pass
that wrapper to SSL_CTX_set_keylog_callback(SSL_context, wrapper); reference
PQsslKeyLogCB, SSL_CTX_set_keylog_callback and SSL_context when applying the
change.
- Around line 34-44: PQsslKeyLogCB is accessed without synchronization; follow
the existing PQsslKeyPassHook pattern by documenting the thread-safety contract
instead of adding locks: update the implementation of PQsetSSLKeyLogCallback,
PQgetSSLKeyLogCallback (and near the PQsslKeyLogCB declaration) to include a
short comment stating that the global callback is expected to be set once during
single-threaded startup and is intentionally not synchronized, so callers must
not install/change the callback after threads are running; reference the symbols
PQsslKeyLogCB, PQsetSSLKeyLogCallback, and PQgetSSLKeyLogCallback in the
comment.
🪄 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: bacd1dfe-7eb3-49c3-a9be-4fc3c9e417c1
📒 Files selected for processing (9)
deps/Makefiledeps/postgresql/sslkeylogfile.patchdoc/ssl_keylog/ssl_keylog_user_guide.mdinclude/proxysql_sslkeylog.hlib/ProxySQL_GloVars.cpplib/proxysql_sslkeylog.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-ssl_keylog-t.cpptest/tap/tests/unit/genai_query_handler_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via#ifdefPROXYSQLGENAI,#ifdefPROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
test/tap/tests/unit/genai_query_handler_unit-t.cpplib/ProxySQL_GloVars.cpplib/proxysql_sslkeylog.cppinclude/proxysql_sslkeylog.htest/tap/tests/pgsql-ssl_keylog-t.cpp
**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Use RAII for resource management and jemalloc for memory allocation
Files:
test/tap/tests/unit/genai_query_handler_unit-t.cpplib/ProxySQL_GloVars.cpplib/proxysql_sslkeylog.cpptest/tap/tests/pgsql-ssl_keylog-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Files:
test/tap/tests/unit/genai_query_handler_unit-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file is the typical convention
Files:
lib/ProxySQL_GloVars.cpplib/proxysql_sslkeylog.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards must follow the pattern
#ifndef_CLASS*_H
Files:
include/proxysql_sslkeylog.h
test/tap/tests/{test_*,*-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Files:
test/tap/tests/pgsql-ssl_keylog-t.cpp
🧠 Learnings (11)
📚 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/genai_query_handler_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
test/tap/tests/unit/genai_query_handler_unit-t.cpplib/proxysql_sslkeylog.cpptest/tap/tests/pgsql-ssl_keylog-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/tests/unit/genai_query_handler_unit-t.cpptest/tap/tests/pgsql-ssl_keylog-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
test/tap/tests/unit/genai_query_handler_unit-t.cpp
📚 Learning: 2026-03-22T14:38:16.093Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/tap/tests/unit/genai_query_handler_unit-t.cpptest/tap/tests/pgsql-ssl_keylog-t.cpp
📚 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/genai_query_handler_unit-t.cpptest/tap/tests/pgsql-ssl_keylog-t.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/tests/unit/genai_query_handler_unit-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's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
lib/ProxySQL_GloVars.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-ssl_keylog-t.cpp
📚 Learning: 2026-02-13T05:55:42.693Z
Learnt from: mevishalr
Repo: sysown/proxysql PR: 5364
File: lib/MySQL_Logger.cpp:1211-1232
Timestamp: 2026-02-13T05:55:42.693Z
Learning: In ProxySQL, the MySQL_Logger and PgSQL_Logger destructors run after all worker threads have been joined during shutdown. The sequence in src/main.cpp is: (1) join all worker threads, (2) call ProxySQL_Main_shutdown_all_modules() which deletes the loggers. Therefore, there is no concurrent thread access during logger destruction, and lock ordering in the destructors cannot cause deadlocks.
Applied to files:
lib/ProxySQL_GloVars.cppinclude/proxysql_sslkeylog.h
📚 Learning: 2026-03-26T16:39:02.446Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.
Applied to files:
test/tap/groups/groups.json
📚 Learning: 2026-01-20T09:34:27.165Z
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
doc/ssl_keylog/ssl_keylog_user_guide.md
🪛 Clang (14.0.6)
test/tap/tests/unit/genai_query_handler_unit-t.cpp
[error] 17-17: 'tap.h' file not found
(clang-diagnostic-error)
test/tap/tests/pgsql-ssl_keylog-t.cpp
[error] 28-28: 'unistd.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (17)
test/tap/tests/unit/genai_query_handler_unit-t.cpp (1)
17-18: LGTM - correct placement of tap.h include.Moving
tap.houtside the#ifdef PROXYSQLGENAIblock is correct because the#elsebranch (lines 408-412) also uses TAP macros (plan,ok,exit_status). The static analysis hint about'tap.h' file not foundis a false positive from missing include paths in the analyzer configuration.deps/Makefile (1)
373-373: LGTM - patch integration follows existing conventions.The
sslkeylogfile.patchis correctly applied after extraction and beforeconfigure, using the same-p0flag as other libpq patches.test/tap/groups/groups.json (1)
148-148: LGTM - test group mapping follows existing PGSQL conventions.The new
pgsql-ssl_keylog-ttest is correctly assigned to the same group configuration as other PostgreSQL tests in thelegacy-g4category.include/proxysql_sslkeylog.h (1)
106-119: LGTM - well-documented API declaration.The function declaration and documentation clearly describe the purpose, timing requirements (after
proxysql_keylog_init()), and thread-safety assumptions. The@seereference toPQsetSSLKeyLogCallbackhelps developers understand the underlying mechanism.lib/proxysql_sslkeylog.cpp (2)
26-26: Unconditional libpq include may break MySQL-only builds.This
#include "libpq-fe.h"is unconditional. If ProxySQL supports builds without PostgreSQL/libpq, this will cause compilation failures. Consider guarding with a preprocessor check if PostgreSQL support is optional.This is related to the concern raised in
lib/ProxySQL_GloVars.cpp. If libpq is always a required dependency, this is fine.
246-248: LGTM - callback registration implementation.The cast from
proxysql_keylog_write_line_callback(which takesconst SSL*) toPQsslKeyLogCallback_type(which takesconst void*) is safe because the callback ignores the first parameter anyway (line 174:(void)ssl;). The function correctly integrates with the patched libpq API.doc/ssl_keylog/ssl_keylog_user_guide.md (1)
497-512: LGTM - comprehensive connection type documentation.The new "Supported Connection Types" section clearly documents that a single
admin-ssl_keylog_filevariable enables TLS key logging for all connection types across both MySQL and PostgreSQL protocols, which aligns well with the PR implementation.lib/ProxySQL_GloVars.cpp (1)
490-490: No action needed. libpq is unconditionally built and linked into ProxySQL; there is no MySQL-only build configuration. The call toproxysql_keylog_set_pgsql_callback()on line 490 is safe and will always resolve correctly.test/tap/tests/pgsql-ssl_keylog-t.cpp (9)
28-48: LGTM: Includes and setup are appropriate.The
PGConnPtrsmart pointer alias provides proper RAII for PostgreSQL connections. Theunistd.hstatic analysis warning is a false positive—this is a standard POSIX header available in the build environment.
53-97: LGTM: Helper functions are well-implemented.Proper use of RAII for connection management, correct
PGresultcleanup withPQclear(), and appropriate error diagnostics.
113-191: LGTM: Connection and utility helpers are well-designed.Good RAII usage for connection helpers, and file helpers correctly distinguish between non-existent files (returning -1) and empty files (returning 0).
262-303: LGTM: Concurrent test properly validates thread safety.Good use of
std::atomic<int>for the connection counter, proper thread joining before result inspection, and comprehensive post-validation that all keylog lines match the NSS format regex.
245-260: LGTM: NSS Key Log Format regex patterns are correct.The regex correctly validates the format: label, 64 hex characters (32-byte client random), and variable-length secret. The TLS label check covers both TLS 1.2 (
CLIENT_RANDOM) and TLS 1.3 traffic secret labels.
193-210: LGTM: Test initialization and admin connection setup.Proper use of
BAIL_OUTfor unrecoverable failure, and saving the original keylog setting for cleanup is good practice.
360-390: LGTM: Monitor SSL keylog test has appropriate timing logic.The wait calculation using
2× monitor_intervalwith a 10-second cap (line 379) is a reasonable approach for integration testing monitor-triggered connections.
392-424: LGTM: Non-SSL test properly isolates by disabling monitor.Disabling
pgsql-monitor_enabledbefore the non-SSL test (lines 397-398) ensures the test only measures keylog entries from explicit non-SSL connections, avoiding false positives from background monitor activity.
446-457: LGTM: Cleanup restores original state.Good practice to restore the original
admin-ssl_keylog_filevalue and remove the test file.
| static bool set_keylog_file(PGconn* admin, const char* path) { | ||
| std::string sql = "SET admin-ssl_keylog_file='"; | ||
| if (path) sql += path; | ||
| sql += "'"; | ||
| if (!admin_exec(admin, sql.c_str())) return false; | ||
| return admin_exec(admin, "LOAD ADMIN VARIABLES TO RUNTIME"); | ||
| } |
There was a problem hiding this comment.
Unescaped path in SQL construction could cause test failures.
If original_keylog (restored at line 451) contains a single quote character, the SQL statement becomes malformed. While unlikely in practice, this could cause confusing test failures.
🛡️ Suggested fix using PQescapeLiteral
static bool set_keylog_file(PGconn* admin, const char* path) {
- std::string sql = "SET admin-ssl_keylog_file='";
- if (path) sql += path;
- sql += "'";
+ std::string sql = "SET admin-ssl_keylog_file=";
+ if (path && path[0] != '\0') {
+ char* escaped = PQescapeLiteral(admin, path, strlen(path));
+ if (escaped) {
+ sql += escaped;
+ PQfreemem(escaped);
+ } else {
+ sql += "''";
+ }
+ } else {
+ sql += "''";
+ }
if (!admin_exec(admin, sql.c_str())) return false;
return admin_exec(admin, "LOAD ADMIN VARIABLES TO RUNTIME");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/pgsql-ssl_keylog-t.cpp` around lines 102 - 108, The SQL built
in set_keylog_file uses the raw path and can break if the path contains a single
quote; change set_keylog_file to escape the path via libpq (use PQescapeLiteral
or equivalent) before concatenating into the SQL string, handle the NULL case by
passing 'NULL' (or empty literal) appropriately, and then call admin_exec with
the escaped string; reference set_keylog_file, PGconn*, and admin_exec so you
update that function to call PQescapeLiteral(admin, path, strlen(path)) (and
free the returned string) instead of appending path directly.
|
2 participants
Summary
Extends ProxySQL's existing NSS keylog-based SSL decryption support to PostgreSQL backend connections (ProxySQL → PostgreSQL server). When
admin-ssl_keylog_fileisconfigured, TLS secrets from PgSQL backend SSL handshakes are now captured in NSS Key Log Format, enabling traffic decryption with Wireshark/tshark.
Approach
libpq Patch
Patches libpq (PostgreSQL 16.10) with a global
PQsetSSLKeyLogCallback()API, following the same pattern as libpq's existingPQsslKeyPassHook. The patch modifiestwo libpq files:
libpq-fe.h— AddsPQsslKeyLogCallback_typetypedef,PQsetSSLKeyLogCallback()andPQgetSSLKeyLogCallback()fe-secure-openssl.c— Adds static callback variable, hooks intoinitialize_SSL()to callSSL_CTX_set_keylog_callback()on every new connection's SSL_CTXProxySQL Integration
A single function
proxysql_keylog_set_pgsql_callback()is called once at startup, registering ProxySQL's keylog writer with libpq. This global callbackautomatically covers all PgSQL backend connection paths:
PgSQL_Connection::connect_start())PgSQL_Connection::kill_connection())PgSQL_Monitor)Query_Tool_Handler)PgSQL_Static_Harvester)No new admin variables are needed — the existing
admin-ssl_keylog_filecovers PgSQL backends automatically. No changes to any PgSQL connection code.Why Global Callback (vs Per-Connection)
Unlike the MySQL backend (which uses per-connection
MARIADB_OPT_SSL_KEYLOG_CALLBACK), libpq requires a global approach because:PQconnectdb()completes the SSL handshake synchronously — no window for per-connection setupPQsslKeyPassHookpatternCloses #5281
Summary by CodeRabbit
New Features
Documentation
Tests