Mask password/token in log messages. #205
Conversation
|
@jdamata How about adding a test or two to prove that your masking actually works? :) |
| { | ||
| errorMessage: errors.New("Error updating SonarQube user: failed to execute http request: POST https://PASSWORD:@sonarqube.example.com/api/users/update_identity_provider?login=gitlab-john-doe&newExternalIdentity=john-doe&newExternalProvider=gitlab giving up after 5 attempt(s). Request: &{0xab1940 0xc00021c600}"), | ||
| token: "PASSWORD", | ||
| expected: "Error updating SonarQube user: failed to execute http request: POST https://********:@sonarqube.example.com/api/users/update_identity_provider?login=gitlab-john-doe&newExternalIdentity=john-doe&newExternalProvider=gitlab giving up after 5 attempt(s). Request: &{0xab1940 0xc00021c600}", |
There was a problem hiding this comment.
I'm not sure I would have such a "complex" string in the test - things like the number of attempts, the request id etc are not relevant to the test - wouldn't a simple GET request to an endpoint suffice?
I'm a big fan of keeping tests simple.
I would also argue that the test is a bit brittle because it is testing too much.
Is the purpose of the test to ensure that the sensitive information is masked out or to ensure that the function returns the original error message but with the sensitive info masked?
I would argue the former and therefore all you should be testing here is that the sensitive info is NOT included in the output - this is the key part of the functionality and should be tested in isolation IMO.
You don't want the test to fail because for example some future refactor truncated the message. That's a different problem and would have a different test :)
And indeed ideally you would have a test which has no sensitive info to check that you do get back the original string.
| func censorError(err error, secret string) error { | ||
| // Replace the secret with asterisks of the same length | ||
| asterisks := strings.Repeat("*", len(secret)) | ||
| result := strings.Replace(err.Error(), secret, asterisks, -1) |
There was a problem hiding this comment.
The problem with this (admittedly a small one) is that you are still giving away information about the secret - ie the length of it because you return the same number of characters as the original.
Why not simply replace with say four asterisks - **** - in all cases so that you give nothing away?
| @@ -26,17 +29,17 @@ type Paging struct { | |||
| } | |||
|
|
|||
| // helper function to make api request to sonarqube | |||
| func httpRequestHelper(client *retryablehttp.Client, method string, sonarqubeURL string, expectedResponseCode int, errormsg string) (http.Response, error) { | |||
| func httpRequestHelper(client *retryablehttp.Client, method string, sonarqubeURL url.URL, expectedResponseCode int, errormsg string) (http.Response, error) { | |||
There was a problem hiding this comment.
I'm not objecting to the change but why make this a url now and then have to .String() it everywhere?
| // https://github.com/jdamata/terraform-provider-sonarqube/issues/201 | ||
| // go-retryablehttp error contains the token/user:pass in plaintext. | ||
| // We want to censor that secret before logging the error | ||
| func censorError(err error, secret string) error { |
There was a problem hiding this comment.
There are more types of sensitive data than just the token used to connect to sonar - for example what if there is an error in creating a user? Or simply a debug statement logging the params - Would we be emitting the user details, including the password into the logs?
I wouldn't necessarily "limit" this to errors (which you do by the name of the function) and perhaps consider how would this be used if it is a user request that fails - would you call this function multiple times, once for the API token and again to mask the password? Or pass in an array of sensitive data strings so you can make one call and sensor both?
I think ensuring that all logs are masked is something to consider, perhaps for a later issue - but here ensuring that all errors are masked (including any supplied password) should be the goal of this PR.
There was a problem hiding this comment.
@jdamata Some comments inline - some of it is no biggie, the main thing I think is exposing the number of characters in the secret and also not masking user passwords if trying to manage user resources
#201