Redact sensitive connection parameters in logs

[jarvis24young]

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

[davecramer]

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.

[davecramer]

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.

[jarvis24young]

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.

[davecramer]

Review

Good change, clear motivation. A few observations:

Missing sensitive keywords

The PR description mentions redacting sslcert, sslrootcert, sslcrl, and sslcrldir, but the code only redacts password, passfile, sslpassword, and sslkey. The comment in the code says "Keep certificate and CRL paths visible for SSL diagnostics," which contradicts the PR description. The code is correct (cert paths are not secrets), but the PR description should be updated to match.

stricmp portability

stricmp is MSVC-specific. POSIX systems use strcasecmp. This appears to be fine for this project since psqlodbc already uses stricmp elsewhere (and likely has a portability macro), but worth confirming the project has a #define or equivalent in place for non-Windows builds.

The pqopt redaction may be too aggressive

The raw pqopt string is now unconditionally replaced with "xxxxx" if it has any content. This loses all diagnostic value (host, port, dbname, sslmode, etc. are all useful in logs). The per-keyword redaction you do for PQconnectdbParams is the right granularity. Consider applying the same approach to pqopt (parse it and redact only the sensitive key=value pairs), or at minimum log that a pqopt was provided without showing the value, e.g. pqopt={<redacted, N chars>}.

Overall this is a solid improvement. The inline comments below are minor.

[davecramer]

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.

[davecramer]

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.

[davecramer]

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.

[jarvis24young]

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.