contrib/database/sql: sanitize sensitive data in error messages#3686
Conversation
This change ensures that database connection strings and other sensitive information are properly sanitized in error messages and logs. The implementation handles various DSN formats including MySQL, PostgreSQL, and SQL Server connection strings, preventing exposure of credentials in application logs. Signed-off-by: Kemal Akkoyun <[email protected]>
github-actions
Bot
added
the
apm:ecosystem
|
@eliottness could you have look at it? |
Datadog ReportBranch report: ✅ 0 Failed, 70 Passed, 0 Skipped, 4m 22.43s Total Time |
RomainMuller
left a comment
RomainMuller
left a comment
There was a problem hiding this comment.
TBH I feel this might still fail in ways that expose credentials and I'd rather simplify it if possible...
| keyValueSpacePattern = regexp.MustCompile(`(?i)(password|passwd|pwd)\s*=\s*(.+?)(\s+(?:host|user|port|database|dbname)\s*=)`) | ||
| keyValueSemiPattern = regexp.MustCompile(`(?i)(password|passwd|pwd)\s*=\s*([^;]+)(;)`) | ||
| keyValuePattern = regexp.MustCompile(`(?i)(password|passwd|pwd)\s*=\s*([^\s;]+)`) |
There was a problem hiding this comment.
Like to have regex1 links for all regexes so they can easily be tested etc...
These also make me feel a little frisky -- this is effectively a filtering deny-list, so who know about patterns you've not thought about, yet are still credentials?
There was a problem hiding this comment.
It is just a best effort. I'm aware this solution doesn't cover all possible cases.
| // Fallback: use the comprehensive sanitize() function for all password patterns. | ||
| e.URL = sanitize(e.URL) | ||
| return e |
There was a problem hiding this comment.
As noted below, the fallback makes it possible to leak credentials, in particular since the URL is malformed here, there could be typos in strategic places that cause credentials to still be leaked. I'd rather use an opaque string here...
There was a problem hiding this comment.
👍 Makes sense. It was hard to get the regexes right for the test cases that I have came up with.
Having an opaque string to say indicate that this URL in this error is redacted is fine for me.
| // Look for URL patterns and manually handle them to avoid issues with @ in passwords. | ||
| urlStart := strings.Index(msg, "://") | ||
| if urlStart == -1 { | ||
| return msg | ||
| } |
There was a problem hiding this comment.
This is a fallback for an URL that did not successfully parse isn't it? Yet here we're failing open in cases where the URL is blatantly invalid?
| } | ||
|
|
||
| // sanitizeURLPasswords sanitizes passwords in URL format (user:pass@host). | ||
| func sanitizeURLPasswords(msg string) string { |
There was a problem hiding this comment.
This feels dangerously close to trying to correctly parse an invalid URL (since url.Parse failed)... parsing valid URLs correctly is already extremely difficult...
Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
5 participants
Summary
This PR enhances error handling in the database/sql contrib package by ensuring that sensitive information in database connection strings is properly sanitized before being logged or included in error messages.
Changes
Security Enhancement
This change proactively ensures that database credentials and other sensitive connection details are never exposed in error messages or debug logs, following security best practices for credential handling.
Test Coverage