Handle edge cases for unsanitized secrets in diff.HideSecretData #551

itmustbejj · 2023-11-22T03:16:07Z

…ecretData

Background:
There are several edge cases where invalid Secrets passed to diff.HideSecretData are not sanitized because diff.NormalizeSecret will return prematurely when receiving an error from runtime.DefaultUnstructuredConverter.FromUnstructured. Upstream, this is impacting argo-cd which expects an error to be returned and not simply logged, which leads to them exposing the unsanitized secret in several locations in the logs and ui.

In the current test suite, there are two tests that are failing silently because these errors from runtime.DefaultUnstructuredConverter.FromUnstructured are not being handled. After returning errors from diff.NormalizeSecret, both of these tests started correctly failing, and my changes make them pass again. I've also included a new test that covers my original edge case.

Before

➜ go test -v -run TestInvalidSecretStringData        
=== RUN   TestInvalidSecretStringData
E1121 19:08:57.562082  615354 diff.go:697]  "msg"="Failed to convert from unstructured into Secret" "error"="cannot convert int64 to string" 
--- PASS: TestInvalidSecretStringData (0.00s)
PASS
ok  	github.com/argoproj/gitops-engine/pkg/diff	0.014s

➜ go test -v -run TestHideSecretDataHandleEmptySecret
=== RUN   TestHideSecretDataHandleEmptySecret
E1121 19:09:00.614625  616145 diff.go:697]  "msg"="Failed to convert from unstructured into Secret" "error"="error decoding from json: illegal base64 data at input byte 12" 
--- PASS: TestHideSecretDataHandleEmptySecret (0.00s)
PASS
ok  	github.com/argoproj/gitops-engine/pkg/diff	0.015s

After returning errors in diff.NormalizeSecret

➜ go test -v -run TestInvalidSecretStringData        
=== RUN   TestInvalidSecretStringData
    diff_test.go:151: 
        	Error Trace:	/home/jhud/repos/gitops-engine/pkg/diff/diff_test.go:151
        	            				/home/jhud/repos/gitops-engine/pkg/diff/diff_test.go:687
        	Error:      	Received unexpected error:
        	            	Failed to convert from unstructured into Secret: cannot convert int64 to string
        	Test:       	TestInvalidSecretStringData
--- FAIL: TestInvalidSecretStringData (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x148fe96]

goroutine 13 [running]:
testing.tRunner.func1.2({0x15ac680, 0x296c790})
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1396 +0x24e
testing.tRunner.func1()
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1399 +0x39f
panic({0x15ac680, 0x296c790})
	/home/jhud/.asdf/installs/golang/1.19/go/src/runtime/panic.go:884 +0x212
github.com/argoproj/gitops-engine/pkg/diff.TestInvalidSecretStringData(0x408559?)
	/home/jhud/repos/gitops-engine/pkg/diff/diff_test.go:688 +0x176
testing.tRunner(0xc000484340, 0x188c5e0)
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1493 +0x35f
exit status 2
FAIL	github.com/argoproj/gitops-engine/pkg/diff	0.019s

➜ go test -v -run TestHideSecretDataHandleEmptySecret
=== RUN   TestHideSecretDataHandleEmptySecret
    diff_test.go:1029: 
        	Error Trace:	/home/jhud/repos/gitops-engine/pkg/diff/diff_test.go:1029
        	Error:      	Received unexpected error:
        	            	Failed to convert from unstructured into Secret: error decoding from json: illegal base64 data at input byte 12
        	Test:       	TestHideSecretDataHandleEmptySecret
    diff_test.go:1030: 
        	Error Trace:	/home/jhud/repos/gitops-engine/pkg/diff/diff_test.go:1030
        	Error:      	Expected value not to be nil.
        	Test:       	TestHideSecretDataHandleEmptySecret
    diff_test.go:1031: 
        	Error Trace:	/home/jhud/repos/gitops-engine/pkg/diff/diff_test.go:1031
        	Error:      	Expected value not to be nil.
        	Test:       	TestHideSecretDataHandleEmptySecret
--- FAIL: TestHideSecretDataHandleEmptySecret (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x14935ea]

goroutine 13 [running]:
testing.tRunner.func1.2({0x15ac680, 0x296c790})
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1396 +0x24e
testing.tRunner.func1()
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1399 +0x39f
panic({0x15ac680, 0x296c790})
	/home/jhud/.asdf/installs/golang/1.19/go/src/runtime/panic.go:884 +0x212
github.com/argoproj/gitops-engine/pkg/diff.TestHideSecretDataHandleEmptySecret(0x10?)
	/home/jhud/repos/gitops-engine/pkg/diff/diff_test.go:1032 +0x16a
testing.tRunner(0xc0004804e0, 0x188c5b0)
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	/home/jhud/.asdf/installs/golang/1.19/go/src/testing/testing.go:1493 +0x35f
exit status 2
FAIL	github.com/argoproj/gitops-engine/pkg/diff	0.019s

With PR fixes

➜ go test -v -run TestInvalidSecretStringData        
=== RUN   TestInvalidSecretStringData
--- PASS: TestInvalidSecretStringData (0.00s)
PASS
ok  	github.com/argoproj/gitops-engine/pkg/diff	0.013s

➜ go test -v -run TestHideSecretDataHandleEmptySecret
=== RUN   TestHideSecretDataHandleEmptySecret
--- PASS: TestHideSecretDataHandleEmptySecret (0.00s)
PASS
ok  	github.com/argoproj/gitops-engine/pkg/diff	0.015s

➜ go test -v -run TestHideSecretStringDataInvalidValues
=== RUN   TestHideSecretStringDataInvalidValues
--- PASS: TestHideSecretStringDataInvalidValues (0.00s)
PASS
ok  	github.com/argoproj/gitops-engine/pkg/diff	0.015s

ash2k

I'm not a maintainer and I cannot comment on the correctness of changes. I've made some minor comments.

ash2k · 2023-11-22T03:20:35Z

pkg/diff/diff.go

@@ -682,20 +682,31 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) {

 // NormalizeSecret mutates the supplied object and encodes stringData to data, and converts nils to
 // empty strings. If the object is not a secret, or is an invalid secret, then returns the same object.
-func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) {
+func NormalizeSecret(un *unstructured.Unstructured, opts ...Option) (error) {


Since you are making a breaking change to the function signature, may as well remove the now unused options 🤷

ash2k · 2023-11-22T03:22:42Z

pkg/diff/diff.go

@@ -682,20 +682,31 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) {

 // NormalizeSecret mutates the supplied object and encodes stringData to data, and converts nils to
 // empty strings. If the object is not a secret, or is an invalid secret, then returns the same object.


I think the doc needs to be updated here.

sonarcloud · 2023-11-22T19:05:07Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

codecov · 2023-11-22T19:06:20Z

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 54.51%. Comparing base (5fd9f44) to head (8f851f1).

❗ Current head 8f851f1 differs from pull request most recent head f6aaa0e. Consider uploading reports for the commit f6aaa0e to get more accurate results

Files	Patch %	Lines
pkg/diff/diff.go	40.42%	20 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   54.71%   54.51%   -0.21%     
==========================================
  Files          41       41              
  Lines        4834     4674     -160     
==========================================
- Hits         2645     2548      -97     
+ Misses       1977     1921      -56     
+ Partials      212      205       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ecretData Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

…lls that would fail silently Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

sonarcloud · 2024-03-30T19:03:19Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
2.7% Duplication on New Code

See analysis details on SonarCloud

itmustbejj changed the title ~~Fix/diff normalize invalid secret~~ Handle edge cases for unsanitized secrets from diff.HideSecretData Nov 22, 2023

itmustbejj changed the title ~~Handle edge cases for unsanitized secrets from diff.HideSecretData~~ Handle edge cases for unsanitized secrets in diff.HideSecretData Nov 22, 2023

ash2k reviewed Nov 22, 2023

View reviewed changes

itmustbejj force-pushed the fix/diff-normalize-invalid-secret branch 4 times, most recently from f9415b6 to 8f851f1 Compare November 22, 2023 19:04

itmustbejj added 7 commits March 30, 2024 12:02

Fix edge case in diff.NormalizeSecret that silently breaks diff.HideS…

2aaa360

…ecretData Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

Return an error from diff.NormalizeSecret to handle HideSecretData ca…

4de0eab

…lls that would fail silently Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

Fix bad text fixture data in getLiveSecretJsonBytes

c97a860

Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

Add error return value for diff.Normalize

6888b28

Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

Formatting

73c233f

Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

Improve cognitive complexity of diff.NormalizeSecret

f72530c

Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

Handle diff.Normalize() retval in function calls

f6aaa0e

Signed-off-by: Josh Hudson <382062+itmustbejj@users.noreply.github.com>

itmustbejj force-pushed the fix/diff-normalize-invalid-secret branch from 8f851f1 to f6aaa0e Compare March 30, 2024 19:03

itmustbejj requested a review from ash2k March 30, 2024 19:03

ash2k removed their request for review April 20, 2024 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle edge cases for unsanitized secrets in diff.HideSecretData #551

Handle edge cases for unsanitized secrets in diff.HideSecretData #551

itmustbejj commented Nov 22, 2023 •

edited

Loading

ash2k left a comment

ash2k Nov 22, 2023

ash2k Nov 22, 2023

sonarcloud bot commented Nov 22, 2023

codecov bot commented Nov 22, 2023 •

edited

Loading

sonarcloud bot commented Mar 30, 2024

		@@ -682,20 +682,31 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) {

		// NormalizeSecret mutates the supplied object and encodes stringData to data, and converts nils to
		// empty strings. If the object is not a secret, or is an invalid secret, then returns the same object.

Handle edge cases for unsanitized secrets in diff.HideSecretData #551

Are you sure you want to change the base?

Handle edge cases for unsanitized secrets in diff.HideSecretData #551

Conversation

itmustbejj commented Nov 22, 2023 • edited Loading

Before

After returning errors in diff.NormalizeSecret

With PR fixes

ash2k left a comment

Choose a reason for hiding this comment

ash2k Nov 22, 2023

Choose a reason for hiding this comment

ash2k Nov 22, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Nov 22, 2023

codecov bot commented Nov 22, 2023 • edited Loading

Codecov Report

sonarcloud bot commented Mar 30, 2024

Quality Gate passed

itmustbejj commented Nov 22, 2023 •

edited

Loading

codecov bot commented Nov 22, 2023 •

edited

Loading