Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error messages output potentially sensitive information #1161

Closed
rselbach opened this issue Sep 12, 2024 · 2 comments · Fixed by #1162
Closed

Error messages output potentially sensitive information #1161

rselbach opened this issue Sep 12, 2024 · 2 comments · Fixed by #1162

Comments

@rselbach
Copy link
Contributor

Describe the Bug

A recent change has been made to improve error messages #1103

This change introduced a situation where if golang-migrate cannot connect to the database, it will output the connection string which can contain sensitive information (usernames and passwords) which can in turn can in logs.

Steps to Reproduce
Steps to reproduce the behavior:

  1. Try connecting to a DB that isn't accessible (e.g. use the wrong hostname)
  2. See error error: failed to open database, "postgresql://someusername:SeCretPassword...

Expected Behavior

Software should not output sensitive information

Migrate Version

v4.18.1

@dhui
Copy link
Member

dhui commented Sep 23, 2024

@rselbach Thanks for reporting and suggesting a fix for this issue.
The solutions I've seen for this problem (having both helpful and safe/sure log messages) would either be:

  1. Use multiple log levels. e.g. output sensitive information at verbose level but normally run at info level or higher.
  2. log filtering for sensitive fields - typically requires structured logging or scanning log statements

Unfortunately, neither is easy to do with migrate since it involves changing logging.
Do you have other ideas? Maybe the solution is to either:

  1. backout the db url from the error messages
  2. parse the db url to exclude username/password from the error

@rselbach
Copy link
Contributor Author

rselbach commented Sep 28, 2024

@dhui I actually started by removing the credentials from the databaseURL but then reverted course to err on the side of caution because it's caller input so whatever removal logic I came up with would never be perfect. Also, because it's a parameter passed by the caller, it is safer and easier to leave it to the caller to debug-log their connection string if they choose to do so.

If we're being pendantic, golang-migrate wasn't doing anything wrong here: it doesn't log anything, it doesn't actually output anything. But I think we can assume some people will be surprised to find out that error messages include credentials. I know we were when we found out. So the Principle of Least Surprise applies here 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants