fix: censor sensitive log messages #407

auvik-bheesham · 2021-08-13T01:07:23Z

When working with secrets and there's an error when applying/patching
the resource, we end up printing the full resource. The full resource
may contain secret material.

To prevent logging secret material we first check if we're about to add
a secret, if we are and we errror out then print a censored error
message.

For testing I used a secrets file which was incorrectly formatted:

$ cp controllers/testdata/sops/secret.yaml controllers/testdata/sops/secret.invalid.pgp.yaml
$ sops controllers/testdata/sops/secret.invalid.pgp.yaml
$ sops -d controllers/testdata/sops/secret.invalid.pgp.yaml
apiVersion: v1
kind: Secret
metadata:
    name: sops-invalid-pgp
stringData:
    secret: 0

Note: the secret: 0 bit is invalid: Kubernetes expects a string.

Before:

2021-08-12T20:53:21.349-0400    ERROR   controller-runtime.manager.controller.kustomization     Reconciliation failed after 580.457076ms, next try in 0s     {
"reconciler group": "kustomize.toolkit.fluxcd.io", "reconciler kind": "Kustomization", "name": "sops-d98cs", "namespace": "sops-dkgvw", "revision": "main/9db2
8d39cb66806297385330a532647516c7c814", "error": "apply failed: Error from server (BadRequest): error when creating \"0f1563ce-8273-4879-99dd-f6f58629cc2d.yaml
\": Secret in version \"v1\" cannot be handled as a Secret: v1.Secret.StringData: ReadString: expects \" or n, but found 0, error found in #10 byte of ...|\"s
ecret\":0}}\n|..., bigger context ...|\"namespace\":\"sops-dkgvw\"},\"stringData\":{\"secret\":0}}\n|...\n"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
        /Users/bheesham/dev/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:298
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
        /Users/bheesham/dev/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:253
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
        /Users/bheesham/dev/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:214
2021-08-12T20:53:21.349-0400    DEBUG   controller-runtime.manager.events       Normal  {"object": {"kind":"Kustomization","namespace":"sops-dkgvw","name":"so
ps-d98cs","uid":"0f1563ce-8273-4879-99dd-f6f58629cc2d","apiVersion":"kustomize.toolkit.fluxcd.io/v1beta1","resourceVersion":"371"}, "reason": "error", "messag
e": "apply failed: Error from server (BadRequest): error when creating \"0f1563ce-8273-4879-99dd-f6f58629cc2d.yaml\": Secret in version \"v1\" cannot be handl
ed as a Secret: v1.Secret.StringData: ReadString: expects \" or n, but found 0, error found in #10 byte of ...|\"secret\":0}}\n|..., bigger context ...|\"name
space\":\"sops-dkgvw\"},\"stringData\":{\"secret\":0}}\n|...\n"}

After:

2021-08-12T20:56:32.757-0400    ERROR   controller-runtime.manager.controller.kustomization     Reconciliation failed after 1.091874353s, next try in 0s     {
"reconciler group": "kustomize.toolkit.fluxcd.io", "reconciler kind": "Kustomization", "name": "sops-tcm92", "namespace": "sops-qz58f", "revision": "main/9db2
8d39cb66806297385330a532647516c7c814", "error": "contains sensitive material"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
        /Users/bheesham/dev/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:298
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
        /Users/bheesham/dev/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:253
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
        /Users/bheesham/dev/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:214
2021-08-12T20:56:32.757-0400    DEBUG   controller-runtime.manager.events       Normal  {"object": {"kind":"Kustomization","namespace":"sops-qz58f","name":"so
ps-tcm92","uid":"2989d7c3-b76f-4f15-b2f7-26a663fd00aa","apiVersion":"kustomize.toolkit.fluxcd.io/v1beta1","resourceVersion":"374"}, "reason": "error", "messag
e": "contains sensitive material"}

When working with secrets and there's an error when applying/patching the resource, we end up printing the full resource. The full resource may contain secret material. To prevent logging secret material we first check if we're about to add a secret, if we are and we errror out then print a censored error message. Signed-off-by: Bheesham Persaud <me@bheesham.com>

squaremo · 2021-08-17T07:42:51Z

First-time contributor high five ✋
I've enabled the CI, so we can see what the tests think.

stefanprodan · 2021-08-17T07:45:42Z

I'm wondering how people could more easily debug this if one of many (e.g. 1000+) objects in Git is malformed.

makkes · 2021-08-17T12:35:22Z

I agree that redaction of sensitive information would be a valuable addition to kustomize-controller, however I believe this PR is problematic as it may reduce the data available for diagnosing an issue with the resources that kustomize-controller tries to create. If there's 100 resources in the Kustomization and one of them is a secret, all you'll see is "contains sensitive material" even though only one of them causes this failure. You'll now have to find out which one yourself.

stefanprodan · 2021-08-17T12:52:22Z

IMO storing secrets in plain text in Git should never happen, using either SOPS or Sealed Secrets would prevent malformed secret yamls for reaching the repo as you can't encode them.

squaremo · 2021-08-17T13:13:55Z

using either SOPS or Sealed Secrets would prevent malformed secret yamls for reaching the repo as you can't encode them.

That may be true, but relying on it would not be wise. For one thing, an invalid field value of the kind SOPS would catch might not be the only reason the application fails.

auvik-bheesham · 2021-08-17T14:36:42Z

First-time contributor high five ✋
I've enabled the CI, so we can see what the tests think.

✋ Thanks :)

The tests are failing, though I can't reproduce locally. I'll look into this more.

using either SOPS or Sealed Secrets would prevent malformed secret yamls for reaching the repo as you can't encode them.

That may be true, but relying on it would not be wise. For one thing, an invalid field value of the kind SOPS would catch might not be the only reason the application fails.

That's not entirely true: the test YAML file I used was based on the PGP encrypted file and SOPS seemed happy with it. It seems while SOPS prevents invalid YAML it won't prevent K8s-unfriendly YAML. Though, my approach to using SOPS may be wrong.

The commands I added in my testing section aren't super accurate: specifically the cat bit. I'll modify it so it's sops -d. Apologies!

If there's 100 resources in the Kustomization and one of them is a secret, all you'll see is "contains sensitive material" even though only one of them causes this failure. You'll now have to find out which one yourself.

Good point! My approach is heavy-handed. Admittedly, I didn't even consider that kustomize can generate 100+ resources 😮‍💨 . I'll reconsider my approach.

Many thanks for the review, all!

As an aside: I'm heading out on vacation until Sep 7 so I'll be temporarily dropping this. This is being tracked with an internal ticket and I'll pick it back up when I'm back, if a teammate doesn't take over for me before then.

stefanprodan · 2021-08-17T14:47:10Z

@auvik-bheesham I think we need to parse the flux apply --dry-run output, determine if secrets Data or StringData are in the output and replace those with *****.

michalschott · 2021-09-02T07:23:32Z

I'm using notification controller in pair, today on slack I've observed:

kustomization/flux-system.flux-system
apply failed: 
Error from server (InternalError): error when applying patch:
{"stringData":{"XXX":"XXX"}}
to:
Resource: "/v1, Resource=secrets", GroupVersionKind: "/v1, Kind=Secret"
Name: "XXX", Namespace: "XXX"
for: "d0cb7c41-b7cc-453a-93dd-fa1d98256df4.yaml": Internal error occurred: rpc error: code = DeadlineExceeded desc = context deadline exceeded

Not sure if your patch will fix that as well or do we need something more to do on notification controller site as well?

auvik-bheesham · 2021-09-08T14:21:26Z

Not sure if your patch will fix that as well or do we need something more to do on notification controller site as well?

@michalschott That's not something I tested, so I can't say for certain. I like your approach in #420 more than the one I've tried here. Mind if I close this MR off and defer to yours instead?

stefanprodan · 2021-09-09T12:29:20Z

@auvik-bheesham #420 has been merged and it will part of the next flux release later today.

auvik-bheesham · 2021-09-09T13:24:20Z

@auvik-bheesham #420 has been merged and it will part of the next flux release later today.

Excellent! Thank you! :)

michalschott mentioned this pull request Sep 2, 2021

Secrets included in log messages and notifications #419

Closed

auvik-bheesham closed this Sep 9, 2021

auvik-bheesham deleted the flux2-logs-secrets branch September 9, 2021 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: censor sensitive log messages #407

fix: censor sensitive log messages #407

auvik-bheesham commented Aug 13, 2021 •

edited

Loading

squaremo commented Aug 17, 2021

stefanprodan commented Aug 17, 2021 •

edited

Loading

makkes commented Aug 17, 2021 •

edited

Loading

stefanprodan commented Aug 17, 2021

squaremo commented Aug 17, 2021

auvik-bheesham commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

michalschott commented Sep 2, 2021 •

edited

Loading

auvik-bheesham commented Sep 8, 2021

stefanprodan commented Sep 9, 2021

auvik-bheesham commented Sep 9, 2021

fix: censor sensitive log messages #407

fix: censor sensitive log messages #407

Conversation

auvik-bheesham commented Aug 13, 2021 • edited Loading

squaremo commented Aug 17, 2021

stefanprodan commented Aug 17, 2021 • edited Loading

makkes commented Aug 17, 2021 • edited Loading

stefanprodan commented Aug 17, 2021

squaremo commented Aug 17, 2021

auvik-bheesham commented Aug 17, 2021

stefanprodan commented Aug 17, 2021

michalschott commented Sep 2, 2021 • edited Loading

auvik-bheesham commented Sep 8, 2021

stefanprodan commented Sep 9, 2021

auvik-bheesham commented Sep 9, 2021

auvik-bheesham commented Aug 13, 2021 •

edited

Loading

stefanprodan commented Aug 17, 2021 •

edited

Loading

makkes commented Aug 17, 2021 •

edited

Loading

michalschott commented Sep 2, 2021 •

edited

Loading