gitops-engine icon indicating copy to clipboard operation
gitops-engine copied to clipboard
verified publisher icon argoproj

Handle edge cases for unsanitized secrets in diff.HideSecretData

Open itmustbejj opened this issue 2 years ago • 3 comments

…ecretData

Fixes: https://github.com/argoproj/argo-cd/issues/16193

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

itmustbejj avatar Nov 22 '23 03:11 itmustbejj

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Nov 22 '23 19:11 sonarqubecloud[bot]

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).

:exclamation: 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 :warning:
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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 22 '23 19:11 codecov[bot]

Quality Gate Passed 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

sonarqubecloud[bot] avatar Mar 30 '24 19:03 sonarqubecloud[bot]