feat: Add stats_tls_certificates and stats_global tables for TLS certificate metadata and tracking#5456
Conversation
- Add STATS_SQLITE_TABLE_TLS_CERTIFICATES definition with cert_type, file_path, subject_cn, issuer_cn, serial_number, not_before, not_after, days_until_expiry, sha256_fingerprint, and loaded_at columns - Add TLS tracking fields to GloVars.global: tls_load_count, tls_last_load_timestamp, tls_last_load_ok, tls_cert_file, tls_ca_file, tls_key_file - Update ProxySQL_create_or_load_TLS() to populate tracking fields on success - Register stats_tls_certificates table in Admin_Bootstrap - Add stats___tls_certificates() function that reads certs from disk at query time, extracts metadata (CN, serial, validity dates, SHA-256 fingerprint, days_until_expiry) and populates the table - Add TLS variables to stats___mysql_global(): TLS_Load_Count, TLS_Last_Load_Timestamp, TLS_Last_Load_Result, TLS_Server_Cert_File, TLS_CA_Cert_File, TLS_Key_File - Wire up query detection and refresh in GenericRefreshStatistics() Co-authored-by: renecannao <3645227+renecannao@users.noreply.github.com>
…ASN1_TIME_diff return, refactor dup cert loading, larger CN buffer Co-authored-by: renecannao <3645227+renecannao@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes implement a new runtime-populated Changes
Sequence DiagramsequenceDiagram
actor Client
participant Query as Admin Query
participant Dispatch as GenericRefreshStatistics
participant Stats as stats___tls_certificates()
participant OpenSSL as OpenSSL Helpers
participant DB as stats_tls_certificates Table
Client->>Query: SELECT * FROM stats_tls_certificates
Query->>Dispatch: GenericRefreshStatistics(stats_tls_certificates)
Dispatch->>Stats: stats___tls_certificates()
Stats->>Stats: Lock ssl_mutex
Stats->>Stats: Clear table, snapshot cert paths & timestamp
Stats->>OpenSSL: Load PEM certificates from disk
OpenSSL->>OpenSSL: Parse X509 objects<br/>(CN, serial, dates, fingerprint)
OpenSSL-->>Stats: Extracted metadata
Stats->>DB: INSERT certificate rows<br/>(cert_type, file_path, subject_cn,<br/>issuer_cn, serial, not_before,<br/>not_after, days_until_expiry,<br/>sha256_fingerprint, loaded_at)
Stats->>Stats: Unlock ssl_mutex, commit
DB-->>Client: Return stats_tls_certificates rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (2)
lib/ProxySQL_GloVars.cpp (1)
88-173: Missing destructor cleanup for TLS file path pointers.The destructor frees many
globalstruct members (e.g.,gr_bootstrap_uri,gr_bootstrap_ssl_ca, etc.) but does not free the newly addedtls_cert_file,tls_ca_file, andtls_key_filepointers which are allocated viastrdup()insrc/proxy_tls.cpp.If this is intentional (similar to
checksums_valuesmembers which avoid cleanup due to concurrent access during shutdown), please add a comment explaining the design decision. Otherwise, add cleanup:♻️ Proposed fix to add cleanup
if (global.gr_bootstrap_ssl_mode) { free(global.gr_bootstrap_ssl_mode); global.gr_bootstrap_ssl_mode = nullptr; } + // TLS certificate file paths + if (global.tls_cert_file) { + free(global.tls_cert_file); + global.tls_cert_file = nullptr; + } + if (global.tls_ca_file) { + free(global.tls_ca_file); + global.tls_ca_file = nullptr; + } + if (global.tls_key_file) { + free(global.tls_key_file); + global.tls_key_file = nullptr; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_GloVars.cpp` around lines 88 - 173, The destructor ProxySQL_GlobalVariables::~ProxySQL_GlobalVariables currently frees many global.* strings but omits freeing the TLS pointers (global.tls_cert_file, global.tls_ca_file, global.tls_key_file) that are strdup'd in src/proxy_tls.cpp; either add the same pattern as other globals (if (global.tls_cert_file) { free(global.tls_cert_file); global.tls_cert_file = nullptr; } and likewise for tls_ca_file and tls_key_file) or, if omission is intentional, add a clear comment near the checksums in_shutdown block explaining why these TLS pointers are not freed during shutdown to avoid race/free issues.lib/ProxySQL_Admin_Stats.cpp (1)
2966-3011: Clarify:loaded_atmay be inconsistent with displayed certificate metadata.The implementation reads certificate metadata from disk at query time (which ensures
days_until_expiryis current), but usestls_last_load_timestampforloaded_at. If the certificate file on disk has been updated butPROXYSQL RELOAD TLShasn't been executed yet, the displayed metadata (subject_cn, not_after, fingerprint, etc.) will reflect the new file whileloaded_atstill shows the old load timestamp.Consider either:
- Documenting this behavior (metadata from disk, timestamp from last load)
- Reading from in-memory X509 objects stored during TLS load (if persisted) to ensure consistency
Otherwise the implementation is thread-safe with proper mutex usage and correct memory management.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_Admin_Stats.cpp` around lines 2966 - 3011, The stats___tls_certificates function can show certificate metadata read from disk while using GloVars.global.tls_last_load_timestamp for loaded_at, causing inconsistent displays when files changed but RELOAD TLS wasn't run; fix by either (A) making loaded_at reflect the actual file read time by replacing use of tls_last_load_timestamp with the file's mtime or the time you read the file inside process_cert, or (B) ensuring consistency by reading the in-memory X509 objects and their associated load timestamps held under the ssl_mutex (use the same in-memory structures populated at TLS load rather than re-reading disk) — update stats___tls_certificates and the process_cert logic to obtain both the X509 and the correct loaded_at from the same source (refer to GloVars.global.tls_cert_file/tls_ca_file, GloVars.global.tls_last_load_timestamp, process_cert lambda, and insert_tls_cert_row) or alternatively document the current behavior clearly if you choose not to change code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/proxysql_glovars.hpp`:
- Around line 147-149: The new char* members tls_cert_file, tls_ca_file, and
tls_key_file are strdup()'d elsewhere but not freed in the ProxySQL_GloVars
destructor; add cleanup in the destructor (same pattern as gr_bootstrap_ssl_*
cleanup) to check each pointer, call free() and set the pointer to nullptr for
tls_cert_file, tls_ca_file and tls_key_file to avoid memory leaks (locate the
destructor in ProxySQL_GloVars.cpp and mirror the existing free+nullptr
pattern).
In `@src/proxy_tls.cpp`:
- Around line 439-442: The TLS reload success branch currently increments
GloVars.global.tls_load_count and sets GloVars.global.tls_last_load_timestamp
and GloVars.global.tls_last_load_ok = true, but the failure branches do not set
tls_last_load_ok = false; update each TLS reload failure path to (under the same
ssl_mutex) increment tls_load_count, set tls_last_load_timestamp = time(NULL)
and set tls_last_load_ok = false so TLS_Last_Load_Result correctly reflects
FAILED; look for the reload-related branches that log failures and update those
branches to mirror the success updates (using GloVars.global.tls_load_count,
GloVars.global.tls_last_load_timestamp, and GloVars.global.tls_last_load_ok) and
ensure this is done inside the ssl_mutex-protected section.
---
Nitpick comments:
In `@lib/ProxySQL_Admin_Stats.cpp`:
- Around line 2966-3011: The stats___tls_certificates function can show
certificate metadata read from disk while using
GloVars.global.tls_last_load_timestamp for loaded_at, causing inconsistent
displays when files changed but RELOAD TLS wasn't run; fix by either (A) making
loaded_at reflect the actual file read time by replacing use of
tls_last_load_timestamp with the file's mtime or the time you read the file
inside process_cert, or (B) ensuring consistency by reading the in-memory X509
objects and their associated load timestamps held under the ssl_mutex (use the
same in-memory structures populated at TLS load rather than re-reading disk) —
update stats___tls_certificates and the process_cert logic to obtain both the
X509 and the correct loaded_at from the same source (refer to
GloVars.global.tls_cert_file/tls_ca_file,
GloVars.global.tls_last_load_timestamp, process_cert lambda, and
insert_tls_cert_row) or alternatively document the current behavior clearly if
you choose not to change code.
In `@lib/ProxySQL_GloVars.cpp`:
- Around line 88-173: The destructor
ProxySQL_GlobalVariables::~ProxySQL_GlobalVariables currently frees many
global.* strings but omits freeing the TLS pointers (global.tls_cert_file,
global.tls_ca_file, global.tls_key_file) that are strdup'd in src/proxy_tls.cpp;
either add the same pattern as other globals (if (global.tls_cert_file) {
free(global.tls_cert_file); global.tls_cert_file = nullptr; } and likewise for
tls_ca_file and tls_key_file) or, if omission is intentional, add a clear
comment near the checksums in_shutdown block explaining why these TLS pointers
are not freed during shutdown to avoid race/free issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7547e27b-e848-46ad-83be-ac5073add834
📒 Files selected for processing (8)
include/ProxySQL_Admin_Tables_Definitions.hinclude/proxysql_admin.hinclude/proxysql_glovars.hpplib/Admin_Bootstrap.cpplib/ProxySQL_Admin.cpplib/ProxySQL_Admin_Stats.cpplib/ProxySQL_GloVars.cppsrc/proxy_tls.cpp
| char *tls_cert_file; | ||
| char *tls_ca_file; | ||
| char *tls_key_file; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if the destructor frees the TLS file path pointers
rg -n "tls_cert_file|tls_ca_file|tls_key_file" lib/ProxySQL_GloVars.cppRepository: sysown/proxysql
Length of output: 157
🏁 Script executed:
# Check context around lines 252-254 in lib/ProxySQL_GloVars.cpp
sed -n '240,270p' lib/ProxySQL_GloVars.cppRepository: sysown/proxysql
Length of output: 1820
🏁 Script executed:
# Search for the destructor of ProxySQL_GlobalVariables
rg -n "~ProxySQL_GlobalVariables|ProxySQL_GlobalVariables::~ProxySQL_GlobalVariables" lib/ProxySQL_GloVars.cppRepository: sysown/proxysql
Length of output: 117
🏁 Script executed:
# Read the destructor implementation
sed -n '88,150p' lib/ProxySQL_GloVars.cppRepository: sysown/proxysql
Length of output: 2637
🏁 Script executed:
# Continue reading the destructor to find TLS pointer cleanup
sed -n '150,200p' lib/ProxySQL_GloVars.cppRepository: sysown/proxysql
Length of output: 1400
🏁 Script executed:
# Verify that TLS pointers are allocated with strdup() in proxy_tls.cpp
rg -n "tls_cert_file|tls_ca_file|tls_key_file" src/proxy_tls.cpp | head -20Repository: sysown/proxysql
Length of output: 409
🏁 Script executed:
# Check the context around the TLS allocation code in proxy_tls.cpp
sed -n '400,425p' src/proxy_tls.cppRepository: sysown/proxysql
Length of output: 1629
Add destructor cleanup for TLS file path pointers.
The new char* pointers (tls_cert_file, tls_ca_file, tls_key_file) are allocated via strdup() in src/proxy_tls.cpp (lines 412, 414, 416), but the destructor in lib/ProxySQL_GloVars.cpp does not free them. Add cleanup following the existing pattern used for other dynamically allocated pointers (e.g., gr_bootstrap_ssl_* pointers at lines 144-172):
if (global.tls_cert_file) {
free(global.tls_cert_file);
global.tls_cert_file = nullptr;
}
if (global.tls_ca_file) {
free(global.tls_ca_file);
global.tls_ca_file = nullptr;
}
if (global.tls_key_file) {
free(global.tls_key_file);
global.tls_key_file = nullptr;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/proxysql_glovars.hpp` around lines 147 - 149, The new char* members
tls_cert_file, tls_ca_file, and tls_key_file are strdup()'d elsewhere but not
freed in the ProxySQL_GloVars destructor; add cleanup in the destructor (same
pattern as gr_bootstrap_ssl_* cleanup) to check each pointer, call free() and
set the pointer to nullptr for tls_cert_file, tls_ca_file and tls_key_file to
avoid memory leaks (locate the destructor in ProxySQL_GloVars.cpp and mirror the
existing free+nullptr pattern).
| // Update TLS tracking fields for stats table (under ssl_mutex) | ||
| GloVars.global.tls_load_count++; | ||
| GloVars.global.tls_last_load_timestamp = time(NULL); | ||
| GloVars.global.tls_last_load_ok = true; |
There was a problem hiding this comment.
Missing tls_last_load_ok = false on reload failure paths.
When TLS reload fails (lines 444-472), tls_last_load_ok is not updated to false. Per PR objectives, TLS_Last_Load_Result should reflect NONE/SUCCESS/FAILED. After a failed reload, the stats will still show "SUCCESS" from the previous bootstrap, which is misleading.
Consider updating tls_last_load_ok = false on failure paths to accurately reflect the last reload attempt status:
🐛 Proposed fix for reload failure tracking
} else {
proxy_error("Failed to load location of CA certificates for verification\n");
msg = "Unable to load CA certificates location for verification";
+ std::lock_guard<std::mutex> lock(GloVars.global.ssl_mutex);
+ GloVars.global.tls_last_load_ok = false;
ret = 1;
}
} else {
proxy_error("Private key does not match the public certificate\n");
msg = "Private key does not match the public certificate";
+ std::lock_guard<std::mutex> lock(GloVars.global.ssl_mutex);
+ GloVars.global.tls_last_load_ok = false;
ret = 1;
}Apply similar changes to other failure branches (lines 454-459, 460-465, 466-472).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/proxy_tls.cpp` around lines 439 - 442, The TLS reload success branch
currently increments GloVars.global.tls_load_count and sets
GloVars.global.tls_last_load_timestamp and GloVars.global.tls_last_load_ok =
true, but the failure branches do not set tls_last_load_ok = false; update each
TLS reload failure path to (under the same ssl_mutex) increment tls_load_count,
set tls_last_load_timestamp = time(NULL) and set tls_last_load_ok = false so
TLS_Last_Load_Result correctly reflects FAILED; look for the reload-related
branches that log failures and update those branches to mirror the success
updates (using GloVars.global.tls_load_count,
GloVars.global.tls_last_load_timestamp, and GloVars.global.tls_last_load_ok) and
ensure this is done inside the ssl_mutex-protected section.
|
@copilot : this feature is not strictly related to mysql , thus status variables shouldn't go to Also, add plenty of unit/tap tests , and ensure that the code is well documented. |
…mysql_global; add TAP tests - Add STATS_SQLITE_TABLE_GLOBAL definition (stats_global table for non-MySQL/PgSQL metrics) - Register stats_global in Admin_Bootstrap.cpp - Add stats___global() implementation and declaration; document with Doxygen comment - Remove TLS_* variables from stats___mysql_global() - they were misplaced there - Move all TLS tracking metrics to stats___global() under ssl_mutex - Wire up stats_global query detection and refresh in GenericRefreshStatistics() - Add TAP test test_tls_stats-t.cpp: - Verifies stats_global contains all 6 TLS tracking variables - Checks value ranges and validity of each TLS variable - Verifies stats_tls_certificates has 2 rows (server + ca) with correct fields - Verifies TLS_Load_Count increments and TLS_Last_Load_Timestamp increases after PROXYSQL RELOAD TLS - Confirms TLS variables are absent from stats_mysql_global Co-authored-by: renecannao <3645227+renecannao@users.noreply.github.com>
Done in commit
|
There was a problem hiding this comment.
Pull request overview
Adds new admin stats surfaces for TLS certificate introspection and TLS load/reload tracking, enabling operators to query certificate metadata and reload history without direct filesystem access.
Changes:
- Introduces
stats.stats_tls_certificates(query-time OpenSSL parsing) to expose cert metadata and expiry info. - Introduces
stats.stats_globalto hold ProxySQL-wide TLS tracking variables (moved out ofstats_mysql_global). - Adds TAP coverage to validate TLS variables, table shape/content, and behavior after
PROXYSQL RELOAD TLS.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
include/ProxySQL_Admin_Tables_Definitions.h |
Adds DDL for new stats_global and stats_tls_certificates tables. |
include/proxysql_glovars.hpp |
Adds TLS tracking fields to global state. |
lib/ProxySQL_GloVars.cpp |
Initializes newly added TLS tracking fields. |
src/proxy_tls.cpp |
Updates TLS load/reload path to populate tracking fields. |
lib/Admin_Bootstrap.cpp |
Registers new stats tables in bootstrap table definitions. |
lib/ProxySQL_Admin.cpp |
Wires query detection + refresh dispatch for the new stats tables. |
include/proxysql_admin.h |
Declares new stats refresh functions. |
lib/ProxySQL_Admin_Stats.cpp |
Implements stats___global() and stats___tls_certificates() with OpenSSL parsing helpers. |
test/tap/tests/test_tls_stats-t.cpp |
Adds a TAP test for both new stats tables and reload behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update TLS tracking fields for stats table (under ssl_mutex) | ||
| GloVars.global.tls_load_count++; | ||
| GloVars.global.tls_last_load_timestamp = time(NULL); | ||
| GloVars.global.tls_last_load_ok = true; |
There was a problem hiding this comment.
TLS tracking fields (tls_load_count/tls_last_load_timestamp/tls_last_load_ok) are only updated on the success path. If PROXYSQL RELOAD TLS fails in any of the later else branches, tls_last_load_ok can remain true from a previous success and TLS_Last_Load_Result will incorrectly report SUCCESS. Consider setting tls_last_load_ok=false (and updating the timestamp/result) under ssl_mutex on every failure path as well.
| * | ||
| * Currently tracked variables: | ||
| * - TLS_Load_Count : Number of times TLS has been loaded or reloaded. | ||
| * - TLS_Last_Load_Timestamp : Unix timestamp of the most recent successful TLS load. |
There was a problem hiding this comment.
The docstring says TLS_Last_Load_Timestamp is the timestamp of the most recent successful TLS load, but TLS_Last_Load_Result can be FAILED and the code currently reads a single tls_last_load_timestamp value. Either update the implementation to track both “last attempt” and “last successful” timestamps, or adjust the documentation/variable meaning so timestamp+result are consistent.
| * - TLS_Last_Load_Timestamp : Unix timestamp of the most recent successful TLS load. | |
| * - TLS_Last_Load_Timestamp : Unix timestamp of the most recent TLS load attempt. |
| // Helper lambda: read a PEM cert from file and insert a row into stats_tls_certificates | ||
| auto process_cert = [&](const char *cert_type, char *file_path) { | ||
| if (!file_path) return; | ||
| BIO *bio = BIO_new_file(file_path, "r"); | ||
| if (bio) { | ||
| X509 *cert = PEM_read_bio_X509(bio, NULL, NULL, NULL); | ||
| BIO_free_all(bio); | ||
| if (cert) { | ||
| insert_tls_cert_row(statsdb, stmt, cert_type, file_path, cert, loaded_at); | ||
| X509_free(cert); | ||
| } | ||
| } |
There was a problem hiding this comment.
stats___tls_certificates() only inserts a row when the cert file can be opened and parsed. If the file is missing/unreadable/invalid, the table silently ends up missing the expected cert_type row, which can break monitoring assumptions (and even your own test expects exactly 2 rows). Consider always inserting both server and ca rows (with file_path/loaded_at populated and other fields empty/default) and optionally surface read/parse failures via an additional column or by setting metadata fields empty but still present.
| // Helper lambda: read a PEM cert from file and insert a row into stats_tls_certificates | |
| auto process_cert = [&](const char *cert_type, char *file_path) { | |
| if (!file_path) return; | |
| BIO *bio = BIO_new_file(file_path, "r"); | |
| if (bio) { | |
| X509 *cert = PEM_read_bio_X509(bio, NULL, NULL, NULL); | |
| BIO_free_all(bio); | |
| if (cert) { | |
| insert_tls_cert_row(statsdb, stmt, cert_type, file_path, cert, loaded_at); | |
| X509_free(cert); | |
| } | |
| } | |
| // Helper lambda: read a PEM cert from file and insert a row into stats_tls_certificates. | |
| // Always inserts a row for the given cert_type, even if the file is missing/unreadable/invalid. | |
| auto process_cert = [&](const char *cert_type, char *file_path) { | |
| X509 *cert = nullptr; | |
| if (file_path) { | |
| BIO *bio = BIO_new_file(file_path, "r"); | |
| if (bio) { | |
| cert = PEM_read_bio_X509(bio, NULL, NULL, NULL); | |
| BIO_free_all(bio); | |
| } | |
| } | |
| // Insert a row regardless of whether the certificate was successfully loaded. | |
| insert_tls_cert_row(statsdb, stmt, cert_type, file_path, cert, loaded_at); | |
| if (cert) { | |
| X509_free(cert); | |
| } |
| if (mysql_query(admin, "PROXYSQL RELOAD TLS")) { | ||
| diag("PROXYSQL RELOAD TLS failed: %s", mysql_error(admin)); | ||
| } | ||
| mysql_free_result(mysql_store_result(admin)); | ||
|
|
||
| auto global_stats_after = query_key_value(admin, | ||
| "SELECT Variable_Name, Variable_Value FROM stats.stats_global"); | ||
|
|
||
| int tls_load_count_after = 0; | ||
| if (global_stats_after.count("TLS_Load_Count")) | ||
| tls_load_count_after = std::stoi(global_stats_after["TLS_Load_Count"]); | ||
| ok(tls_load_count_after == tls_load_count + 1, | ||
| "stats_global: TLS_Load_Count incremented after RELOAD TLS (%d -> %d)", | ||
| tls_load_count, tls_load_count_after); | ||
|
|
||
| long long tls_ts_after = 0; | ||
| if (global_stats_after.count("TLS_Last_Load_Timestamp")) | ||
| tls_ts_after = std::stoll(global_stats_after["TLS_Last_Load_Timestamp"]); | ||
| ok(tls_ts_after > tls_ts, | ||
| "stats_global: TLS_Last_Load_Timestamp increased after RELOAD TLS (%lld -> %lld)", | ||
| tls_ts, tls_ts_after); | ||
|
|
||
| string tls_result_after = global_stats_after.count("TLS_Last_Load_Result") ? | ||
| global_stats_after["TLS_Last_Load_Result"] : ""; | ||
| ok(tls_result_after == "SUCCESS", | ||
| "stats_global: TLS_Last_Load_Result='SUCCESS' after RELOAD TLS (got '%s')", | ||
| tls_result_after.c_str()); | ||
|
|
||
| // stats_tls_certificates rows still present after reload | ||
| auto cert_rows_after = query_rows(admin, | ||
| "SELECT cert_type, file_path, sha256_fingerprint FROM stats.stats_tls_certificates", | ||
| {"cert_type", "file_path", "sha256_fingerprint"}); | ||
| ok(cert_rows_after.size() == 2, | ||
| "stats_tls_certificates: still has 2 rows after RELOAD TLS (got %zu)", | ||
| cert_rows_after.size()); |
There was a problem hiding this comment.
The test logs an error if PROXYSQL RELOAD TLS fails but then continues and performs assertions that assume reload succeeded. This can produce noisy, misleading failures; consider turning the reload into an ok(mysql_query(...)==0, ...) assertion (and bail/skip dependent checks if it fails).
| if (mysql_query(admin, "PROXYSQL RELOAD TLS")) { | |
| diag("PROXYSQL RELOAD TLS failed: %s", mysql_error(admin)); | |
| } | |
| mysql_free_result(mysql_store_result(admin)); | |
| auto global_stats_after = query_key_value(admin, | |
| "SELECT Variable_Name, Variable_Value FROM stats.stats_global"); | |
| int tls_load_count_after = 0; | |
| if (global_stats_after.count("TLS_Load_Count")) | |
| tls_load_count_after = std::stoi(global_stats_after["TLS_Load_Count"]); | |
| ok(tls_load_count_after == tls_load_count + 1, | |
| "stats_global: TLS_Load_Count incremented after RELOAD TLS (%d -> %d)", | |
| tls_load_count, tls_load_count_after); | |
| long long tls_ts_after = 0; | |
| if (global_stats_after.count("TLS_Last_Load_Timestamp")) | |
| tls_ts_after = std::stoll(global_stats_after["TLS_Last_Load_Timestamp"]); | |
| ok(tls_ts_after > tls_ts, | |
| "stats_global: TLS_Last_Load_Timestamp increased after RELOAD TLS (%lld -> %lld)", | |
| tls_ts, tls_ts_after); | |
| string tls_result_after = global_stats_after.count("TLS_Last_Load_Result") ? | |
| global_stats_after["TLS_Last_Load_Result"] : ""; | |
| ok(tls_result_after == "SUCCESS", | |
| "stats_global: TLS_Last_Load_Result='SUCCESS' after RELOAD TLS (got '%s')", | |
| tls_result_after.c_str()); | |
| // stats_tls_certificates rows still present after reload | |
| auto cert_rows_after = query_rows(admin, | |
| "SELECT cert_type, file_path, sha256_fingerprint FROM stats.stats_tls_certificates", | |
| {"cert_type", "file_path", "sha256_fingerprint"}); | |
| ok(cert_rows_after.size() == 2, | |
| "stats_tls_certificates: still has 2 rows after RELOAD TLS (got %zu)", | |
| cert_rows_after.size()); | |
| int reload_rc = mysql_query(admin, "PROXYSQL RELOAD TLS"); | |
| ok(reload_rc == 0, "PROXYSQL RELOAD TLS succeeds"); | |
| if (reload_rc != 0) { | |
| diag("PROXYSQL RELOAD TLS failed: %s", mysql_error(admin)); | |
| } else { | |
| MYSQL_RES* reload_res = mysql_store_result(admin); | |
| if (reload_res != nullptr) { | |
| mysql_free_result(reload_res); | |
| } | |
| auto global_stats_after = query_key_value(admin, | |
| "SELECT Variable_Name, Variable_Value FROM stats.stats_global"); | |
| int tls_load_count_after = 0; | |
| if (global_stats_after.count("TLS_Load_Count")) | |
| tls_load_count_after = std::stoi(global_stats_after["TLS_Load_Count"]); | |
| ok(tls_load_count_after == tls_load_count + 1, | |
| "stats_global: TLS_Load_Count incremented after RELOAD TLS (%d -> %d)", | |
| tls_load_count, tls_load_count_after); | |
| long long tls_ts_after = 0; | |
| if (global_stats_after.count("TLS_Last_Load_Timestamp")) | |
| tls_ts_after = std::stoll(global_stats_after["TLS_Last_Load_Timestamp"]); | |
| ok(tls_ts_after > tls_ts, | |
| "stats_global: TLS_Last_Load_Timestamp increased after RELOAD TLS (%lld -> %lld)", | |
| tls_ts, tls_ts_after); | |
| string tls_result_after = global_stats_after.count("TLS_Last_Load_Result") ? | |
| global_stats_after["TLS_Last_Load_Result"] : ""; | |
| ok(tls_result_after == "SUCCESS", | |
| "stats_global: TLS_Last_Load_Result='SUCCESS' after RELOAD TLS (got '%s')", | |
| tls_result_after.c_str()); | |
| // stats_tls_certificates rows still present after reload | |
| auto cert_rows_after = query_rows(admin, | |
| "SELECT cert_type, file_path, sha256_fingerprint FROM stats.stats_tls_certificates", | |
| {"cert_type", "file_path", "sha256_fingerprint"}); | |
| ok(cert_rows_after.size() == 2, | |
| "stats_tls_certificates: still has 2 rows after RELOAD TLS (got %zu)", | |
| cert_rows_after.size()); | |
| } |
| // Check non-empty required fields for server cert | ||
| if (!server_row.empty()) { | ||
| ok(!server_row["file_path"].empty(), | ||
| "stats_tls_certificates: server file_path is non-empty ('%s')", | ||
| server_row["file_path"].c_str()); | ||
| ok(!server_row["sha256_fingerprint"].empty(), | ||
| "stats_tls_certificates: server sha256_fingerprint is non-empty"); | ||
| int server_days = std::stoi(server_row["days_until_expiry"]); | ||
| ok(server_days > -MAX_CERT_DAYS && server_days < MAX_CERT_DAYS, | ||
| "stats_tls_certificates: server days_until_expiry=%d is reasonable", | ||
| server_days); |
There was a problem hiding this comment.
The header comment says the test verifies non-empty subject_cn, issuer_cn, serial_number, not_before, and not_after, but the assertions currently only check file_path, sha256_fingerprint, days_until_expiry, and loaded_at. Either add assertions for the remaining columns or adjust the documented expectations so the test matches what it actually validates.
- lib/ProxySQL_GloVars.cpp: Free tls_cert_file, tls_ca_file, and tls_key_file in ProxySQL_GlobalVariables destructor. - src/proxy_tls.cpp: Added std::lock_guard during initial bootstrap of TLS variables to prevent race conditions. - src/proxy_tls.cpp: Ensure TLS file paths are updated in GloVars during PROXYSQL RELOAD TLS to keep stats table consistent with runtime configuration.
1. stats_global strstr match: add exclusion guards for stats_mysql_global and stats_pgsql_global to prevent spurious stats___global() calls and unnecessary ssl_mutex contention on every MySQL/PgSQL global stats query. 2. TLS reload failure tracking: set tls_last_load_ok=false and update tls_load_count/tls_last_load_timestamp on non-bootstrap reload failures. Previously, a failed PROXYSQL RELOAD TLS would leave stale SUCCESS status in stats_global.
…g collision stats_global is a substring of stats_mysql_global and stats_pgsql_global, causing the strstr-based dispatch to match incorrectly. Exclusion guards don't work either because a query referencing both tables would suppress the stats_proxysql_global refresh. Renaming to stats_proxysql_global follows the existing naming convention (stats_mysql_global, stats_pgsql_global) and eliminates the substring collision entirely.
renecannao
force-pushed
the
copilot/add-ssl-tls-certificate-stats-table
branch
from
ac97aa7 to
a616b34
Compare
…ertificate-stats-table # Conflicts: # test/tap/groups/groups.json
|
No way currently exists to inspect which SSL/TLS certificates ProxySQL has loaded, their validity periods, or reload history — without accessing the filesystem directly.
New table:
stats_tls_certificatesPopulated at query time (every query, no caching) by reading certs from disk via OpenSSL.
days_until_expiryis always fresh.New table:
stats_globalA new in-memory-only table for ProxySQL-wide metrics that are not specific to MySQL or PgSQL. TLS tracking variables are placed here rather than
stats_mysql_global, which is reserved for MySQL-specific metrics.TLS_Load_CountTLS_Last_Load_TimestampTLS_Last_Load_ResultNONE/SUCCESS/FAILEDTLS_Server_Cert_FileTLS_CA_Cert_FileTLS_Key_FileImplementation
include/ProxySQL_Admin_Tables_Definitions.h: DDL forstats_tls_certificatesand newstats_globaltablesinclude/proxysql_glovars.hpp+lib/ProxySQL_GloVars.cpp: Six newglobalstruct fields (tls_load_count,tls_last_load_timestamp,tls_last_load_ok,tls_cert_file,tls_ca_file,tls_key_file); initialized in constructorsrc/proxy_tls.cpp:ProxySQL_create_or_load_TLS()populates tracking fields on success; file paths stored asstrdupcopies (freed before reassignment); reload path updates counters inside existingssl_mutexlocklib/Admin_Bootstrap.cpp: Both tables registered viainsert_into_tables_defslib/ProxySQL_Admin_Stats.cpp:stats___tls_certificates()reads certs at query time usingPEM_read_bio_X509, extracts metadata via OpenSSL (ASN1_TIME_diff,X509_digest,X509_NAME_get_text_by_NID); newstats___global()populates TLS tracking variables (removed fromstats___mysql_global());ASN1_TIME_diffreturn value is checkedlib/ProxySQL_Admin.cpp: Query detection (strstr) and refresh dispatch wired inGenericRefreshStatistics()for both tablesinclude/proxysql_admin.h: Declarations forstats___tls_certificates()andstats___global()test/tap/tests/test_tls_stats-t.cpp: TAP test with 29 assertions covering presence and validity of all TLS variables instats_global, correct structure and content ofstats_tls_certificates, counter/timestamp updates afterPROXYSQL RELOAD TLS, and absence of TLS variables fromstats_mysql_globalOriginal prompt
This section details on the original issue you should resolve
<issue_title>Feature Request: Add stats table to track loaded SSL/TLS certificates metadata</issue_title>
<issue_description>## Summary
ProxySQL loads server-side SSL certificates/keys during startup and when
PROXYSQL RELOAD TLSis executed. Currently, there is no way to query which certificates are loaded, their validity periods, or when they were last reloaded. This feature request proposes adding a new stats table to expose SSL certificate metadata.Current Behavior
Certificate Loading Process
SSL certificates are loaded in
src/proxy_tls.cpp:ProxySQL_create_or_load_TLS(true, msg)is called fromsrc/main.cpp:416PROXYSQL RELOAD TLScommand triggersProxySQL_create_or_load_TLS(false, msg)fromlib/Admin_Handler.cpp:988The three expected files are:
proxysql-key.pem- Private keyproxysql-cert.pem- Server certificateproxysql-ca.pem- CA certificateFile paths are constructed from
GloVars.datadirand stored in static variables:Current Tracking (Minimal)
The only SSL-related metric currently tracked is in
lib/ProxySQL_Admin_Stats.cpp:616:No certificate metadata (subject, issuer, validity dates, etc.) is captured or exposed.
Logging Only
Current logging provides minimal information:
src/proxy_tls.cpp:286:"No SSL keys/certificates found in datadir (%s). Generating new keys/certificates."src/proxy_tls.cpp:321:"SSL keys/certificates found in datadir (%s): loading them."Problem Statement
Operators cannot:
PROXYSQL RELOAD TLS)Proposed Solution
New Stats Table:
stats_tls_certificatesCreate a new table in the
statsdatabase to expose certificate metadata:Example Output
Additional Global Stats
Add the following to
stats_mysql_global:TLS_Load_Count5TLS_Last_Load_Timestamp1705315800TLS_Last_Load_ResultSUCCESSTLS_Server_Cert_File/var/lib/proxysql/proxysql-cert.pemTLS_CA_Cert_File/var/lib/proxysql/proxysql-ca.pemTLS_Key_File/var/lib/proxysql/proxysql-key.pemImplementation Details
Runtime Table Refresh
The table must be repopulated at runtime every time it is queried, following the pattern used by other stats tables. This ensures:
days_until_expiryis always current (calculat...🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit
Release Notes