postgresql-interfaces/psqlodbc GitHub issues and pull requests (mirror)  

help / color / mirror / Atom feed

From: davecramer (@davecramer) <[email protected]>
To: postgresql-interfaces/psqlodbc <[email protected]>
Subject: Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
Date: Wed, 13 May 2026 09:37:47 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[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.

view thread (8+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: github://postgresql-interfaces/psqlodbc
  Cc: [email protected], [email protected]
  Subject: Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
  In-Reply-To: <<[email protected]>>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox