Repository: tetratelabs/getenvoy
Pull request #63 (2019-09-10 18:20:52)

Issue #1703 - envoy config dump - data obfuscation
# Task Accomplished
- struct `Filter` that describes elements for filtering and hashing of logs
- `process` method for `Filter` that processes logs based on the specific `Filter` instance
- package level function `ProcessLogs` that filters logs with a default `Filter` instance
- test cases for the newly added lines

# Todos
- salting the hash
- adding checks on the fields of logs using regex


lizan: @jhengy re: lint error, run `go mod tidy` once and commit the change. then you will see real issues:

```
pkg/binary/envoy/debug/pii_test.go:72:41: Using the variable on range scope `tc` in function literal (scopelint)
                        actualLogs, actualErr := ProcessLogs(tc.logs, tc.format, tc.containsPII)
                                                             ^
pkg/binary/envoy/debug/pii_test.go:75:24: Using the variable on range scope `tc` in function literal (scopelint)
                        for i, log := range tc.want.logs {
                                            ^
pkg/binary/envoy/debug/pii_test.go:82:9: Using the variable on range scope `tc` in function literal (scopelint)
                                        if tc.containsPII[fieldName] {
                                           ^
pkg/binary/envoy/debug/pii_test.go:90:5: Using the variable on range scope `tc` in function literal (scopelint)
                                tc.want.logs[i] = shell.Join(fields)
                                ^
pkg/binary/envoy/debug/pii_test.go:93:26: Using the variable on range scope `tc` in function literal (scopelint)
                        errorDiff := cmp.Diff(tc.want.err, actualErr)
                                              ^
pkg/binary/envoy/debug/pii_test.go:94:25: Using the variable on range scope `tc` in function literal (scopelint)
                        logsDiff := cmp.Diff(tc.want.logs, actualLogs)
                                             ^
pkg/binary/envoy/debug/pii_test.go:91:5: commentFormatting: put a space between `//` and comment text (gocritic)
                                //tc.want.logs[i] = "'" + log + "'" // adding single quote to the test case
                                ^
pkg/binary/envoy/debug/pii.go:60: G104: Errors unhandled. (gosec)
        h.Write([]byte(s)) //nolint: errcheck
pkg/binary/envoy/debug/pii_test.go:25: line is 544 characters (lll)
        var FORMAT_STRING string = "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%\" %RESPONSE_CODE% %RESPONSE_FLAGS% \"%DYNAMIC_METADATA(istio.mixer:status)%\" \"%UPSTREAM_TRANSP
ORT_FAILURE_REASON%\" %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HO
ST%\" %UPSTREAM_CLUSTER% %UPSTREAM_LOCAL_ADDRESS% %DOWNSTREAM_LOCAL_ADDRESS% %DOWNSTREAM_REMOTE_ADDRESS% %REQUESTED_SERVER_NAME%\n"
pkg/binary/envoy/debug/pii_test.go:32: line is 442 characters (lll)
        var VALID_LOG string = `[2019-09-05T17:53:36.908Z] "GET /productpage HTTP/1.1" 200 - "-" "-" 0 5183 47 46 "10.28.2.1" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.
3809.132 Safari/537.36" "4273c09c-2534-962a-b7da-04a04cf39e88" "35.199.144.198" "127.0.0.1:9080" inbound|9080|http|productpage.default.svc.cluster.local - 10.28.2.27:9080 10.28.2.1:0 outbound_.9080_._.productpa
ge.default.svc.cluster.local`
pkg/binary/envoy/debug/pii.go:24:1: comment on exported function `ProcessLogs` should be of the form `ProcessLogs ...` (golint)
// process the array of logs by filtering it using format and containsPII, Hash the resulting array of logs and return the final array.
^
pkg/binary/envoy/debug/pii.go:57:1: comment on exported function `Hash` should be of the form `Hash ...` (golint)
// TODO: salt the Hash
^
pkg/binary/envoy/debug/pii_test.go:33:6: don't use ALL_CAPS in Go names; use CamelCase (golint)
        var LOG_WITH_ONE_FIELD string = `[2019-09-05T17:53:36.908Z]`
            ^
golangci-lint failed.
One or more linters failed.
```
ZackButcher: Otherwise the code looks great, this last piece is a stylistic one that keeps things testable but hides a bunch of the details from a typical user just trying to consume the interface.
ZackButcher: Ah, sorry - I lied a little bit: there's one other change we'll make in addition to introducing a `struct` to hang our state off of. We'll want to introduce a default, package level `Process` method. So for users there's two APIs: `pii.NewFilter().Process(logs)` and `pii.Process(logs)`. That should use the default PII filter and hash function, as well as the default Istio Envoy format string; 99% of the time that's all we'll need and the user's API is dead simple and can't be misused.
taiki45: @jhengy Just fyi, we should avoid force-pushing in this project? If it's on purpose, ignore my comment.
jhengy: > @jhengy Just fyi, we should avoid force-pushing in this project? If it's on purpose, ignore my comment.

Yes, I did it on purpose after I did a `git rebase` on my local branch. The normal `git push` does not work after I rebase my local branch to master in an attempt to solve the branch out of date error on this PR
ZackButcher: Oh, and one other thing: let's go ahead and introduce our `pii` package. Since this is private to our tool, it should go in the `pkg` directory (this is a common go convention: stuff for others to consume goes in a top level package, stuff that's library code for this codebase is tucked into `pkg`). So we should have a new directory `getenvoy/pkg/pii`.

Patch:
diff --git a/go.mod b/go.mod
index 4a33dc6..53eef20 100644
--- a/go.mod
+++ b/go.mod
@@ -3,6 +3,7 @@ module github.com/tetratelabs/getenvoy
 go 1.12
 
 require (
+	bitbucket.org/creachadair/shell v0.0.6
 	github.com/StackExchange/wmi v0.0.0-20190523213315-cbe66965904d // indirect
 	github.com/go-ole/go-ole v1.2.4 // indirect
 	github.com/gogo/protobuf v1.2.2-0.20190730201129-28a6bbf47e48
diff --git a/go.sum b/go.sum
index 92a35e9..36b32b7 100644
--- a/go.sum
+++ b/go.sum
@@ -1,3 +1,5 @@
+bitbucket.org/creachadair/shell v0.0.6 h1:reJflDbKqnlnqb4Oo2pQ1/BqmY/eCWcNGHrIUO8qIzc=
+bitbucket.org/creachadair/shell v0.0.6/go.mod h1:8Qqi/cYk7vPnsOePHroKXDJYmb5x7ENhtiFtfZq8K+M=
 cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 cloud.google.com/go v0.34.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 cloud.google.com/go v0.37.4/go.mod h1:NHPJ89PdicEuT9hdPXMROBD91xc5uRDxsMtSB16k7hw=
diff --git a/pkg/pii/pii.go b/pkg/pii/pii.go
new file mode 100644
index 0000000..38145ed
--- /dev/null
+++ b/pkg/pii/pii.go
@@ -0,0 +1,137 @@
+// Copyright 2019 Tetrate
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package pii
+
+import (
+	"crypto/sha256"
+	"fmt"
+
+	"bitbucket.org/creachadair/shell"
+
+	l "github.com/tetratelabs/log"
+)
+
+var logger = l.RegisterScope("pkg/pii", "filters log lines for PII data", 0)
+
+var (
+	defaultFilter, _ = Default()
+	istioFormat      = `[%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%"` +
+		` %RESPONSE_CODE% %RESPONSE_FLAGS% "%DYNAMIC_METADATA(istio.mixer:status)%" "%REQ(USER-AGENT)%"`
+	defaultPII = map[string]bool{
+		// keys without variables
+		`[%START_TIME%]`:                           true,
+		`[%BYTES_RECEIVED%]`:                       true,
+		`%PROTOCOL%`:                               true,
+		`%RESPONSE_CODE%`:                          true,
+		`%RESPONSE_CODE_DETAILS%`:                  true,
+		`%BYTES_SENT%`:                             true,
+		`%DURATION%`:                               true,
+		`%RESPONSE_DURATION%`:                      true,
+		`%RESPONSE_FLAGS%`:                         true,
+		`%RESPONSE_TX_DURATION%`:                   true,
+		`%ROUTE_NAME%`:                             true,
+		`%UPSTREAM_HOST%`:                          true,
+		`%UPSTREAM_CLUSTER%`:                       true,
+		`%UPSTREAM_LOCAL_ADDRESS%`:                 true,
+		`%UPSTREAM_TRANSPORT_FAILURE_REASON%`:      true,
+		`%DOWNSTREAM_REMOTE_ADDRESS%`:              true,
+		`%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%`: true,
+		`%DOWNSTREAM_LOCAL_ADDRESS%`:               true,
+		`%DOWNSTREAM_LOCAL_ADDRESS_WITHOUT_PORT%`:  true,
+		`%REQUESTED_SERVER_NAME%`:                  true,
+		`%DOWNSTREAM_LOCAL_URI_SAN%`:               true,
+		`%DOWNSTREAM_PEER_URI_SAN%`:                true,
+		`%DOWNSTREAM_LOCAL_SUBJECT%`:               true,
+		`%DOWNSTREAM_PEER_SUBJECT%`:                true,
+		`%DOWNSTREAM_PEER_ISSUER%`:                 true,
+		`%DOWNSTREAM_TLS_SESSION_ID%`:              true,
+		`%DOWNSTREAM_TLS_CIPHER%`:                  true,
+		`%DOWNSTREAM_TLS_VERSION%`:                 true,
+		`%DOWNSTREAM_PEER_FINGERPRINT_256%`:        true,
+		`%DOWNSTREAM_PEER_SERIAL%`:                 true,
+		`%DOWNSTREAM_PEER_CERT%`:                   true,
+		`%DOWNSTREAM_PEER_CERT_V_START%`:           true,
+		`%DOWNSTREAM_PEER_CERT_V_END%`:             true,
+		// keys with variables
+		`%REQ(X?Y):Z%`:                         true,
+		`%RESP(X?Y):Z%`:                        true,
+		`%TRAILER(X?Y):Z%`:                     true,
+		`%DYNAMIC_METADATA(NAMESPACE:KEY*):Z%`: true,
+	}
+)
+
+// Filter filters log fields using pii and modify all PII fields using f
+type Filter struct {
+	f      func(string) string
+	pii    map[string]bool
+	format []string
+}
+
+// NewFilter constructs a custom filter object
+func NewFilter(formatStr string, hash func(string) string, pii map[string]bool) (Filter, error) {
+	// splitting formats and handle error
+	format, ok := shell.Split(formatStr)
+	if !ok {
+		return Filter{}, fmt.Errorf("error in splitting format string: %s", format)
+	}
+	return Filter{f: hash, pii: pii, format: format}, nil
+}
+
+// Default creates a filter with default fields for istio
+func Default() (Filter, error) {
+	return NewFilter(istioFormat, defaultHash, defaultPII)
+}
+
+// Process logs and hash the filter, assumes that filter has valid fields
+func (f Filter) Process(logs []string) []string {
+	out := make([]string, 0, len(logs))
+	for _, log := range logs {
+		fieldValues, ok := shell.Split(log)
+		if !ok {
+			if logger.DebugEnabled() {
+				logger.Debugf("error splitting log, skipping: %s", log)
+			} else {
+				logger.Info("error splitting log, skipping")
+			}
+			continue
+		}
+
+		if len(fieldValues) == len(f.format) {
+			// pick the PII fields and Hash the fields
+			for j, name := range f.format {
+				// TODO: may require further parsing name to verify if it matches as a key in the map with variables
+				if f.pii[name] {
+					fieldValues[j] = f.f(fieldValues[j])
+				}
+			}
+			out = append(out, shell.Join(fieldValues))
+		}
+	}
+	return out
+}
+
+// Process process logs with the default filter
+// an empty array of logs and an error instance is returned in the event of an error
+func Process(logs []string) ([]string, error) {
+	return defaultFilter.Process(logs), nil
+}
+
+// defaultHash returns the hashed value of s using sha256 defaultHash function
+// TODO: salt the Hash
+func defaultHash(s string) string {
+	h := sha256.New()
+	_, _ = h.Write([]byte(s))
+	return string(h.Sum(nil))
+}


Test patch:
diff --git a/pkg/pii/pii_test.go b/pkg/pii/pii_test.go
new file mode 100644
index 0000000..bd89240
--- /dev/null
+++ b/pkg/pii/pii_test.go
@@ -0,0 +1,135 @@
+// Copyright 2019 Tetrate
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package pii
+
+import (
+	"reflect"
+	"strings"
+	"testing"
+)
+
+func TestProcess(t *testing.T) {
+	format := `[%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%"` +
+		` %RESPONSE_CODE% %RESPONSE_FLAGS% "%DYNAMIC_METADATA(istio.mixer:status)%" "%REQ(USER-AGENT)%"`
+
+	validLog := `[2019-09-05T17:53:36.908Z] "GET /productpage HTTP/1.1" 200 - "-"` +
+		` "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36"`
+	logWithOneField := `[2019-09-05T17:53:36.908Z]`
+
+	// test cases wrapped up in a map where key is the test case name and value is the struct
+	// defining the inputs and expecte outcome
+	tests := map[string]struct {
+		// input fields
+		in     []string
+		format string
+		pii    map[string]bool
+
+		// output fields
+		outLogs []string
+		outErr  string
+	}{
+		// positive test cases
+		// only return an array containing one valid log with [%START_TIME%] field hashed
+		"simple": {
+			in:     []string{validLog, logWithOneField},
+			format: format,
+			pii:    map[string]bool{"[%START_TIME%]": true},
+			// expected expression in "" replaced by ''
+			outLogs: []string{`pii 'GET /productpage HTTP/1.1' 200 - - 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko)` +
+				` Chrome/76.0.3809.132 Safari/537.36'`},
+			outErr: "",
+		},
+		"multiple_pii_fields": {
+			in:     []string{validLog, logWithOneField},
+			format: format,
+			pii: map[string]bool{
+				"[%START_TIME%]": true,
+				"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%": true,
+				"%RESPONSE_CODE%": true,
+			},
+			outLogs: []string{`pii pii pii - - 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36'`},
+			outErr:  "",
+		},
+
+		// negative test cases
+		"empty_logs": {
+			in:      []string{},
+			format:  format,
+			pii:     map[string]bool{"[%START_TIME%]": true},
+			outLogs: []string{},
+			outErr:  "",
+		},
+		"all_invalid_logs": {
+			in:      []string{"test", "test1", "test2"},
+			format:  format,
+			pii:     map[string]bool{"[%START_TIME%]": true},
+			outLogs: []string{},
+			outErr:  "",
+		},
+		"empty_format_string": {
+			in:      []string{validLog, logWithOneField},
+			format:  "",
+			pii:     map[string]bool{"[%START_TIME%]": true},
+			outLogs: []string{},
+			outErr:  "",
+		},
+		"empty_pii_map": {
+			in:     []string{validLog, logWithOneField},
+			format: format,
+			pii:    map[string]bool{},
+			outLogs: []string{`'[2019-09-05T17:53:36.908Z]' 'GET /productpage HTTP/1.1' 200 - - 'Mozilla/5.0 (X11; Linux x86_64)` +
+				` AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36'`},
+			outErr: "",
+		},
+		"invalid_pii_map": {
+			in:     []string{validLog, logWithOneField},
+			format: format,
+			pii:    map[string]bool{"test": true, "test2": true},
+			outLogs: []string{`'[2019-09-05T17:53:36.908Z]' 'GET /productpage HTTP/1.1' 200 - - 'Mozilla/5.0 (X11; Linux x86_64)` +
+				` AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36'`},
+			outErr: "",
+		},
+	}
+
+	piiFunc := func(string) string { return "pii" }
+
+	// defining subtest with t.Run
+	//nolint:gocritic,scopelint
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			filter, err := NewFilter(tc.format, piiFunc, tc.pii)
+			actual := filter.Process(tc.in)
+
+			if err != nil {
+				if tc.outErr == "" {
+					// expected no error but get error
+					t.Fatalf("ProcessLogs(%v, %v, %v, piiFunc) = %v, %v, expected no error", tc.in, tc.format, tc.pii, actual, err)
+				} else if !strings.Contains(err.Error(), tc.outErr) {
+					// actual error does not contain the expected error
+					t.Fatalf("ProcessLogs(%v, %v, %v, piiFunc) = %v, %v, but expected error: %v", tc.in, tc.format, tc.pii, actual, err, tc.outErr)
+				}
+			} else if tc.outErr != "" {
+				// expect error but no acutal error
+				t.Fatalf("ProcessLogs(%v, %v, %v, piiFunc) = %v, %v, but expected error: %v", tc.in, tc.format, tc.pii, actual, err, tc.outErr)
+			}
+
+			// always check if actual logs do match the expected logs
+			if !reflect.DeepEqual(actual, tc.outLogs) {
+				t.Fatalf("ProcessLogs(%v, %v, %v, piiFunc) = %v but expected output: %v", tc.in, tc.format, tc.pii, actual, tc.outLogs)
+			}
+
+		})
+	}
+}