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.

Sign up for GitHub

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

Mask password/token in log messages. #205

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Mask password/token in log messages. #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e24e1d0
Mask password/token in log messages.
jdamata Oct 3, 2023
3bb7b5d
add test case
jdamata Oct 20, 2023
5304336
Merge branch 'master' into mask-secret-201
jdamata Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions sonarqube/httpHelpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ package sonarqube

import (
"encoding/json"
"errors"
"fmt"
"net/http"
"net/url"
"strings"

"github.com/hashicorp/go-retryablehttp"
)
Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not objecting to the change but why make this a url now and then have to .String() it everywhere?

// Prepare request
req, err := retryablehttp.NewRequest(method, sonarqubeURL, http.NoBody)
req, err := retryablehttp.NewRequest(method, sonarqubeURL.String(), http.NoBody)
if err != nil {
return http.Response{}, fmt.Errorf("failed to prepare http request: %v. Request: %v", err, req)
return http.Response{}, fmt.Errorf("failed to prepare http request: %v", censorError(err, sonarqubeURL.User.String()))
}

// Execute request
resp, err := client.Do(req)
if err != nil {
return http.Response{}, fmt.Errorf("failed to execute http request: %v. Request: %v", err, req)
return http.Response{}, fmt.Errorf("failed to execute http request: %v", censorError(err, sonarqubeURL.User.String()))
}

// Check response code
Expand All @@ -57,3 +60,13 @@ func httpRequestHelper(client *retryablehttp.Client, method string, sonarqubeURL

return *resp, nil
}

// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Replace the secret with asterisks of the same length
asterisks := strings.Repeat("*", len(secret))
result := strings.Replace(err.Error(), secret, asterisks, -1)
Copy link
Collaborator

@freeranger freeranger Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return errors.New(result)
}
29 changes: 29 additions & 0 deletions sonarqube/httpHelpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package sonarqube

import (
"errors"
"reflect"
"testing"
)

// TestCensorError calls censorError with sample log messages , checking
// for a valid return value.
func TestCensorError(t *testing.T) {
cases := []struct {
errorMessage error
token string
expected string
}{
{
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}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
}
for _, c := range cases {
out := censorError(c.errorMessage, c.token)
if !reflect.DeepEqual(out.Error(), c.expected) {
t.Fatalf("Error matching output and expected: %#v vs %#v", out.Error(), c.expected)
}
}
}
2 changes: 1 addition & 1 deletion sonarqube/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func sonarqubeSystemInfo(client *retryablehttp.Client, sonarqube url.URL) (strin
resp, err := httpRequestHelper(
client,
"GET",
sonarqube.String(),
sonarqube,
http.StatusOK,
"sonarqubeHealth",
)
Expand Down
8 changes: 4 additions & 4 deletions sonarqube/resource_sonarqube_alm_azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func resourceSonarqubeAlmAzureCreate(d *schema.ResourceData, m interface{}) erro
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAlmAzureCreate",
)
Expand All @@ -90,7 +90,7 @@ func resourceSonarqubeAlmAzureRead(d *schema.ResourceData, m interface{}) error
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeAlmAzureRead",
)
Expand Down Expand Up @@ -129,7 +129,7 @@ func resourceSonarqubeAlmAzureUpdate(d *schema.ResourceData, m interface{}) erro
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeAlmAzureUpdate",
)
Expand All @@ -151,7 +151,7 @@ func resourceSonarqubeAlmAzureDelete(d *schema.ResourceData, m interface{}) erro
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAlmAzureDelete",
)
Expand Down
8 changes: 4 additions & 4 deletions sonarqube/resource_sonarqube_alm_github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func resourceSonarqubeAlmGithubCreate(d *schema.ResourceData, m interface{}) err
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAlmGithubCreate",
)
Expand All @@ -108,7 +108,7 @@ func resourceSonarqubeAlmGithubRead(d *schema.ResourceData, m interface{}) error
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeAlmGithubRead",
)
Expand Down Expand Up @@ -153,7 +153,7 @@ func resourceSonarqubeAlmGithubUpdate(d *schema.ResourceData, m interface{}) err
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeAlmGithubUpdate",
)
Expand All @@ -175,7 +175,7 @@ func resourceSonarqubeAlmGithubDelete(d *schema.ResourceData, m interface{}) err
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAlmGithubDelete",
)
Expand Down
8 changes: 4 additions & 4 deletions sonarqube/resource_sonarqube_alm_gitlab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func resourceSonarqubeAlmGitlabCreate(d *schema.ResourceData, m interface{}) err
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAlmGitlabCreate",
)
Expand All @@ -85,7 +85,7 @@ func resourceSonarqubeAlmGitlabRead(d *schema.ResourceData, m interface{}) error
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeAlmGitlabRead",
)
Expand Down Expand Up @@ -126,7 +126,7 @@ func resourceSonarqubeAlmGitlabUpdate(d *schema.ResourceData, m interface{}) err
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeAlmGitlabUpdate",
)
Expand All @@ -148,7 +148,7 @@ func resourceSonarqubeAlmGitlabDelete(d *schema.ResourceData, m interface{}) err
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAlmGitlabDelete",
)
Expand Down
6 changes: 3 additions & 3 deletions sonarqube/resource_sonarqube_azure_binding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func resourceSonarqubeAzureBindingCreate(d *schema.ResourceData, m interface{})
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAzureBindingCreate",
)
Expand Down Expand Up @@ -129,7 +129,7 @@ func resourceSonarqubeAzureBindingRead(d *schema.ResourceData, m interface{}) er
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeAzureBindingRead",
)
Expand Down Expand Up @@ -173,7 +173,7 @@ func resourceSonarqubeAzureBindingDelete(d *schema.ResourceData, m interface{})
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeAzureBindingDelete",
)
Expand Down
6 changes: 3 additions & 3 deletions sonarqube/resource_sonarqube_github_binding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func resourceSonarqubeGithubBindingCreate(d *schema.ResourceData, m interface{})
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeGithubBindingCreate",
)
Expand Down Expand Up @@ -119,7 +119,7 @@ func resourceSonarqubeGithubBindingRead(d *schema.ResourceData, m interface{}) e
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeGithubBindingRead",
)
Expand Down Expand Up @@ -161,7 +161,7 @@ func resourceSonarqubeGithubBindingDelete(d *schema.ResourceData, m interface{})
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeGithubBindingDelete",
)
Expand Down
6 changes: 3 additions & 3 deletions sonarqube/resource_sonarqube_gitlab_binding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func resourceSonarqubeGitlabBindingCreate(d *schema.ResourceData, m interface{})
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeGitlabBindingCreate",
)
Expand Down Expand Up @@ -101,7 +101,7 @@ func resourceSonarqubeGitlabBindingRead(d *schema.ResourceData, m interface{}) e
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeGitlabBindingRead",
)
Expand Down Expand Up @@ -142,7 +142,7 @@ func resourceSonarqubeGitlabBindingDelete(d *schema.ResourceData, m interface{})
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeGitlabBindingDelete",
)
Expand Down
8 changes: 4 additions & 4 deletions sonarqube/resource_sonarqube_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func resourceSonarqubeGroupCreate(d *schema.ResourceData, m interface{}) error {
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeGroupCreate",
)
Expand Down Expand Up @@ -99,7 +99,7 @@ func resourceSonarqubeGroupRead(d *schema.ResourceData, m interface{}) error {
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeGroupRead",
)
Expand Down Expand Up @@ -168,7 +168,7 @@ func resourceSonarqubeGroupUpdate(d *schema.ResourceData, m interface{}) error {
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeGroupUpdate",
)
Expand All @@ -191,7 +191,7 @@ func resourceSonarqubeGroupDelete(d *schema.ResourceData, m interface{}) error {
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeGroupDelete",
)
Expand Down
8 changes: 4 additions & 4 deletions sonarqube/resource_sonarqube_group_member.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func resourceSonarqubeGroupMemberCreate(d *schema.ResourceData, m interface{}) e
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeGroupMemberCreate",
)
Expand All @@ -93,7 +93,7 @@ func resourceSonarqubeGroupMemberRead(d *schema.ResourceData, m interface{}) err
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeGroupMemberRead",
)
Expand Down Expand Up @@ -141,7 +141,7 @@ func resourceSonarqubeGroupMemberDelete(d *schema.ResourceData, m interface{}) e
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusNoContent,
"resourceSonarqubeGroupMemberDelete",
)
Expand Down Expand Up @@ -181,7 +181,7 @@ func checkGroupMemberExists(groupName string, loginName string, m interface{}) (
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"checkGroupMemberExists",
)
Expand Down
6 changes: 3 additions & 3 deletions sonarqube/resource_sonarqube_new_code_periods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func resourceSonarqubeNewCodePeriodsCreate(d *schema.ResourceData, m interface{}
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeNewCodePeriodsCreate",
)
Expand Down Expand Up @@ -148,7 +148,7 @@ func resourceSonarqubeNewCodePeriodsRead(d *schema.ResourceData, m interface{})
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"GET",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeNewCodePeriodsRead",
)
Expand Down Expand Up @@ -197,7 +197,7 @@ func resourceSonarqubeNewCodePeriodsDelete(d *schema.ResourceData, m interface{}
resp, err := httpRequestHelper(
m.(*ProviderConfiguration).httpClient,
"POST",
sonarQubeURL.String(),
sonarQubeURL,
http.StatusOK,
"resourceSonarqubeNewCodePeriodsDelete",
)
Expand Down
Loading