feat(api): redact secrets in logs. Fixes #8685 #9859

anilkumar-pcs · 2022-10-19T03:49:31Z

FIlter and mask secrets in workflow logs.

Logs when running examples/secrets.yaml workflow with no redaction

secrets-tdwm7: time="2022-10-19T12:47:54.251Z" level=info msg="capturing logs" argo=true
secrets-tdwm7: secret from env: S00perS3cretPa55word
secrets-tdwm7: secret from file: S00perS3cretPa55word
secrets-tdwm7: time="2022-10-19T12:47:55.263Z" level=info msg="sub-process exited" argo=true error="<nil>"

Logs when running examples/secrets.yaml workflow with redaction enabled

secrets-tdwm7: time="2022-10-19T12:47:54.251Z" level=info msg="capturing logs" [*********]=true
secrets-tdwm7: secret from env: [*********]
secrets-tdwm7: secret from file: [*********]
secrets-tdwm7: time="2022-10-19T12:47:55.263Z" level=info msg="sub-process exited" [*********]=true error="<nil>"

Please do not open a pull request until you have checked ALL of these:

Create the PR as draft .
Run make pre-commit -B to fix codegen and lint problems.
Sign-off your commits (otherwise the DCO check will fail).
Use a conventional commit message (otherwise the commit message check will fail).
"Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
Github checks are green.
Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

Signed-off-by: Anil Kumar <[email protected]>

anilkumar-pcs · 2022-10-19T13:24:35Z

@alexec @terrytangyuan Can one of you review the changes?

Signed-off-by: Anil Kumar <[email protected]>

alexec · 2022-10-19T17:00:59Z

This is cool. I wonder if it can be even smarter? E.g. auto-detect secrets using a regex?

anilkumar-pcs · 2022-10-19T18:02:55Z

@alexec while the idea to auto-detect the secrets is nice, secrets are something user configurable and they can be literally anything. I believe it would be difficult to have a regex to identify all possible secrets.

Do you have any references in mind? Please share your thoughts.

tczhao

I tested how Github Action hides the secret
from the result, I think this is exaclty how it's done.

string replace rather than any regex.
(e.g. if I echo "text same as secret", it will hide it, if I remove 1 char from it, it will show)

util/logs/workflow-logger.go

Signed-off-by: Anil Kumar <[email protected]>

terrytangyuan · 2022-10-21T12:22:18Z

docs/environment-variables.md

@@ -22,6 +22,7 @@ most users. Environment variables may be removed at any time.
 | `ARGO_AGENT_PATCH_RATE`                | `time.Duration`     | `DEFAULT_REQUEUE_TIME`                                                                      | Rate that the Argo Agent will patch the workflow task-set.                                                                                                                                                                                                               |
 | `ARGO_AGENT_CPU_LIMIT`                 | `resource.Quantity` | `100m`                                                                                      | CPU resource limit for the agent.                                                                                                                                                                                                                                        |
 | `ARGO_AGENT_MEMORY_LIMIT`              | `resource.Quantity` | `256m`                                                                                      | Memory resource limit for the agent.                                                                                                                                                                                                                                     |
+| `ARGO_REDACT_POD_LOGS`                 | `bool`              | `false`                                                                                      | Whether to redact pod logs to hide/mask secrets.                                                                                                                                                                                                                        |


I think this should be turned on by default.

Although I am not sure if we should turn on new features by default. WDYT? @sarabala1979 @alexec

terrytangyuan · 2022-10-21T12:25:10Z

util/logs/workflow-logger.go

+	// get env variable for pod logs redaction
+	enablePodLogRedaction := os.Getenv("ARGO_REDACT_POD_LOGS")
+	// get secrets for redaction
+	secrets, err := kubeClient.CoreV1().Secrets(req.GetNamespace()).List(ctx, metav1.ListOptions{})


I don't think we want to list all the secrets. See #8534

@terrytangyuan I can see the GetSecret method is available but it requires the service account name and we can only fetch the secrets associated with a service account using this method.

One approach is using Service account Lister to get all the service accounts and fetch the secrets of each SA using above mentioned GetSecret method. Although we might not have all the secrets with this approach.

Can you suggest any other alternate approaches?

@terrytangyuan @anilkumar-pcs
I'd like to resolve #8685 but I don't have any better idea then

list secrets and keep then in cache or queue

when command like "echo" or "export", check command if they include one of those secrets

hide secrets

but I think those feature make large burden on argo-workflows....
any suggestion on this??

Signed-off-by: Anil Kumar <[email protected]>

alexec

I don't think this is the right approach. It moves responsibly for secret redaction from the user to the operator, yet it is the user's fault secrets are being logged.

I'd be amenable to a redaction that was zero-configuration. Github Actions automatically redacts secrets for example.

anilkumar-pcs · 2022-10-29T16:21:55Z

@alexec I have introduced configuration as it was suggested in here

We can remove the env variable configuration and mask the secrets by default. WDYT?

Signed-off-by: Anil Kumar <[email protected]>

anilkumar-pcs · 2022-10-31T03:20:49Z

@alexec made changes to remove the env configuration and masking secrets by default.

alexec

Server should/does not have list secrets permission. So this should/will not work. Instead, I think some research into "how to mask secrets" in logs is needed.

stale · 2022-11-23T03:44:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

stale · 2022-12-31T22:24:58Z

This issue has been closed due to inactivity. Feel free to re-open if you still encounter this issue.

tooptoop4 · 2024-10-22T10:18:48Z

where @anilkumar-pcs

agilgur5 · 2024-10-22T20:58:35Z

where @anilkumar-pcs

Per above, this approach has security ramifications. This approach also only affects the logs API.
See #8685 (comment) for an alternative without those ramifications and that would encompass any output logs

anilkumar-pcs added 2 commits October 18, 2022 12:09

fix: Hide secrets in logs. Fixes argoproj#8685

b1ba634

Signed-off-by: Anil Kumar <[email protected]>

fix: Added tests. Fixes argoproj#8685

dc5e925

Signed-off-by: Anil Kumar <[email protected]>

anilkumar-pcs changed the title ~~Hide secrets in logs~~ Hide secrets in logs. Fixes #8685 Oct 19, 2022

anilkumar-pcs force-pushed the hide-secrets-in-logs branch from dfbc009 to dc5e925 Compare October 19, 2022 09:42

anilkumar-pcs added 2 commits October 19, 2022 16:20

fix: retrigger checks

47fee7b

Signed-off-by: Anil Kumar <[email protected]>

fix: retrigger checks

b8333af

Signed-off-by: Anil Kumar <[email protected]>

anilkumar-pcs marked this pull request as ready for review October 19, 2022 13:11

anilkumar-pcs added 3 commits October 19, 2022 19:18

fix: added right test yaml

9e3a1f5

Signed-off-by: Anil Kumar <[email protected]>

fix: retrigger checks

0276b22

Signed-off-by: Anil Kumar <[email protected]>

fix: retrigger checks

ac964fe

Signed-off-by: Anil Kumar <[email protected]>

anilkumar-pcs mentioned this pull request Oct 19, 2022

GitHub checks for PRs are not reliable -- flakey test(s) #9861

Closed

tczhao approved these changes Oct 20, 2022

View reviewed changes

util/logs/workflow-logger.go Outdated Show resolved Hide resolved

anilkumar-pcs added 2 commits October 21, 2022 17:42

fix: use * to hide the secrets

f35fa08

Signed-off-by: Anil Kumar <[email protected]>

fix: update test

eedd376

Signed-off-by: Anil Kumar <[email protected]>

terrytangyuan reviewed Oct 21, 2022

View reviewed changes

anilkumar-pcs added 4 commits October 21, 2022 21:32

fix: address review comments

7966c34

Signed-off-by: Anil Kumar <[email protected]>

fix: retrigger checks

9a1a528

Signed-off-by: Anil Kumar <[email protected]>

fix: retrigger checks

b4b5ece

Signed-off-by: Anil Kumar <[email protected]>

fix: retrigger checks

4589605

Signed-off-by: Anil Kumar <[email protected]>

alexec reviewed Oct 29, 2022

View reviewed changes

alexec changed the title ~~Hide secrets in logs. Fixes #8685~~ feat: Hide secrets in logs. Fixes #8685 Oct 30, 2022

feat: remove env configuration

b284fb5

Signed-off-by: Anil Kumar <[email protected]>

alexec requested changes Nov 7, 2022

View reviewed changes

stale bot added the problem/stale This has not had a response in some time label Nov 23, 2022

stale bot closed this Dec 31, 2022

or-shachar mentioned this pull request Jan 24, 2023

Hide secrets in logs #8685

Open

agilgur5 added area/server area/api Argo Server API solution/unimplemented This was not implemented, but may be in the future labels Feb 11, 2024

agilgur5 added type/security Security related solution/invalid This is incorrect. Also can be used for spam and removed problem/stale This has not had a response in some time solution/unimplemented This was not implemented, but may be in the future labels Oct 22, 2024

agilgur5 changed the title ~~feat: Hide secrets in logs. Fixes #8685~~ feat(api): redact secrets in logs. Fixes #8685 Oct 22, 2024

argoproj locked as resolved and limited conversation to collaborators Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): redact secrets in logs. Fixes #8685 #9859

feat(api): redact secrets in logs. Fixes #8685 #9859

anilkumar-pcs commented Oct 19, 2022 •

edited

Loading

anilkumar-pcs commented Oct 19, 2022

alexec commented Oct 19, 2022

anilkumar-pcs commented Oct 19, 2022

tczhao left a comment

terrytangyuan Oct 21, 2022

terrytangyuan Oct 21, 2022

terrytangyuan Oct 21, 2022 •

edited

Loading

anilkumar-pcs Oct 25, 2022

da-head0 Sep 2, 2023

alexec left a comment

anilkumar-pcs commented Oct 29, 2022

anilkumar-pcs commented Oct 31, 2022

alexec left a comment

stale bot commented Nov 23, 2022

stale bot commented Dec 31, 2022

tooptoop4 commented Oct 22, 2024

agilgur5 commented Oct 22, 2024 •

edited

Loading

feat(api): redact secrets in logs. Fixes #8685 #9859

feat(api): redact secrets in logs. Fixes #8685 #9859

Conversation

anilkumar-pcs commented Oct 19, 2022 • edited Loading

anilkumar-pcs commented Oct 19, 2022

alexec commented Oct 19, 2022

anilkumar-pcs commented Oct 19, 2022

tczhao left a comment

Choose a reason for hiding this comment

terrytangyuan Oct 21, 2022

Choose a reason for hiding this comment

terrytangyuan Oct 21, 2022

Choose a reason for hiding this comment

terrytangyuan Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

anilkumar-pcs Oct 25, 2022

Choose a reason for hiding this comment

da-head0 Sep 2, 2023

Choose a reason for hiding this comment

alexec left a comment

Choose a reason for hiding this comment

anilkumar-pcs commented Oct 29, 2022

anilkumar-pcs commented Oct 31, 2022

alexec left a comment

Choose a reason for hiding this comment

stale bot commented Nov 23, 2022

stale bot commented Dec 31, 2022

tooptoop4 commented Oct 22, 2024

agilgur5 commented Oct 22, 2024 • edited Loading

anilkumar-pcs commented Oct 19, 2022 •

edited

Loading

terrytangyuan Oct 21, 2022 •

edited

Loading

agilgur5 commented Oct 22, 2024 •

edited

Loading