Feature/credentials filter

[Useurmind]

Changes

This PR is related to https://github.com/tektoncd/pipeline/issues/3373. The issue was closed by your bot but the topic is still relevant to us.

It solves the problem that secret values are printed to the output log. All secrets contained in the namespace of the pod will be redacted. The pod needs a service account token with permissions to read out all secret in the namespace from the api server.

I added a credential filter to the runner app of tekton steps. It reads out all secrets from the namespace of the pod and redacts them from the output log.

This is currently a draft, I would be happy to polish it if you have interest in adopting this approach.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• [x] Docs included if any changes are user facing
• [ ] Tests included if any functionality added or changed
• [ ] Follows the commit message standard
• [x] Meets the Tekton contributor standards (including functionality, content, code)
• [x] Release notes block below has been filled in (if there are no user facing changes, use release note "NONE")

Release Notes

add credential filter that can be activated via a new feature flag `enable-logging-credentials-filter`

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: Useurmind / name: Jochen Grün (53d99afa80622e8353392816bf3e13fe39190ab8, 83db002c1bee0ae6901ef1e617b66278fb30fbc4)

[tekton-robot]

Hi @Useurmind. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Useurmind]

Any interest in this feature?

[afrittoli]

/ok-to-test

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/credentials/filter/credentials_detection.go	Do not exist	0.0%
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%

[afrittoli]

Thanks @Useurmind for your PR! I think there is interest in the feature, it may be something that we let user control via a feature flag? I'm sorry about the slow response - between KubeCon and cdCon it's a busy conference time :D

/cc @imjasonh @vdemeester

[Useurmind]

Then I will start implementing a feature toggle for it and will see that I add some tests.

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/credentials/filter/credentials_detection.go	Do not exist	0.0%
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_detection.go	Do not exist	0.0%
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_detection.go	Do not exist	0.0%
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%

[Useurmind]

So I started looking into this feature and implementing a feature flag for it. Currently I am not sure how a feature flag may be best implemented for this.

I have found the following points:

1. feature flag config map is only read and used in controller
2. but credential filtering is done in the entrypoint binary during task runtime
3. credentials filter needs a service account token that can read secrets and that is something which needs to be ensured when a pipeline is run

Point 1 and 2 could be solved handing down an argument or setting an environment variable to the entrypoint from the controller.

Point 3 probably must be solved by documenting the requirements for the credentials filter, so that pipeline creators can pay attention to it.

Does that approach make sense or is there an easier way?

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: Useurmind / name: Jochen Grün (80aea825045fd994d11b00c095f0aa65750a1547)

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	81.5%	65.9%	-15.6
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_detection.go	Do not exist	0.0%
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/pod/pod.go	88.3%	87.8%	-0.5

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	81.5%	63.4%	-18.1
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_detection.go	Do not exist	0.0%
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/pod/pod.go	88.3%	87.8%	-0.5

[imjasonh]

The pod needs a service account token with permissions to read out all secret in the namespace from the api server.

This part worries me a bit. Does this feature require that all TaskRun pods now have more permissions than before, to be able to tell what to redact? That seems valuable, but it also may mean that TaskRuns that, e.g., execute kubectl get secrets will now be able to get all those secrets, unless we take extra steps to stop them.

Could this somehow query "all accessible secrets" instead, and redact whatever it can find, instead of requiring new access to all secrets in its namespace?

I wonder if this could also lead to more load on the API server, since every TaskRun would start by fetching all secrets in its namespace, which for clusters running lots of TaskRuns could cause problems.

Not saying we shouldn't do this -- redacting secrets would be great -- just curious about some of the implications around limiting access and being mindful to reduce API traffic.

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	81.5%	65.9%	-15.6
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_detection.go	Do not exist	0.0%
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/pod/pod.go	88.3%	87.8%	-0.5

[Useurmind]

You are correct in your understanding Jason. The pods can only redact secrets that they know and have access to. And they will query the API server for this.

The current implementation uses the go-client to do so. I am not entirely sure but I suspect that the client will return only the secrets that the service account has access to. If not we can implement it in this way. That would mean if the service account has no access to secrets, he will not be able to redact them properly.

The performance implications are as you say. But as this is hidden behind a feature flag, it should not harm existing installations. If the feature is not enabled not communication to the api server takes place.

Btw. as far as I understand the permission concept in k8s a user can either see all secrets in a namespace or none. If you want to hide resources from a user only partially, you need to put them in different namespaces. Seeing can be limited to listing or reading them. But in the end its all or none.

[Useurmind]

Some more things to consider:

I would advice to extend this in the future so multiple secret detection algorithms can be used. Secrets have so many sources meanwhile that it becomes hard to configure one that works for everyone. This is a rather simple approach to read all secrets from the namespace. Others could be:

• Read all secrets referenced in environment variables of your pod
• Read all secrets referenced in volume mounts via csi drivers (this could depend on the csi secret provisioner)
• Read secrets from secret parameter values of the task itself
• ... more?

Should we consider implementing one of those? Then the pod would only need access to its own resource and could detect the secrets from files and environment variables. But I am not sure it is as complete as the approach above. It is also more complicated to implement.

[Useurmind]

I thought about the implenentation again over the weekend. If there is currently no contact between tekton pipeline run pods and the API server it would be a grave error to require this in the future. It makes the whole design of the pipelines more complicated and the behaviour less stable and harder to predict.

Therefore, I would suggest the following approach, similar to my previous post but not the same:

• The controller knows the created pods ressources exactly, he does not even need to contact the API server to read it
• The controller can extract information about which secrets are linked into the pod, examples (not the actual secrets, just the locations):
  • environment variables: based on the secret ref value in the pod
  • files: based on CSI providers and secret refs volumes
• This information is handed to the entrypoint which can extract the secrets from the locations and use them to redact them from the output

Much more complicated than the current implementation, but more in line with the current design and easier to reason about regarding the permissions because the pipeline pod will not need any new permissions.

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	77.8%	55.3%	-22.5
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/credentials/filter/secret_detection.go	Do not exist	42.0%
pkg/credentials/filter/secret_extraction.go	Do not exist	87.5%
pkg/pod/pod.go	88.3%	87.8%	-0.5

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	81.5%	61.4%	-20.1
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/credentials/filter/secret_detection.go	Do not exist	42.0%
pkg/credentials/filter/secret_extraction.go	Do not exist	87.5%
pkg/credentials/filter/secret_locations.go	Do not exist	0.0%
pkg/pod/pod.go	88.3%	86.4%	-1.9

[imjasonh]

Btw. as far as I understand the permission concept in k8s a user can either see all secrets in a namespace or none. If you want to hide resources from a user only partially, you need to put them in different namespaces. Seeing can be limited to listing or reading them. But in the end its all or none.

You can grant access to only certain Secrets in a namespace, e.g.:

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  namespace: my-ns
  name: only-this-secret-binding
rules:
- apiGroups: [""]
  resources: ["secrets"]
  resourceNames: ["only-this-secret"]
  verbs: ["get"]

In this case, I believe listing secrets for the namespace will only return only-this-secret, as you say, and like we want, so no additional permissions are required. It does mean that if the value of some-other-secret snuck into logs, it wouldn't be redacted, but I think that's fine and pretty well expected.

[...] This is a rather simple approach to read all secrets from the namespace. Others could be:

Should we consider implementing one of those? Then the pod would only need access to its own resource and could detect the secrets from files and environment variables. But I am not sure it is as complete as the approach above. It is also more complicated to implement.

I think what we have here is better than any more complicated solution that does more work to discover associated secrets. More complexity means more opportunity for bugs. 🐛

If there is currently no contact between tekton pipeline run pods and the API server it would be a grave error to require this in the future. It makes the whole design of the pipelines more complicated and the behaviour less stable and harder to predict. [...]

👍 Thanks for considering this! I think we should try to avoid contacting the API server if we can avoid it, but having the controller (that already knows secrets) pass them down to the entrypoint also has issues, namely, that the secret values must be passed via Pod spec, and now anybody who has pods.get permissions in that namespace can also read secrets 😢.

There are probably steps we could take to encrypt them and decrypt them using an entrypoint-only secret, but this sure is getting complex quickly 😅.

I'd be interested to hear what other folks think about this approach. @vdemeester @afrittoli @lbernick any concerns with just listing all accessible namespaced secrets, or do you think we should do more to discover them on the controller-side and securely pass them down?

[Useurmind]

Today I implemented the code to tell the pod where to find secrets, it has access to.

The controller looks for secret refs in volumes and env and also for CSI volumes referencing secrets. It saves the information in form of the names of environment variables and paths to files with secrets. The pod can then read those values and redact them.

I see opportunity for bugs but I also think it is simpler to configure and run.

This design does not transmit actual secrets. Only the places where the pod can find secrets it has attached to it. And those are the secrets that need to be redacted. Because only those can be used in the corresponding pipeline step.

What do you think of that?

[imjasonh]

The controller looks for secret refs in volumes and env and also for CSI volumes referencing secrets. It saves the information in form of the names of environment variables and paths to files with secrets. The pod can then read those values and redact them. [...] This design does not transmit actual secrets. Only the places where the pod can find secrets it has attached to it. And those are the secrets that need to be redacted. Because only those can be used in the corresponding pipeline step.

What do you think of that?

Oh gotcha, that's great, thanks for doing that!

So in this model, the entrypoint doesn't have to list secrets at startup, and only gets information from the controller about where to find secret values in env vars, directory paths, etc. That seems like the best possible way to do this. 👍

[Useurmind]

Yes the only thing remaining is the documentation and some testing, manually to convince myself and fixing one of the pipelines that does not work. Btw I am clueless what the problem of that pipeline is.

[imjasonh]

Btw I am clueless what the problem of that pipeline is.

That error is just (badly) telling you you need to gofmt that file. You can do that with gofmt -w pkg/credentials/filter/secret_extraction.go.

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	77.8%	59.1%	-18.7
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/credentials/filter/secret_detection.go	Do not exist	42.0%
pkg/credentials/filter/secret_extraction.go	Do not exist	87.5%
pkg/credentials/filter/secret_locations.go	Do not exist	0.0%
pkg/pod/pod.go	88.3%	86.4%	-1.9

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/runner.go	77.8%	61.4%	-16.4
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/credentials/filter/secret_detection.go	Do not exist	42.0%
pkg/credentials/filter/secret_extraction.go	Do not exist	87.5%
pkg/credentials/filter/secret_locations.go	Do not exist	0.0%
pkg/pod/pod.go	88.3%	86.4%	-1.9

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5

[tekton-robot]

The following is the coverage report on the affected files. Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	88.0%	86.5%	-1.5
pkg/credentials/filter/credentials_filter.go	Do not exist	100.0%
pkg/credentials/filter/ring_buffer.go	Do not exist	100.0%
pkg/credentials/filter/secret_detection.go	Do not exist	77.8%
pkg/credentials/filter/secret_extraction.go	Do not exist	87.5%
pkg/credentials/filter/secret_locations.go	Do not exist	0.0%
pkg/pod/pod.go	88.3%	86.4%	-1.9