Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

support for CRD's that extend common types #373

Closed
1 of 2 tasks
nobletrout opened this issue Nov 5, 2021 · 7 comments · Fixed by #374
Closed
1 of 2 tasks

support for CRD's that extend common types #373

nobletrout opened this issue Nov 5, 2021 · 7 comments · Fixed by #374

Comments

@nobletrout
Copy link
Contributor

nobletrout commented Nov 5, 2021

ISSUE TYPE
  • Bug Report
  • Feature Idea
SUMMARY

I've noticed that Kubeaudit doesn't catch CRDS that are extending typical k8s resources, like apps/v1.
It looks like either with finagling the data out with kubectl and scanning you can still get to the original source of truth.

STEPS TO REPRODUCE

create a custom CRD based off apps
scan k8s
don't find anything wrong with deploy applications

EXPECTED RESULTS

It should find them

other stuff

if you guys want to point me in the direction of where in the code one can specify a custom CRD, that'll work for me and I'll add a flag to support it.

@ghost
Copy link

ghost commented Nov 5, 2021

Thanks for opening your first issue here! Be sure to follow the issue template!

@nobletrout
Copy link
Contributor Author

I think this is the issue here:

func excludeGenerated(resources []k8s.Resource) []k8s.Resource {
var filteredResources []k8s.Resource
for _, resource := range resources {
if len(k8s.GetObjectMeta(resource).OwnerReferences) == 0 {
filteredResources = append(filteredResources, resource)
}
}
return filteredResources
}

the filtering of resources removes any resources that are based on CRD's. This can include templated out and extended resources, leaving a blind spot for anyone that's running resources that extend that base resources.

@genevieveluyt
Copy link
Contributor

Hey @nobletrout , thanks for opening an issue! I think you are right and we want the excludeGenerated function to only exclude a resource if the owner reference is NOT a CRD. Does that sound right? (the intent behind that function is that we don't show duplicate kubeaudit results for each pod generated by a deployment/replicaset/job etc.)

@nobletrout
Copy link
Contributor Author

that does sound right. I could keep working on the diff I've provided, but I think dumping out all the records, duplicated or not, is still valuable in case there's a risk of the excludeGenerated function removing a resource that has a security violation.

@nobletrout
Copy link
Contributor Author

The current check looks to see if the ownerReferences array is > 0.

The only stuff I have to check against is proprietary and I can't share. Can you give a generic example of when that array is populated with duplicates?

@genevieveluyt
Copy link
Contributor

The ownerReferences array itself won't contain duplicates but if I recall correctly, if you were to apply a Deployment into a cluster and it's missing the apparmor annotation, and then run kubeaudit apparmor, it would generate a single

-- [error] AppArmorAnnotationMissing

result for the Deployment. Even though the deployment created pods in the cluster, those get ignored by kubeaudit. If you were to remove the excludeGenerated function call, then kubeaudit would also audit the pods created by the deployment and you would see the

-- [error] AppArmorAnnotationMissing

multiple times, once for the deployment, and once for each generated pod. This is not useful to the user since, in order to fix the issue, they need to edit the Deployment, not any of the pods. Showing the results for the pod just makes it harder to find the root cause.

Fyi I'm out most of this week so I will likely be slow to respond. I can give you a more concrete example next week!

I would be ok adding a flag for excludeGenerated (as per the PR you opened) but I would really like to make the default kubeaudit behaviour only ignore resources generated by non-CRD resources.

Here is an example of what the ownerReferences field looks like (which you should be able to see on any pod generated by a deployment/replicaset etc. with kubectl get po <NAME> -o yaml)

  ownerReferences:
  - apiVersion: apps/v1
    blockOwnerDeletion: true
    controller: true
    kind: ReplicaSet
    name: ...
    uid: ...

I wonder if we could use the apiVersion field to determine whether the owner is a CRD or not?

If we are able to apply this to kubeaudit's default behaviour, I would like a concrete example of when having an additional excludeGenerated flag would be beneficial, before we add that flag into the codebase.

Thanks for working on this!

@nobletrout
Copy link
Contributor Author

So here is my (sanitized) output for ownerReferences array for a CRD deployment

  ownerReferences:
  - apiVersion: apps.XYZCOMP.com/v1
    blockOwnerDeletion: true
    controller: true
    kind: MyCustomResource
    name: ...
    uid: .....

The problem is that there isn't a good way to extract all resources from kubernetes. That said, you could do a somewhat intelligent feature that when parsing Kind values in deployments, if it was a non-default resource, it would enumerate all of the templates for that kind. in the above case

kubectl get my-customer-resources -o yaml

I think having an exludeGenerated flag would be handy for times when you might have orphaned resources that no longer have their deployment templates or otherwise available. In this case, the pod is still running with an old deployment template, but the operator is unaware of it.

The PR I submitted leaves the default behavior alone, the flag disables filtering of assets based on ownerRefs count if used so I think it'll have minimal impact on default user behavior.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants