-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Improve query sanitization to prevent a password leak in the logs #6421
Conversation
// This function works on the raw query and attempts to retain the original input | ||
// as much as possible. | ||
func Sanitize(query string) string { | ||
sanitizeSetPassword := regexp.MustCompile(`(?i)password\s+for[^=]*=\s+(["']?[^\s"]+["']?)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a little strange to compile the regexp on every call to Sanitize. Any reason for not pulling it up to a package-level, unexported var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be. Let me move it. This was just easier while I was writing the code.
23e9e02
to
a0af340
Compare
I like the regex approach, but will running two regexes against every query add significant overhead? |
@@ -257,23 +257,17 @@ func (h *Handler) serveQuery(w http.ResponseWriter, r *http.Request, user *meta. | |||
p := influxql.NewParser(strings.NewReader(qp)) | |||
db := q.Get("db") | |||
|
|||
// Sanitize the request query params so it doesn't show up in the response logger. | |||
// Do this before anything else so a parsing error doesn't leak passwords. | |||
sanitize(r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since query.Statements
is no longer used, I think sanitize
can be called in the response logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to run the sanitizer for non-query requests. The response logger doesn't seem to be limited to just serveQuery
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response logger feels like a more natural fit for this code than the query handler. Other requests won't have a q
query parameter but it could also be skipped with an if r.URL.EscapedPath() == "/query"
check.
I'm not sure. The golang regex library is based off of re2 rather than perl regular expressions so it's generally much more efficient. The advantage of this approach is that the query doesn't have to parse correctly. I made mistakes while testing and did this:
This wouldn't get redacted because it resulted in a parse error and the password just got printed to the log. |
a0af340
to
3c381f3
Compare
Added a benchmark of the Sanitize function.
|
I like the regex approach and that it covers mistyped queries. Just wanted to mention the potential performance impact since the regexes are run on every query and the number of queries containing a password is very small. I ran some quick tests of 10,000 |
I tried a simple test using
It seems like the average query response time gets slowed down by about 3ms. @jwilder thoughts? |
LGTM 👍 |
Go's |
Sanitizing is now done through pattern matching rather than parsing the query and replacing the password in the query. This prevents accidentally redacting the wrong part of a query and revealing what the password is through association. Fixes #3883.
3c381f3
to
62c66b7
Compare
@e-dard I notice that much variation w/ |
I'll merge this then and we'll update sanitize if it turns out to be bad for performance. |
Sanitizing is now done through pattern matching rather than parsing the
query and replacing the password in the query. This prevents
accidentally redacting the wrong part of a query and revealing what the
password is through association.
Fixes #3883.