From e3b2269e54bea893d0b37d0f43632a29e70dd66c Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Mon, 31 May 2021 17:27:17 +0530 Subject: [PATCH 1/3] Sanitize Telemetry data --- pkg/segment/segment.go | 60 ++++++++++--- pkg/segment/segment_test.go | 173 ++++++++++++++++++++++-------------- 2 files changed, 156 insertions(+), 77 deletions(-) diff --git a/pkg/segment/segment.go b/pkg/segment/segment.go index eb5e0837095..7ffc9e70e85 100644 --- a/pkg/segment/segment.go +++ b/pkg/segment/segment.go @@ -7,6 +7,7 @@ import ( "os" "os/user" "path/filepath" + "regexp" "runtime" "strconv" "strings" @@ -170,19 +171,20 @@ func SetError(err error) (errString string) { if err == nil { return "" } - // sanitize user information - user1, err1 := user.Current() - if err1 != nil { - return errors.Wrapf(err1, err1.Error()).Error() - } - errString = strings.ReplaceAll(err.Error(), user1.Username, Sanitizer) + errString = err.Error() + + // Sanitize user information + errString = sanitizeUserInfo(errString) + + // Sanitize file path + errString = sanitizeFilePath(errString) + + // Sanitize exec commands: For errors when a command exec fails in cases like odo exec or odo test, we do not want to know the command that the user executed, so we simply return + errString = sanitizeExec(errString) + + // Sanitize URL + errString = sanitizeURL(errString) - // sanitize file path - for _, str := range strings.Split(errString, " ") { - if strings.Count(str, string(os.PathSeparator)) > 1 { - errString = strings.ReplaceAll(errString, str, Sanitizer) - } - } return errString } @@ -216,3 +218,37 @@ func IsTelemetryEnabled(cfg *preference.PreferenceInfo) bool { } return false } + +// sanitizeUserInfo sanitizes username from the error string +func sanitizeUserInfo(errString string) string { + user1, err1 := user.Current() + if err1 != nil { + return errors.Wrapf(err1, err1.Error()).Error() + } + errString = strings.ReplaceAll(errString, user1.Username, Sanitizer) + return errString +} + +// sanitizeFilePath sanitizes file paths from error string +func sanitizeFilePath(errString string) string { + for _, str := range strings.Split(errString, " ") { + if strings.Count(str, string(os.PathSeparator)) > 1 { + errString = strings.ReplaceAll(errString, str, Sanitizer) + } + } + return errString +} + +// sanitizeURL sanitizes URLs from the error string +func sanitizeURL(errString string) string { + urlPattern, _ := regexp.Compile(`(http)\S*`) + errString = urlPattern.ReplaceAllString(errString, Sanitizer) + return errString +} + +// sanitizeExec sanitizes commands from the error string that might have been executed by users while running commands like odo test or odo exec +func sanitizeExec(errString string) string { + pattern, _ := regexp.Compile("exec command.*") + errString = pattern.ReplaceAllString(errString, fmt.Sprintf("exec command %s", Sanitizer)) + return errString +} diff --git a/pkg/segment/segment_test.go b/pkg/segment/segment_test.go index d67cbcc5943..8d1da2a22e9 100644 --- a/pkg/segment/segment_test.go +++ b/pkg/segment/segment_test.go @@ -193,71 +193,6 @@ func TestClientUploadWithConsent(t *testing.T) { } } -func TestSetError(t *testing.T) { - user, err := user.Current() - if err != nil { - t.Error(err.Error()) - } - unixPath := "/home/xyz/.odo/preference.yaml" - windowsPath := "C:\\User\\XYZ\\preference.yaml" - - tests := []struct { - name string - err error - hasPII bool - }{ - { - name: "no PII information", - err: errors.New("this is an error string"), - hasPII: false, - }, - { - name: "username", - err: fmt.Errorf("cannot create component name with %s", user.Username), - hasPII: true, - }, - { - name: "filepath-unix", - err: fmt.Errorf("cannot find the preference file at %s", unixPath), - hasPII: true, - }, - { - name: "filepath-windows", - err: fmt.Errorf("cannot find the preference file at %s", windowsPath), - hasPII: true, - }, - } - - for _, tt := range tests { - if tt.name == "filepath-windows" && os.Getenv("GOOS") != "windows" { - t.Skip("Cannot run windows test on a unix system") - } else if tt.name == "filepath-unix" && os.Getenv("GOOS") != "linux" { - t.Skip("Cannot run unix test on a windows system") - } - var want string - got := SetError(tt.err) - - // if error has PII, string returned by SetError must not be the same as the error since it was sanitized - // else it will be the same. - if (tt.hasPII && got == tt.err.Error()) || (!tt.hasPII && got != tt.err.Error()) { - if tt.hasPII { - switch tt.name { - case "username": - want = strings.ReplaceAll(tt.err.Error(), user.Username, Sanitizer) - case "filepath-unix": - want = strings.ReplaceAll(tt.err.Error(), unixPath, Sanitizer) - case "filepath-windows": - want = strings.ReplaceAll(tt.err.Error(), windowsPath, Sanitizer) - default: - } - t.Errorf("got: %q, want: %q", got, want) - } else { - t.Errorf("got: %s, want: %s", got, tt.err.Error()) - } - } - } -} - func TestIsTelemetryEnabled(t *testing.T) { tests := []struct { errMesssage, envVar string @@ -369,6 +304,114 @@ func TestClientUploadWithContext(t *testing.T) { } } +func TestSetError(t *testing.T) { + user, err := user.Current() + if err != nil { + t.Error(err.Error()) + } + + tests := []struct { + err error + hasPII bool + }{ + { + err: errors.New("this is an error string"), + hasPII: false, + }, + { + err: fmt.Errorf("failed to execute devfile commands for component %s-comp. failed to Get https://my-cluster.project.local cannot run exec command [curl https://mycluster.domain.local -u foo -p password 123]", user.Username), + hasPII: true, + }, + } + + for _, tt := range tests { + var want string + got := SetError(tt.err) + + // if error has PII, string returned by SetError must not be the same as the error since it was sanitized + // else it will be the same. + if tt.hasPII { + want = fmt.Sprintf("failed to execute devfile commands for component %s-comp. failed to Get %s cannot run exec command %s", Sanitizer, Sanitizer, Sanitizer) + } else { + want = tt.err.Error() + } + if got != want { + t.Errorf("got: %q\nwant:%q", got, want) + } + + } +} + +func Test_sanitizeExec(t *testing.T) { + err := fmt.Errorf("unable to execute the run command: unable to exec command [curl -K localhost:8080 -u user1 -p pwd123]") + got := sanitizeExec(err.Error()) + want := fmt.Sprintf("unable to execute the run command: unable to exec command %s", Sanitizer) + if got != want { + t.Errorf("got: %q\nwant:%q", got, want) + } +} + +func Test_sanitizeURL(t *testing.T) { + cases := []error{ + fmt.Errorf("resource project validation check failed.: Get https://my-cluster.project.local request cancelled"), + fmt.Errorf("resource project validation check failed.: Get http://my-cluster.project.local request cancelled"), + } + + for _, err := range cases { + got := sanitizeURL(err.Error()) + want := fmt.Sprintf("resource project validation check failed.: Get %s request cancelled", Sanitizer) + if got != want { + t.Errorf("got: %q\nwant:%q", got, want) + } + } +} + +func Test_sanitizeFilePath(t *testing.T) { + unixPath := "/home/xyz/.odo/preference.yaml" + windowsPath := "C:\\User\\XYZ\\preference.yaml" + + cases := []struct { + name string + err error + }{ + { + name: "filepath-unix", + err: fmt.Errorf("cannot find the preference file at %s", unixPath), + }, + { + name: "filepath-windows", + err: fmt.Errorf("cannot find the preference file at %s", windowsPath), + }, + } + for _, tt := range cases { + if tt.name == "filepath-windows" && os.Getenv("GOOS") != "windows" { + t.Skip("Cannot run a windows test on a unix system") + } else if tt.name == "filepath-unix" && os.Getenv("GOOS") != "linux" { + t.Skip("Cannot run a unix test on a windows system") + } + + got := sanitizeFilePath(tt.err.Error()) + want := fmt.Sprintf("cannot find the preference file at %s", Sanitizer) + if got != want { + t.Errorf("got: %q\nwant:%q", got, want) + } + } +} + +func Test_sanitizeUserInfo(t *testing.T) { + user, err1 := user.Current() + if err1 != nil { + t.Error(err1.Error()) + } + + err := fmt.Errorf("cannot create component name with %s", user.Username) + got := sanitizeUserInfo(err.Error()) + want := fmt.Sprintf("cannot create component name with %s", Sanitizer) + if got != want { + t.Errorf("got: %q\nwant:%q", got, want) + } +} + // createConfigDir creates a mock filesystem func createConfigDir(t *testing.T) string { fs := filesystem.NewFakeFs() From 04a0467ca7c5023c2a4c304ce3d2c13d904d3384 Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Mon, 7 Jun 2021 13:25:55 +0530 Subject: [PATCH 2/3] Add more sanitization for IPv4 addresses and hostnames, add tests --- pkg/segment/segment.go | 8 +++++++- pkg/segment/segment_test.go | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/segment/segment.go b/pkg/segment/segment.go index 7ffc9e70e85..6cdb5cb95d4 100644 --- a/pkg/segment/segment.go +++ b/pkg/segment/segment.go @@ -241,7 +241,13 @@ func sanitizeFilePath(errString string) string { // sanitizeURL sanitizes URLs from the error string func sanitizeURL(errString string) string { - urlPattern, _ := regexp.Compile(`(http)\S*`) + // the following regex parses hostnames and ip addresses + // references - https://www.oreilly.com/library/view/regular-expressions-cookbook/9780596802837/ch07s16.html + // https://www.oreilly.com/library/view/regular-expressions-cookbook/9781449327453/ch08s15.html + urlPattern, err := regexp.Compile(`((https?|ftp|smtp)://)?((?:[0-9]{1,3}\.){3}[0-9]{1,3}(:([0-9]{1,5}))?|([a-z0-9]+(-[a-z0-9]+)*\.)+[a-z]{2,})`) + if err != nil { + return errString + } errString = urlPattern.ReplaceAllString(errString, Sanitizer) return errString } diff --git a/pkg/segment/segment_test.go b/pkg/segment/segment_test.go index 8d1da2a22e9..81df6444950 100644 --- a/pkg/segment/segment_test.go +++ b/pkg/segment/segment_test.go @@ -355,6 +355,9 @@ func Test_sanitizeURL(t *testing.T) { cases := []error{ fmt.Errorf("resource project validation check failed.: Get https://my-cluster.project.local request cancelled"), fmt.Errorf("resource project validation check failed.: Get http://my-cluster.project.local request cancelled"), + fmt.Errorf("resource project validation check failed.: Get http://192.168.0.1:6443 request cancelled"), + fmt.Errorf("resource project validation check failed.: Get 10.18.25.1 request cancelled"), + fmt.Errorf("resource project validation check failed.: Get www.sample.com request cancelled"), } for _, err := range cases { From 4fd10a2e77abc32e06216cbef780979ba905c1ab Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Tue, 8 Jun 2021 13:48:36 +0530 Subject: [PATCH 3/3] Add requested changes --- pkg/segment/segment.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/segment/segment.go b/pkg/segment/segment.go index 6cdb5cb95d4..9dee2b71751 100644 --- a/pkg/segment/segment.go +++ b/pkg/segment/segment.go @@ -223,7 +223,7 @@ func IsTelemetryEnabled(cfg *preference.PreferenceInfo) bool { func sanitizeUserInfo(errString string) string { user1, err1 := user.Current() if err1 != nil { - return errors.Wrapf(err1, err1.Error()).Error() + return err1.Error() } errString = strings.ReplaceAll(errString, user1.Username, Sanitizer) return errString