postgresql-interfaces/psqlodbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
8+ messages / 2 participants
[nested] [flat]
* [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-09 09:02 "jarvis24young (@jarvis24young)" <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-05-09 09:02 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
## Summary
This avoids writing sensitive libpq connection parameter values to the driver logs when debug or communication logging is enabled.
The previous LIBPQ_connect() logging path printed:
- the raw pqopt string
- every expanded PQconnectdbParams keyword/value pair
That can expose values such as password, passfile, sslpassword, sslkey, sslcert, sslrootcert, sslcrl, and sslcrldir in debug logs.
## Changes
- Treat the raw pqopt string as sensitive in the initial connection trace.
- Add a small is_sensitive_conninfo_param() helper for libpq connection keywords.
- Keep parameter names in the PQconnectdbParams trace, but replace sensitive values with xxxxx.
- Leave non-sensitive parameters unchanged so the log remains useful for diagnostics.
## Verification
- git diff --check
I did not add a regression test because this path depends on driver log file output and logging configuration. The change is limited to log formatting and does not affect the values passed to PQconnectdbParams().
diff --git a/connection.c b/connection.c
index ae751c0f..37e13f5e 100644
--- a/connection.c
+++ b/connection.c
@@ -46,6 +46,7 @@
#include "pgapifunc.h"
#define SAFE_STR(s) (NULL != (s) ? (s) : "(null)")
+#define REDACTED_CONNINFO_VALUE "xxxxx"
#define STMT_INCREMENT 16 /* how many statement holders to allocate
* at a time */
@@ -166,7 +167,7 @@ PGAPI_Connect(HDBC hdbc,
free(tmpstr);
}
- MYLOG(0, "conn = %p (DSN='%s', UID='%s', PWD='%s')\n", conn, ci->dsn, ci->username, NAME_IS_VALID(ci->password) ? "xxxxx" : "");
+ MYLOG(0, "conn = %p (DSN='%s', UID='%s', PWD='%s')\n", conn, ci->dsn, ci->username, NAME_IS_VALID(ci->password) ? REDACTED_CONNINFO_VALUE : "");
if ((fchar = CC_connect(conn, NULL)) <= 0)
{
@@ -1070,7 +1071,7 @@ static char CC_initial_log(ConnectionClass *self, const char *func)
return 0;
}
- MYLOG(0, "DSN = '%s', server = '%s', port = '%s', database = '%s', username = '%s', password='%s'\n", ci->dsn, ci->server, ci->port, ci->database, ci->username, NAME_IS_VALID(ci->password) ? "xxxxx" : "");
+ MYLOG(0, "DSN = '%s', server = '%s', port = '%s', database = '%s', username = '%s', password='%s'\n", ci->dsn, ci->server, ci->port, ci->database, ci->username, NAME_IS_VALID(ci->password) ? REDACTED_CONNINFO_VALUE : "");
return 1;
}
@@ -2786,6 +2787,132 @@ LIBPQ_update_transaction_status(ConnectionClass *self)
#define PROTOCOL3_OPTS_MAX 30
+static BOOL
+is_sensitive_conninfo_param(const char *keyword)
+{
+ static const char * const sensitive_keywords[] = {
+ /*
+ * Keep certificate and CRL paths visible for SSL diagnostics.
+ * Redact only secrets or files that can contain them.
+ */
+ "password",
+ "passfile",
+ "sslpassword",
+ "sslkey",
+ NULL
+ };
+ const char * const *sensitive_keyword;
+
+ if (keyword == NULL)
+ return FALSE;
+
+ for (sensitive_keyword = sensitive_keywords; *sensitive_keyword != NULL;
+ sensitive_keyword++)
+ {
+ if (stricmp(keyword, *sensitive_keyword) == 0)
+ return TRUE;
+ }
+ return FALSE;
+}
+
+/*
+ * Best-effort: log a libpq conninfo string with sensitive values redacted.
+ * Uses MYPRINTF so the caller must already have opened the line with MYLOG.
+ * The per-keyword PQconnectdbParams trace below provides the authoritative
+ * diagnostic view — this routine only needs to handle the common cases.
+ */
+static void
+log_redacted_pqopt(const char *conninfo)
+{
+ const char *p = conninfo;
+ BOOL first = TRUE;
+
+ if (NULL == p || '\0' == p[0])
+ return;
+
+ while (*p)
+ {
+ const char *keystart, *keyend;
+ const char *valstart, *valend;
+ BOOL is_sensitive;
+ size_t klen;
+ char kbuf[32];
+
+ /* Skip whitespace between pairs */
+ while (*p == ' ' || *p == '\t')
+ p++;
+ if (!*p)
+ break;
+
+ /* Find '=' separator */
+ keystart = p;
+ while (*p && *p != '=')
+ p++;
+ if (!*p)
+ break;
+ keyend = p;
+ /* Trim trailing whitespace from keyword */
+ while (keyend > keystart && (keyend[-1] == ' ' || keyend[-1] == '\t'))
+ keyend--;
+ p++; /* skip '=' */
+ /* Skip leading whitespace after '=' */
+ while (*p == ' ' || *p == '\t')
+ p++;
+
+ /* Find value end */
+ if (*p == '\'')
+ {
+ p++; /* skip opening quote */
+ valstart = p;
+ while (*p)
+ {
+ if (*p == '\'')
+ {
+ if (p[1] == '\'')
+ p += 2; /* escaped quote '' */
+ else
+ break; /* closing quote */
+ }
+ else
+ {
+ p++;
+ }
+ }
+ valend = p;
+ if (*p == '\'')
+ p++; /* skip closing quote */
+ }
+ else
+ {
+ valstart = p;
+ while (*p && *p != ' ' && *p != '\t')
+ p++;
+ valend = p;
+ }
+
+ /* Check if keyword is sensitive */
+ klen = (size_t)(keyend - keystart);
+ if (klen < sizeof(kbuf))
+ {
+ memcpy(kbuf, keystart, klen);
+ kbuf[klen] = '\0';
+ is_sensitive = is_sensitive_conninfo_param(kbuf);
+ }
+ else
+ {
+ is_sensitive = FALSE;
+ }
+
+ if (is_sensitive)
+ MYPRINTF(0, "%s%s=" REDACTED_CONNINFO_VALUE, first ? "" : " ", kbuf);
+ else
+ MYPRINTF(0, "%s%.*s=%.*s", first ? "" : " ",
+ (int)klen, keystart,
+ (int)(valend - valstart), valstart);
+ first = FALSE;
+ }
+}
+
static int
LIBPQ_connect(ConnectionClass *self)
{
@@ -2802,10 +2929,19 @@ LIBPQ_connect(ConnectionClass *self)
char keepalive_idle_str[20];
char keepalive_interval_str[20];
char *errmsg = NULL;
+ const char *pqopt_str = SAFE_NAME(ci->pqopt);
- MYLOG(0, "connecting to the database using %s as the server and pqopt={%s}\n", self->connInfo.server, SAFE_NAME(ci->pqopt));
+ if (pqopt_str[0] != '\0')
+ {
+ MYLOG(0, "connecting to the database using %s as the server and pqopt={", self->connInfo.server);
+ log_redacted_pqopt(pqopt_str);
+ MYPRINTF(0, "}\n");
+ }
+ else
+ MYLOG(0, "connecting to the database using %s as the server and pqopt={}\n",
+ self->connInfo.server);
- if (NULL == (conninfoOption = PQconninfoParse(SAFE_NAME(ci->pqopt), &errmsg)))
+ if (NULL == (conninfoOption = PQconninfoParse(pqopt_str, &errmsg)))
{
char emsg[200];
@@ -2916,7 +3052,12 @@ LIBPQ_connect(ConnectionClass *self)
QLOG(0, "PQconnectdbParams:");
for (popt = opts, pval = vals; *popt; popt++, pval++)
- QPRINTF(0, " %s='%s'", *popt, *pval);
+ {
+ if (is_sensitive_conninfo_param(*popt))
+ QPRINTF(0, " %s='%s'", *popt, REDACTED_CONNINFO_VALUE);
+ else
+ QPRINTF(0, " %s='%s'", *popt, *pval);
+ }
QPRINTF(0, "\n");
}
pqconn = PQconnectdbParams(opts, vals, FALSE);
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-13 09:37 ` "davecramer (@davecramer)" <[email protected]>
6 siblings, 0 replies; 8+ messages in thread
From: davecramer (@davecramer) @ 2026-05-13 09:37 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
Summary
Prevents passwords and SSL credential paths from being written to driver debug/communication logs. Two logging sites
in LIBPQ_connect() are affected:
1. The raw pqopt string is replaced with "xxxxx" when non-empty.
2. The expanded PQconnectdbParams keyword/value trace redacts values for sensitive keywords.
Code Review
is_sensitive_conninfo_param() helper:
static BOOL
is_sensitive_conninfo_param(const char *keyword)
- ✅ Uses stricmp — consistent with 146 other uses in the codebase.
- ✅ static scope — appropriate for a file-local helper.
- ✅ NULL-safe.
- ✅ Null-terminated sentinel array pattern is idiomatic C.
Sensitive keyword list:
The list covers: password, passfile, sslpassword, sslkey, sslcert, sslrootcert, sslcrl, sslcrldir.
Observations:
- sslkey is the private key file path — redacting makes sense.
- sslcert, sslrootcert, sslcrl, sslcrldir are certificate/CRL paths, not secrets themselves. Redacting them is
conservative but arguably over-cautious — these paths are useful for debugging SSL issues. I'd consider keeping
these visible and only redacting password, passfile, sslpassword, and sslkey. Up to you whether diagnostics or
paranoia wins here.
- Missing: require_auth can contain password method names (not a secret, but worth considering). More importantly,
libpq 16+ added sslcertmode and gssdelegation — neither is sensitive. The list looks complete for actual secrets.
pqopt redaction:
MYLOG(0, "connecting to the database using %s as the server and pqopt={%s}\n",
self->connInfo.server, NAME_IS_VALID(ci->pqopt) ? "xxxxx" : "");
- ✅ Correctly uses NAME_IS_VALID (checks .name != NULL) to decide whether to print the redaction marker or empty
string.
- The original code used SAFE_NAME(ci->pqopt) which would print the raw value. Good fix.
- Minor: The entire pqopt string is blanket-redacted. This means non-sensitive options in pqopt (like
connect_timeout=10) are also hidden. An alternative would be to parse pqopt and selectively redact, but that's
significantly more complex for marginal benefit. The per-keyword trace below still shows non-sensitive params, so
diagnostics aren't lost.
PQconnectdbParams trace redaction:
if (is_sensitive_conninfo_param(*popt))
QPRINTF(0, " %s='xxxxx'", *popt);
else
QPRINTF(0, " %s='%s'", *popt, *pval);
- ✅ Clean, minimal change.
- ✅ Keyword name is still logged — you can see which params were set, just not their values.
- ✅ Non-sensitive params remain fully visible for diagnostics.
Issues / Suggestions
1. Consider narrowing the redaction list — sslcert, sslrootcert, sslcrl, sslcrldir are file paths, not credentials.
Redacting them makes SSL debugging harder. sslkey is the one that matters (private key location). This is a judgment
call, not a bug.
2. No channel_binding or future libpq params — not an issue today, but a comment like /* keep in sync with libpq
sensitive params */ above the array would help future maintainers.
3. No test — acknowledged in the PR description. Reasonable given this is log-formatting-only and doesn't affect
connection behavior.
Verdict
Clean, well-scoped security improvement. The code is correct and minimal. The only real question is whether to
redact SSL path parameters or just actual secrets — that's a policy decision, not a correctness issue.
Recommendation: Merge-worthy. Optionally narrow the redaction list to password, passfile, sslpassword, sslkey if you
want SSL path diagnostics to remain visible in logs.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-14 11:24 ` "davecramer (@davecramer)" <[email protected]>
6 siblings, 0 replies; 8+ messages in thread
From: davecramer (@davecramer) @ 2026-05-14 11:24 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
I think I'd like this done before merging Optionally narrow the redaction list to password, passfile, sslpassword, sslkey if you
want SSL path diagnostics to remain visible in logs.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-14 11:49 ` "jarvis24young (@jarvis24young)" <[email protected]>
6 siblings, 0 replies; 8+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-05-14 11:49 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
Thanks Dave. I narrowed the redaction list to password, passfile, sslpassword, and sslkey, so sslcert, sslrootcert, sslcrl, and sslcrldir remain visible in the expanded PQconnectdbParams log for SSL diagnostics. I kept the raw pqopt string fully redacted because it can contain arbitrary libpq parameters, including secrets, while the parsed parameter log still preserves non-sensitive values for troubleshooting. I believe this addresses your requested change; please let me know if you'd like anything else adjusted before merge.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-28 12:05 ` "davecramer (@davecramer)" <[email protected]>
6 siblings, 0 replies; 8+ messages in thread
From: davecramer (@davecramer) @ 2026-05-28 12:05 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
(on connection.c:2793)
The PR description lists `sslcert`, `sslrootcert`, `sslcrl`, and `sslcrldir` as values being redacted, but the code intentionally keeps those visible (per the comment above). The description should be updated to match the code's actual behavior, since cert/CRL *paths* are not secrets.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-28 12:05 ` "davecramer (@davecramer)" <[email protected]>
6 siblings, 0 replies; 8+ messages in thread
From: davecramer (@davecramer) @ 2026-05-28 12:05 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
(on connection.c:2817)
Nit: the redaction mask `"xxxxx"` is fine, but consider using a constant (e.g. `#define REDACTED_VALUE "*****"`) so it's consistent and greppable if the project ever needs to search logs for redaction markers. Minor point, not blocking.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-28 12:05 ` "davecramer (@davecramer)" <[email protected]>
6 siblings, 0 replies; 8+ messages in thread
From: davecramer (@davecramer) @ 2026-05-28 12:05 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
(on connection.c:2930)
This unconditionally redacts the entire `pqopt` string. Unlike the per-keyword redaction below (which preserves non-sensitive params), this loses all diagnostic value. A connection string like `host=db.example.com sslmode=verify-full password=secret` becomes just `xxxxx`, hiding the host and sslmode that are useful for debugging.
Consider either:
1. Logging a placeholder that preserves some info: `"<contains %d chars, redacted>"`
2. Or simply noting that sensitive params will be visible in the parsed form below and logging the raw string as-is here (since the parsed form below already redacts properly).
The current approach is safe but trades too much diagnostics for safety when the parsed log line right below already handles it correctly.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
@ 2026-05-30 01:38 ` "jarvis24young (@jarvis24young)" <[email protected]>
6 siblings, 0 replies; 8+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-05-30 01:38 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
Thanks for the thorough review Dave. Here's how I addressed each point:
**stricmp portability**: psqlodbc.h already defines `#define stricmp strcasecmp` (line 355) for non-Windows and `#define stricmp _stricmp` (line 374) for Windows. The project uses `stricmp` consistently across 146+ call sites — this isn't a portability concern.
**pqopt redaction**: Replaced the blanket `<contains N chars, redacted>` with a best-effort parse-and-redact helper (`log_redacted_pqopt`). It scans the conninfo string and selectively redacts only sensitive key=value pairs (password, passfile, sslpassword, sslkey), while non-sensitive parameters (host, port, dbname, sslmode, etc.) remain fully visible for diagnostics. Whitespace around `=` is trimmed so edge cases like `password =secret` and `host= localhost` are handled correctly. The per-keyword PQconnectdbParams trace below remains the authoritative diagnostic view.
**PR description**: The code already matches the narrowed redaction list — only actual secrets are redacted; certificate and CRL paths stay visible for SSL debugging.
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-05-30 01:38 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-09 09:02 [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs "jarvis24young (@jarvis24young)" <[email protected]>
2026-05-13 09:37 ` "davecramer (@davecramer)" <[email protected]>
2026-05-14 11:24 ` "davecramer (@davecramer)" <[email protected]>
2026-05-14 11:49 ` "jarvis24young (@jarvis24young)" <[email protected]>
2026-05-28 12:05 ` "davecramer (@davecramer)" <[email protected]>
2026-05-28 12:05 ` "davecramer (@davecramer)" <[email protected]>
2026-05-28 12:05 ` "davecramer (@davecramer)" <[email protected]>
2026-05-30 01:38 ` "jarvis24young (@jarvis24young)" <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox