search

argoproj / gitops-engine

Democratizing GitOps
https://pkg.go.dev/github.com/argoproj/gitops-engine?tab=subdirectories
Apache License 2.0
1.7k stars 264 forks source link

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

Open itmustbejj opened 1 year ago

itmustbejj commented 1 year ago

…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
sonarcloud[bot] commented 1 year ago

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

codecov[bot] commented 1 year ago

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 ```diff @@ 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.

sonarcloud[bot] commented 8 months ago

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