Subject: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Sat, 09 May 2026 09:02:56 +0000

## 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);

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 13 May 2026 09:37:47 +0000

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.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 14 May 2026 11:24:39 +0000

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.

---

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Thu, 14 May 2026 11:49:33 +0000

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.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 28 May 2026 12:05:21 +0000

(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.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 28 May 2026 12:05:21 +0000

(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.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 28 May 2026 12:05:22 +0000

(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.

---

From: "jarvis24young (@jarvis24young)" <[email-masked]>
Date: Sat, 30 May 2026 01:38:19 +0000

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.