Secret data not redacted when rendering invalid Secret

[itmustbejj]

Describe the bug

When rendering an invalid Secret, ArgoCD will leak the sensitive stringData in both the error message and diff view.

To Reproduce

To reproduce, create a simple secret with arbitrary values, and then set a value as an integer instead of a string. You will get an error like this (with sensitive and identifying info omitted):

one or more objects failed to apply, reason: "" is invalid: patch: Invalid value: patch: Invalid value: "map[ ....]" cannot convert int64 to string.

Additionally, the resource and app diffs will show unredacted stringData, regardless of whether the original Secret used data or stringData.

Expected behavior

The secret's data should be redacted during a failed sync of an invalid resource in the same way that a successfully synced Secret resource has its data redacted in the Argo UI.

Screenshots

Version

$ argocd version
argocd: v2.8.6+113b538
  BuildDate: 2023-10-31T14:18:21Z
  GitCommit: 113b53859dbf01cee7f1abac255cb112ad30bd8b
  GitTreeState: clean
  GoVersion: go1.20.10
  Compiler: gc
  Platform: linux/amd64

[itmustbejj]

This is leaking secrets in the application controller's logs: time="2023-10-31T23:19:39Z" level=info msg="Sync operation to 740d9ad8abd553ee0a85da3e8a9a973e91822ed3 failed: one or more objects failed to apply, reason: \"\" is invalid: patch: Invalid value: \"map[metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\\\"apiVersion\\\":\\\"v1\\\",\\\"kind\\\":\\\"Secret\\\",\\\"metadata\\\":{\\\"annotations\\\":{},\\\"labels\\\":{\\\"argocd.argoproj.io/instance\\\":\\\"myapp\\\",\\\"myapp/app\\\":\\\"myapp\\\",\\\"myapp/env\\\":\\\"sandbox\\\",\\\"myapp/project\\\":\\\"myapp\\\"},\\\"name\\\":\\\"myapp\\\",\\\"namespace\\\":\\\"myapp\\\"},\\\"stringData\\\":{\\\"API_KEY\\\":\\\"NOTACTUALVALUE\\\",\\\"INVALID_SECRET\\\":0},\\\"type\\\":\\\"Opaque\\\"}\\n]] stringData:map[API_KEY:NOTACTUALVALUE INVALID_SECRET:0]]\": cannot convert int64 to string" application=myapp dest-namespace=myapp dest-server="https://kubernetes.default.svc" reason=OperationCompleted type=Warning

and

time="2023-09-26T15:47:45Z" level=info msg="Updating operation state. phase: Running -> Failed, message: 'one or more tasks are running' -> 'one or more objects failed to apply, reason: \"\" is invalid: patch: Invalid value: \"map[metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\\\"apiVersion\\\":\\\"v1\\\",\\\"kind\\\":\\\"Secret\\\",\\\"metadata\\\":{\\\"annotations\\\":{},\\\"labels\\\":{\\\"argocd.argoproj.io/instance\\\":\\\"myapp\\\",\\\"myapp/app\\\":\\\"myapp\\\",\\\"myapp/env\\\":\\\"sandbox\\\",\\\"myapp/project\\\":\\\"myapps\\\"},\\\"name\\\":\\\"myapp\\\",\\\"namespace\\\":\\\"myapps\\\"},\\\"stringData\\\":{\\\"API_KEY\\\":\\\"NOTACTUALVALUE\\\",\\\"INVALID_SECRET\\\":0},\\\"type\\\":\\\"Opaque\\\"}\\n]] stringData:map[API_KEY:NOTACTUALVALUE INVALID_SECRET:0]]\": cannot convert int64 to string'" application=argocd/myapp syncId=00417-gDhur

[itmustbejj]

I tracked this down to the diff package of gitops-engine. For some reason the error is not getting raised properly, which breaks logic in ArgoCD like this.

[christoffer-eide]

We have seen the same issue in our argocd (v2.8.6) installation. A dev added a new entry to an existing secret, and he put invalid base64 data in data. which caused the error:

error decoding from json: illegal base64 data at input byte 14 (retried 5 times).

The error message from argocd also included the last-applied-configuration annotation, which contains the decrypted secret values. We have configured slack notifications, so the decrypted secret was posted there :(

[jannfis]

While this definitely must (and will) be fixed, I just wanted to raise concern about using stringData in a resource stored in Git.

IMHO, stringData shouldn't be used in the resource representing the desired state, because it will never end up in the desired state. I was wondering, does a Secret which is stored in Git and that uses stringData will ever reach synced state? Because once applied to the cluster, values in stringData will be base64 encoded and stored in data, and stringData will be stripped from the resource.

[christoffer-eide]

Using stringData works fine as its normalized.

I just had a look at a secret that uses stringData in git. In the argocd UI, the stringData is not shown in the diff, only the data is shown.

[jannfis]

Thanks, I didn't know that yet :)

I just had a look at a secret that uses stringData in git. In the argocd UI, the stringData is not shown in the diff, only the data is shown.

Yep, makes sense if it's being normalized before.

[andrii-korotkov-verkada]

ArgoCD versions 2.10 and below have reached EOL. Can you upgrade and let us know if the issue is still present, please?