Message-ID: <gh-postgresql-interfaces-psqlodbc-184-c4439513606@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "postgresql-interfaces/psqlodbc" <noreply+postgresql-interfaces-psqlodbc@github.com>
Date: Wed, 13 May 2026 09:37:47 +0000
Subject: Re: [postgresql-interfaces/psqlodbc] PR #184: Redact sensitive connection parameters in logs
In-Reply-To: <gh-postgresql-interfaces-psqlodbc-184@github.com>
References: <gh-postgresql-interfaces-psqlodbc-184@github.com>
List-Id: <gh-postgresql-interfaces-psqlodbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 4439513606
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 184
X-GitHub-Repo: postgresql-interfaces/psqlodbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/postgresql-interfaces/psqlodbc/pull/184#issuecomment-4439513606
Content-Type: text/plain; charset=utf-8

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.