Skip to content
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: restrict access to secrets #1009

Merged
merged 9 commits into from Apr 3, 2024

Conversation

jnonino
Copy link
Contributor

@jnonino jnonino commented Feb 21, 2024

What does this PR do?

This PR aims to allow to users to restrict the secrets accesible by Traefik on the Kubernetes cluster. For this reason, I have added a new field into RBAC configuration secretResourceNames. By default, that value is an empty list ([]) and the behaviour is the same as before this change, all secrets are accesible from Traefik.

When that field contains one or more secret names, then only those secrets are the ones accessible from Traefik.

Motivation

Access to secrets without limits raises a security concern and Traefik should have access only to the secrets it requires to work properly, for example TLS certificates stored in secrets and used by Traefik. All other secrets in the cluster should not be accessible.
This was raised in the issue 1006.

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

@jnonino jnonino changed the title feat: restric access to secrets feat: restrict access to secrets Feb 21, 2024
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. I would assume $ is required for safe null checks

traefik/templates/rbac/clusterrole.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @jnonino,

Thank you for your contribution.
The PR looks good. I'll test it.
In the mean time WDYT about renaming accessibleSecrets by secretResourceNames ?

@jnonino
Copy link
Contributor Author

jnonino commented Feb 23, 2024

Hi @darkweaver87, I'll change the name of the field. I was actually expecting feedback about it 😄

@jnonino
Copy link
Contributor Author

jnonino commented Feb 27, 2024

Linking with issue traefik/traefik#7097 in Traefik repo as the discussion is relevant to the change added in this PR.

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darkweaver87
Copy link
Contributor

@jnonino , it looks like our bot is not able to merge this PR because it can't rebase on master and push to your branch. Would you mind to rebase it ?

@mloiseleur
Copy link
Contributor

@jnonino any chance that you can rebase this PR ?

@jnonino
Copy link
Contributor Author

jnonino commented Apr 3, 2024

@jnonino any chance that you can rebase this PR ?

Done!!! Sorry for the delay

Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@traefiker traefiker merged commit 8fbae97 into traefik:master Apr 3, 2024
1 check passed
@jnonino jnonino deleted the restric-secrets-access branch April 3, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants