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

Set seccomp profile to RuntimeDefault for calico-kube-controllers and calico-typha #6524

Merged

Conversation

dimityrmirchev
Copy link
Contributor

Description

We can enhance the securityContext of calico-node, calico-kube-controllers, calico-typha by explicitly adding a "RuntimeDefault" seccomp profile.

      securityContext:
        seccompProfile:
          type: RuntimeDefault

By default Kubernetes runs the pods with profile "Unconfined". Privileged containers always run with Unconfined profile (reference). Having said that this change will only affect calico-kube-controllers and calico-typha since all the containers in calico-node are ran as privileged. The specifying of the seccomp profile there acts as a guardrails against:

  1. Adding a new container and running it as Unconfined
  2. Removing privileged: true from a container definition and still running as Unconfined

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

The calico-node, calico-kube-controllers and calico-typha pods now run with `securityContext.seccompProfile.type=RuntimeDefault`.

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

@dimityrmirchev dimityrmirchev requested a review from a team as a code owner August 10, 2022 09:03
@marvin-tigera marvin-tigera added this to the Calico v3.25.0 milestone Aug 10, 2022
@marvin-tigera marvin-tigera added docs-pr-required Change is not yet documented release-note-required Change has user-facing impact (no matter how small) labels Aug 10, 2022
@CLAassistant
Copy link

CLAassistant commented Aug 10, 2022

CLA assistant check
All committers have signed the CLA.

@caseydavenport
Copy link
Member

/sem-approve

@caseydavenport
Copy link
Member

@coutinhop could you own reviewing this guy?

@dimityrmirchev
Copy link
Contributor Author

Friendly ping @caseydavenport @coutinhop

@mgleung mgleung modified the milestones: Calico v3.25.0, Calico v3.26.0 Jan 5, 2023
@mgleung mgleung added the docs-not-required Docs not required for this change label Mar 14, 2023
@marvin-tigera marvin-tigera removed the docs-pr-required Change is not yet documented label Mar 14, 2023
Copy link
Contributor

@mgleung mgleung left a comment

Choose a reason for hiding this comment

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

Hey @dimityrmirchev sorry for the delay for the review. I think this would be really good for us to have, but I'm not sure if adding this to all of the node daemonsets would work if they run as privileged. Have you tested these changes?

Also, we generate a lot of the manifests from our helm charts so a faster change might be to make the changes to those and then re-generate the manifests.

@@ -38,6 +38,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: {{include "nodeName" . }}
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding was that this won't make a difference for calico-node since it needs to run as privileged (though it doesn't seem to have the privileged flag set here for some reason). Does this make sense to have here?

@@ -4381,6 +4381,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: calico-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -258,6 +258,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: calico-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -4378,6 +4378,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: calico-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -4412,6 +4412,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: calico-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -4376,6 +4376,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: calico-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -337,6 +337,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: canal-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -4400,6 +4400,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: canal
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -4378,6 +4378,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: calico-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@@ -398,6 +398,9 @@ spec:
- effect: NoExecute
operator: Exists
serviceAccountName: calico-node
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@dimityrmirchev
Copy link
Contributor Author

Hi @mgleung and thanks for reviewing this PR!

I think this would be really good for us to have, but I'm not sure if adding this to all of the node daemonsets would work if they run as privileged.

Setting RuntimeDefault seccomp profile for calico-node will not actually change anything since the containers run as privileged (they will still have the Unconfined profile). The specifying of the seccomp profile for calico-node acts as guardrails against:

  • Adding a new container and running it as Unconfined
  • Removing privileged: true from a container definition and still running as Unconfined

Have you tested these changes?

I created a kind cluster with disabled CNI, then applied the regenerated ./manifests/calico.yaml and ./manifests/calico-typha.yaml. The calico pods started successfully and I did not observe any errors.

Also, we generate a lot of the manifests from our helm charts so a faster change might be to make the changes to those and then re-generate the manifests.

Yes, this is what I did in order to change the files located in the manifests folder.

@hjiawei
Copy link
Contributor

hjiawei commented Mar 27, 2023

Setting RuntimeDefault to SeccompProfile should be fine as Tigera Operator had a similar change recently in tigera/operator#2433.

@caseydavenport
Copy link
Member

/sem-approve

@marvin-tigera marvin-tigera removed release-note-required Change has user-facing impact (no matter how small) docs-not-required Docs not required for this change merge-when-ready labels Jun 18, 2024
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Jun 18, 2024
@caseydavenport
Copy link
Member

/sem-approve

@caseydavenport caseydavenport added the docs-not-required Docs not required for this change label Jun 18, 2024
@marvin-tigera marvin-tigera removed the docs-pr-required Change is not yet documented label Jun 18, 2024
@caseydavenport caseydavenport merged commit e11585c into projectcalico:master Jun 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required Docs not required for this change release-note-required Change has user-facing impact (no matter how small)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants