Migrate to Seccomp profile in security Context ⚠️ #475

Ser87ch · 2022-09-02T08:09:09Z

Description

This PR changes seccomp auditor to scan seccompProfile instead of annotations as requested in #343
The following changes were done:

seccomp auditor changed to scan securityContext in pod and containers to find seccompProfile. ⚠️
New fix created in seccomp auditor to create seccomp profile in a pod and containers and to remove profile in containers. The auditor updated to use the new fix.
SeccompAnnotationMissing rule renamed to SeccompProfileMissing. ⚠️
SeccompDeprecatedContainer rule dropped. ⚠️
Tests updated to cover changes in auditor and fix.
Documentation updated.

Fixes #343

Type of change

This change requires a documentation update 📖
Breaking changes ⚠️

How Has This Been Tested?

Mostly by automated tests.
I've also executed kubeaudit for examples from documentation to update them.

Checklist:

I have 🎩 my changes (A 🎩 specifically includes pulling down changes, setting them up, and manually testing the changed features and potential side effects to make sure nothing is broken)
I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The test coverage did not decrease
I have signed the appropriate Contributor License Agreement

ghost · 2022-09-02T08:09:14Z

Thanks for opening this pull request! Please check out our contributing guidelines and sign the CLA.

Ser87ch · 2022-09-02T08:14:37Z

I've signed the CLA!

Ser87ch · 2022-09-06T06:58:51Z

@genevieveluyt , could you take a look at the PR please?

dani-santos-code · 2022-09-08T16:09:54Z

hi, @Ser87ch ! Thank you for your contribution! I will take a closer look at your PR either today or tomorrow. In the meantime, would you mind adding support to both annotations and seccompProfile?

Ser87ch · 2022-09-08T16:16:45Z

@dani-santos-code Thanks for your reply. May I ask, why? Seccomp annotation support will be removed quite soon.
Additionally, supporting both will add more complications, like what to do if we've got both security context and annotations set.

dani-santos-code · 2022-09-08T16:36:28Z

Hi, @Ser87ch, you asked a question about the possibility of supporting both which led me to believe that this would be an option you considered. I do understand that supporting both would be a more involved implementation. However, it would be preferable to be backwards compatible and not ship breaking changes and only ship the changes once it is effectively deprecated (as of k8s v.1.25, if I understand correctly?). We can open an issue for this.

We could check either for missing annotations or missing seccompProfile. We could add a warning about annotations to mention it will soon be deprecated?

if we find a valid seccompProfile, the auditor won't yield errors
if we don't find a valid seccompProfile but find seccomp annotations, the auditor will yield a warning about future deprecation
if we don't find either a valid seccompProfile nor a seccomp annotation, the auditor will yield an error

as a follow-up, once annotations are fully deprecated, we can remove the part that adds support to annotations.

let me know what you think and if this would be feasible! 🙏

Ser87ch · 2022-09-08T16:40:12Z

Thanks @dani-santos-code , I'll let you know tomorrow.

genevieveluyt · 2022-09-08T18:24:08Z

If the annotation is going to stop working and the security profile has already been available for some time, I think dropping annotation support is fair. +1 to @dani-santos-code's suggestion though, if it's not too much of a hassle.

I think ideally Fix would remove the seccomp annotation as well, if it's present.

Ser87ch · 2022-09-09T13:05:02Z

Thanks guys, I will implement your suggestions.

Ser87ch · 2022-09-12T17:39:46Z

I've encountered an issue, that I don't know how to fix yet. I'm testing a change that you suggested with the following pod spec:

apiVersion: v1
kind: Pod
metadata:
  name: pod
  namespace: seccomp-profile-missing-annotations
  annotations:
    seccomp.security.alpha.kubernetes.io/pod: runtime/default
    container.seccomp.security.alpha.kubernetes.io/container: localhost/bla
spec:
  containers:
    - name: container
      image: scratch

The problem is when I fetch security context the pod spec, it returns me a security context with seccomp profile set:

&PodSecurityContext{SELinuxOptions:nil,RunAsUser:nil,RunAsNonRoot:nil,SupplementalGroups:[],FSGroup:nil,RunAsGroup:nil,Sysctls:[]Sysctl{},WindowsOptions:nil,FSGroupChangePolicy:nil,SeccompProfile:&SeccompProfile{Type:RuntimeDefault,LocalhostProfile:nil,},}

I have a feeling that k8s library returns seccomp profile in security context even if only seccomp annotation set. If it's the case I'm not able to differentiate between seccomp set in security context and annotations.
I've committed this change, this test fails seccomp-profile-missing-disabled-container.yaml. Am I missing something?

Ser87ch · 2022-09-13T09:09:19Z

It's not a k8s library problem, it the way k8s API works. If I export the pod spec I mentioned in the previous comment by using kubectl get pod pod -n seccomp-profile-missing-annotations -o yaml, it returns:

apiVersion: v1
kind: Pod
metadata:
  annotations:
    container.seccomp.security.alpha.kubernetes.io/container: localhost/bla
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","kind":"Pod","metadata":{"annotations":{"container.seccomp.security.alpha.kubernetes.io/container":"localhost/bla","seccomp.security.alpha.kubernetes.io/pod":"runtime/default"},"name":"pod","namespace":"seccomp-profile-missing-annotations"},"spec":{"containers":[{"image":"scratch","name":"container"}]}}
    seccomp.security.alpha.kubernetes.io/pod: runtime/default
  creationTimestamp: "2022-09-13T08:46:02Z"
  name: pod
  namespace: seccomp-profile-missing-annotations
  resourceVersion: "1735"
  uid: cc1671c0-bbde-4a98-91ee-5d4761cb19d4
spec:
  containers:
  - image: scratch
    imagePullPolicy: Always
    name: container
    resources: {}
    securityContext:
      seccompProfile:
        localhostProfile: bla
        type: Localhost
    terminationMessagePath: /dev/termination-log
    terminationMessagePolicy: File
    volumeMounts:
    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
      name: default-token-jh5xb
      readOnly: true
  dnsPolicy: ClusterFirst
  enableServiceLinks: true
  nodeName: kubeaudit-test-control-plane
  preemptionPolicy: PreemptLowerPriority
  priority: 0
  restartPolicy: Always
  schedulerName: default-scheduler
  securityContext:
    seccompProfile:
      type: RuntimeDefault
  serviceAccount: default
  serviceAccountName: default
  terminationGracePeriodSeconds: 30
  tolerations:
 ...

So, I don't believe it's possible to catch a situation when seccompProfile is absent in securityContext but is present in annotations.
@genevieveluyt @dani-santos-code what do you think?
For now, I've added the requested warning, but it works only in manifest mode, not cluster mode.

genevieveluyt · 2022-09-15T19:09:47Z

Based on these docs, it sounds like having seccomp enabled is the new default behaviour, so kubeaudit might not even have to check that seccomp is enabled but rather should produce an error if seccomp is explicitly disabled (eg. if the profile is set to Unconfined). Did you run your kubelet with the --seccomp-default command line flag? I wonder what the API returns for the seccompProfile if you don't use that flag?

Ser87ch · 2022-09-15T19:35:23Z

@genevieveluyt thanks for your comment. As far as I understand from the link you shared and feature gates, it is a default behaviour but only since version 1.25 (which is in beta). I tested it with the default v1.20.15 node image, where this behaviour isn't even available.

genevieveluyt · 2022-09-15T20:07:05Z

I see. If neither the annotation nor the seccomp profile are set, what does the library return for the profile?

Ser87ch · 2022-09-15T20:11:49Z

I see. If neither the annotation nor the seccomp profile are set, what does the library return for the profile?

There is misunderstanding here. Annotation is set in the example I put above. The problem is that Kubernetes API returns also seccomp profile in the security context when only annotation is set.

genevieveluyt · 2022-09-16T17:37:07Z

Yes I understand that when the annotation is set (but not the seccomp profile) the Kubernetes API still returns a seccomp profile so we cannot differentiate between seccomp being enabled via annotation or profile. I'm wondering though what the Kubernetes API returns for seccomp profile if neither the annotation nor the profile are set. Does it default to Unconfined profile? Or is the seccomp profile empty?

Ser87ch · 2022-09-16T17:39:14Z

If neither is set, then it returns empty securityContext.

genevieveluyt

I think we should check for annotations separately (an auditor can return multiple results).

Check the pod for seccomp annotations. If there are any, produce a warning result where the pending fix removes all said annotations
Check the pod and containers for seccomp profile (as you do currently). If it's unset or set to Unconfined, produce an error result and the pending fix sets the profile to RuntimeDefault

This means in manifest mode you might get both a warning and an error, but I think that's ok. In cluster and local mode, since autofix doesn't do anything, likely someone would manually fix the annotation warning by removing annotation, rerun kubeaudit, and then get the seccomp profile error. In future we could upgrade the annotation warning to an error results as well.

What do you think?

genevieveluyt · 2022-09-16T18:13:41Z

auditors/seccomp/seccomp.go

-		if isSeccompEnabledForContainers(annotations, resource) {
+	if isPodSeccompProfileMissing(podSpec.SecurityContext) {
+		// If all the containers have container-level seccomp profiles then we don't need a pod-level profile
+		if isSeccompEnabledForAllContainers(resource) {


If we get here then we won't produce any warnings for potential annotations 🤔

Annotations are checked in a separate method now.

genevieveluyt · 2022-09-16T18:15:32Z

auditors/seccomp/seccomp.go

+			Rule:       SeccompProfileMissing,
+			Severity:   severity,
+			Message:    msg,
+			PendingFix: &BySettingSeccompProfileAndRemovingAnnotations{seccompProfileType: ProfileRuntimeDefault, annotationsToRemove: seccompAnnotations},


I think there's a weird case here where if there are seccomp annotations but also a valid seccomp profile, the pending fix will overwrite the profile with RuntimeDefault

I think it's also not relevant now.

Ser87ch · 2022-09-16T18:38:10Z

Thanks, your proposal sounds good. I will change accordingly.

Ser87ch · 2022-09-16T20:30:34Z

Unfortunately, there is another problem. The opposite problem is also true. When I create a pod with a seccomp profile in the securityContext only:

apiVersion: v1
kind: Pod
metadata:
  name: pod
spec:
  securityContext:
    seccompProfile:
      type: RuntimeDefault
  containers:
    - name: container
      image: scratch

Kubernetes API also returns annotations for the pod.

$ kl describe pod pod 
Name:             pod
Namespace:        default
Priority:         0
Service Account:  default
Node:             kubeaudit-test-control-plane/10.8.1.2
Start Time:       Fri, 16 Sep 2022 21:24:20 +0100
Labels:           <none>
Annotations:      seccomp.security.alpha.kubernetes.io/pod: runtime/default
Status:           Pending
...

It means that the annotation deprecation warning will be always created with a valid seccomp profile in the SecurityContext.
I have a strong feeling that I shouldn't check annotations in the code at all. Or can I somehow limit annotations check to manifest only mode?

genevieveluyt · 2022-09-16T20:59:58Z

Well that's a rather unexpected and annoying way for the API to behave lol. There's no way in kubeaudit to only run a check manifest mode, no. What if, for number 1 above ie

Check the pod for seccomp annotations. If there are any, produce a warning result where the pending fix removes all said annotations

We change that check to "the pod has only seccomp annotations"? That should only ever happen in manifest mode then right? I don't think there's anything we can do about the annotations in cluster and local mode given how the API behaves...

Ser87ch · 2022-09-20T08:32:49Z

We change that check to "the pod has only seccomp annotations"?

I don't think it's possible unless we can restrict this check only to the manifest mode. If this can't be restricted, then in the cluster and local modes this warning will be always fired when there is any kind of seccomp profile in SecurityContext.

genevieveluyt · 2022-09-20T21:45:41Z

What I mean is, we can check if the pod has a seccomp annotation AND there is NO seccompProfile, then produce a warning. If I understand correctly, this can never happen in local and cluster mode because of how the API behaves (either both the annotation and the seccompProfile are set, or neither are set). So effectively, the warning can only ever happen in manifest mode.

…comp profile

Ser87ch · 2022-09-21T10:16:40Z

@genevieveluyt I've introduced a separate method that audit annotations only if seccomp profile is missing for both pod and all containers. This way it will be easier to remove this logic once seccomp annotation support is dropped.

Ser87ch · 2022-09-23T08:04:58Z

@genevieveluyt could you review PR?

genevieveluyt · 2022-09-23T16:28:20Z

@Ser87ch thank you for making the changes, I will review and 🎩 by end of next week.

genevieveluyt

Code changes look good. Thanks for updating documentation! I will 🎩 tomorrow

genevieveluyt · 2022-09-29T23:49:58Z

auditors/seccomp/fixtures/seccomp-disabled-localhost.yml

+  namespace: seccomp-disabled-localhost
+spec:
+  securityContext:
+        seccompProfile:


Nit: the indentation looks a bit off here, maybe tabs vs spaces?

genevieveluyt · 2022-09-29T23:55:02Z

auditors/seccomp/seccomp.go

-				"MissingAnnotation": PodAnnotationKey,
-			},
-		}
+	// We check annotations only when seccomp profile is missing for both pod and all containers


This comment is amazing, thank you! ❤️

auditors/seccomp/seccomp.go

Co-authored-by: Genevieve Luyt <[email protected]>

genevieveluyt

🎩 looks good! Sorry for the delays, thanks so much for your contribution!

Ser87ch · 2022-10-02T18:58:59Z

Is it possible to request a release after the PR is merged?

dani-santos-code

I also tested on my end and it works really well! Thanks for your changes @Ser87ch ! 🙏

dani-santos-code · 2022-10-05T13:17:04Z

by the way, feel free to open an issue to request a release! :)

Ser87ch · 2022-10-05T13:24:58Z

@dani-santos-code thanks, how can we merge this one first?

dani-santos-code · 2022-10-05T16:56:14Z

I guess we can merge this on our end!

Sergey Chernobrivenko added 7 commits September 1, 2022 11:03

Initial commit

532c2b5

Fix tests

648748b

update docs

9e5a377

cover localhost

1fca2ed

Update usages of seccomp

8cd3553

remove test

c5fffe4

Update docs

42a64a3

ghost added the readme label Sep 2, 2022

github-actions bot added the cla-needed label Sep 2, 2022

github-actions bot removed the cla-needed label Sep 2, 2022

Ser87ch changed the title ~~Migrate to Seccomp profile in security Context~~ Migrate to Seccomp profile in security Context ⚠️ Sep 2, 2022

Sergey Chernobrivenko added 2 commits September 13, 2022 11:03

Add warning when annotations are present

81af075

upd

7500d89

genevieveluyt reviewed Sep 16, 2022

View reviewed changes

Add warning for manifest mode when annotations are present and no sec…

5770159

…comp profile

genevieveluyt reviewed Sep 30, 2022

View reviewed changes

Ser87ch and others added 3 commits September 30, 2022 08:13

Update auditors/seccomp/seccomp.go

fdcb318

Co-authored-by: Genevieve Luyt <[email protected]>

yml formatting

badbe67

fix name

8f5ff04

genevieveluyt approved these changes Oct 1, 2022

View reviewed changes

dani-santos-code approved these changes Oct 4, 2022

View reviewed changes

dani-santos-code merged commit 2061bc7 into Shopify:main Oct 5, 2022

Migrate to Seccomp profile in security Context ⚠️ #475

Migrate to Seccomp profile in security Context ⚠️ #475

Conversation

Ser87ch commented Sep 2, 2022

Description

Type of change

How Has This Been Tested?

Checklist:

ghost commented Sep 2, 2022

Ser87ch commented Sep 2, 2022

Ser87ch commented Sep 6, 2022

dani-santos-code commented Sep 8, 2022

Ser87ch commented Sep 8, 2022

dani-santos-code commented Sep 8, 2022

Ser87ch commented Sep 8, 2022

genevieveluyt commented Sep 8, 2022

Ser87ch commented Sep 9, 2022

Ser87ch commented Sep 12, 2022

Ser87ch commented Sep 13, 2022 • edited Loading

genevieveluyt commented Sep 15, 2022

Ser87ch commented Sep 15, 2022 • edited Loading

genevieveluyt commented Sep 15, 2022

Ser87ch commented Sep 15, 2022

genevieveluyt commented Sep 16, 2022

Ser87ch commented Sep 16, 2022

genevieveluyt left a comment

Choose a reason for hiding this comment

genevieveluyt Sep 16, 2022

Choose a reason for hiding this comment

Ser87ch Sep 21, 2022

Choose a reason for hiding this comment

genevieveluyt Sep 16, 2022

Choose a reason for hiding this comment

Ser87ch Sep 21, 2022

Choose a reason for hiding this comment

Ser87ch commented Sep 16, 2022

Ser87ch commented Sep 16, 2022 • edited Loading

genevieveluyt commented Sep 16, 2022

Ser87ch commented Sep 20, 2022 • edited Loading

genevieveluyt commented Sep 20, 2022

Ser87ch commented Sep 21, 2022

Ser87ch commented Sep 23, 2022

genevieveluyt commented Sep 23, 2022

genevieveluyt left a comment

Choose a reason for hiding this comment

genevieveluyt Sep 29, 2022

Choose a reason for hiding this comment

Ser87ch Sep 30, 2022

Choose a reason for hiding this comment

genevieveluyt Sep 29, 2022

Choose a reason for hiding this comment

genevieveluyt left a comment

Choose a reason for hiding this comment

Ser87ch commented Oct 2, 2022

dani-santos-code left a comment

Choose a reason for hiding this comment

dani-santos-code commented Oct 5, 2022

Ser87ch commented Oct 5, 2022

dani-santos-code commented Oct 5, 2022

Ser87ch commented Sep 13, 2022 •

edited

Loading

Ser87ch commented Sep 15, 2022 •

edited

Loading

Ser87ch commented Sep 16, 2022 •

edited

Loading

Ser87ch commented Sep 20, 2022 •

edited

Loading