-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api): redact secrets in logs. Fixes #8685 #9859
Changes from 7 commits
b1ba634
dc5e925
47fee7b
b8333af
9e3a1f5
0276b22
ac964fe
f35fa08
eedd376
7966c34
9a1a528
b4b5ece
4589605
b284fb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
apiVersion: argoproj.io/v1alpha1 | ||
kind: Workflow | ||
metadata: | ||
generateName: secrets- | ||
spec: | ||
entrypoint: print-secret | ||
templates: | ||
- name: print-secret | ||
container: | ||
image: argoproj/argosay:v2 | ||
args: [echo, "secret from env: $MYSECRETPASSWORD"] | ||
env: | ||
- name: MYSECRETPASSWORD | ||
valueFrom: | ||
secretKeyRef: | ||
name: test-secret | ||
key: testpassword |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import ( | |
"bufio" | ||
"context" | ||
"fmt" | ||
"os" | ||
"regexp" | ||
"sort" | ||
"strings" | ||
|
@@ -80,6 +81,14 @@ func WorkflowLogs(ctx context.Context, wfClient versioned.Interface, kubeClient | |
|
||
var podListOptions metav1.ListOptions | ||
|
||
// 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{}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want to list all the secrets. See #8534 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @terrytangyuan @anilkumar-pcs
but I think those feature make large burden on argo-workflows.... |
||
if err != nil { | ||
logCtx.WithField("err", err).Debugln("error in listing secrets") | ||
} | ||
|
||
// we add selector if cli specify the pod selector when using logs | ||
if req.GetSelector() != "" { | ||
podListOptions = metav1.ListOptions{LabelSelector: common.LabelKeyWorkflow + "=" + req.GetName() + "," + req.GetSelector()} | ||
|
@@ -165,6 +174,17 @@ func WorkflowLogs(ctx context.Context, wfClient versioned.Interface, kubeClient | |
} | ||
if rx.MatchString(content) { // this means we filter the lines in the server, but will still incur the cost of retrieving them from Kubernetes | ||
logCtx.WithFields(log.Fields{"timestamp": timestamp, "content": content}).Debug("Log line") | ||
|
||
// log redaction for secrets | ||
if secrets != nil && enablePodLogRedaction == "true" { | ||
for _, s := range secrets.Items { | ||
for _, v := range s.Data { | ||
if strings.Contains(content, string(v)) { | ||
content = strings.Replace(content, string(v), "[ redacted ]", -1) | ||
anilkumar-pcs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
} | ||
unsortedEntries <- logEntry{podName: podName, content: content, timestamp: timestamp} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be turned on by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I am not sure if we should turn on new features by default. WDYT? @sarabala1979 @alexec