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

Configure SAs to enforce mountable secrets #3064

Merged
merged 2 commits into from
May 16, 2024

Conversation

skitt
Copy link
Member

@skitt skitt commented May 14, 2024

This prevents accessing arbitrary secrets in pods running with these SAs. See
https://kubernetes.io/docs/reference/labels-annotations-taints/#enforce-mountable-secrets for details.

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr3064/skitt/enforce-mountable-secrets
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@skitt skitt force-pushed the enforce-mountable-secrets branch 5 times, most recently from 0b4ed3e to cfdfb99 Compare May 15, 2024 11:44
@skitt skitt marked this pull request as ready for review May 15, 2024 11:44
@skitt
Copy link
Member Author

skitt commented May 15, 2024

Verified by submariner-io/subctl#1128

@skitt skitt requested a review from dfarrell07 May 15, 2024 11:51
kubernetes.io/enforce-mountable-secrets: "true"
secrets:
- name: submariner-broker-secret
- name: submariner-ipsec-psk
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you're listing mounted secrets here but not for the others. How come?

Copy link
Member Author

@skitt skitt May 15, 2024

Choose a reason for hiding this comment

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

I think this is the only SA that mounts secrets other than the Kubernetes-provisioned secret (which is allowed by default).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, no, the Lighthouse agent does this too...

Copy link
Contributor

@tpantelis tpantelis May 15, 2024

Choose a reason for hiding this comment

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

So if specific secrets are not specified, then this will only allow the default K8s-provisioned secret to be mounted?

I wonder how it works with automountServiceAccountToken: false ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that’s orthogonal: this setting determines which secrets are allowed to be mounted, the automount setting determines whether the default provisioned secret is mounted automatically.

@skitt skitt force-pushed the enforce-mountable-secrets branch from cfdfb99 to 8ad86d4 Compare May 15, 2024 14:34
This prevents accessing arbitrary secrets in pods running with these
SAs. See
https://kubernetes.io/docs/reference/labels-annotations-taints/#enforce-mountable-secrets
for details.

Signed-off-by: Stephen Kitt <[email protected]>
@skitt skitt force-pushed the enforce-mountable-secrets branch from 8ad86d4 to 4735cb9 Compare May 15, 2024 15:15
@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label May 15, 2024
@dfarrell07
Copy link
Member

Should this be backported?

@skitt
Copy link
Member Author

skitt commented May 16, 2024

Should this be backported?

I think it should, but only after thorough testing — in particular, it reverts submariner-io/subctl#884 so I want to make sure that’s not a problem.

@skitt skitt merged commit e0189ee into submariner-io:devel May 16, 2024
35 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr3064/skitt/enforce-mountable-secrets]

@skitt skitt deleted the enforce-mountable-secrets branch May 16, 2024 07:40
@skitt skitt added the backport This change requires a backport to eligible release branches label May 16, 2024
@dfarrell07
Copy link
Member

Should this be backported?

I think it should, but only after thorough testing — in particular, it reverts submariner-io/subctl#884 so I want to make sure that’s not a problem.

What's the plan for getting this testing for devel? Do we want to bump subctl and other repos to this operator version to consume the change?

@skitt
Copy link
Member Author

skitt commented May 16, 2024

What's the plan for getting this testing for devel? Do we want to bump subctl and other repos to this operator version to consume the change?

That’s already planned, submariner-io/subctl#1128

@dfarrell07
Copy link
Member

We were hoping to see this work in 0.18, including d/s, before backporting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport This change requires a backport to eligible release branches backport-handled ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants