Migrate to Seccomp profile in security Context :warning:

[Ser87ch]

Description

This PR changes seccomp auditor to scan seccompProfile instead of annotations as requested in https://github.com/Shopify/kubeaudit/issues/343 The following changes were done:

• seccomp auditor changed to scan securityContext in pod and containers to find seccompProfile. :warning:
• 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. :warning:
• SeccompDeprecatedContainer rule dropped. :warning:
• Tests updated to cover changes in auditor and fix.
• Documentation updated.

Fixes #343

Type of change

• [X] This change requires a documentation update :book:
• [X] Breaking changes :warning:

How Has This Been Tested?

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

Checklist:

• [ ] I have :tophat: 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)
• [X] I have performed a self-review of my own code
• [X] I have made corresponding changes to the documentation
• [X] I have added tests that prove my fix is effective or that my feature works
• [X] New and existing unit tests pass locally with my changes
• [X] The test coverage did not decrease
• [X] I have signed the appropriate Contributor License Agreement

[ghost]

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

[Ser87ch]

I've signed the CLA!

[Ser87ch]

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

[dani-santos-code]

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]

@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]

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]

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

[genevieveluyt]

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]

Thanks guys, I will implement your suggestions.

[Ser87ch]

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]

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]

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]

@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]

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

[Ser87ch]

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]

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]

If neither is set, then it returns empty securityContext.

[Ser87ch]

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

[Ser87ch]

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]

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

1. 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]

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]

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.

[Ser87ch]

@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]

@genevieveluyt could you review PR?

[genevieveluyt]

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

[Ser87ch]

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

[dani-santos-code]

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

[Ser87ch]

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

[dani-santos-code]

I guess we can merge this on our end!