feat: Make gatekeeper validate subresources

[mac-chaffee]

What this PR does / why we need it:

Gatekeeper's validatingwebhook is currently configured to match '*', which doesn't include subresources. There has been scattered discussion around adding subresources to the validatingwebhook, but nothing conclusive, probably due to the fact that a complete list of subresources doesn't really exist.

I was able to get a really good list of subresources by searching the Kubernetes codebase for Subresource(", which I've included in this PR. However, in my testing, intercepting the status subresource causes cluster instability (it's very high-traffic, and you can't smoothly roll out new policies to pods because the old pods trigger validation upon deletion since the status field updates). So we can't just specify '*/*'.

Which issue(s) this PR fixes

Related to #1087, #45, #1056, and open-policy-agent/gatekeeper-library#188

Fixes #1837

Special notes for your reviewer:

cc @srenatus , mentioned this here but seems this issue about subresources is already public (according to those issues I linked above and the slack convo on May 17th)

[maxsmythe]

Thanks for the PR and finding relevant subresources!

The code LGTM. Should we also add it to the static manifest by updating this codegen comment:

gatekeeper/pkg/webhook/policy.go

Line 73 in 206bbe9

// +kubebuilder:webhook:verbs=create;update,path=/v1/admit,mutating=false,failurePolicy=ignore,groups=*,resources=*,versions=*,name=validation.gatekeeper.sh,sideEffects=None,admissionReviewVersions=v1;v1beta1,matchPolicy=Exact

and running make manifests

Or maybe filing a new issue?

[mac-chaffee]

Hmm forgot about the static manifests. @ritazh 's comment about this changing the default behavior might be more applicable to the static manifest since people might install straight off the master branch.

I still lean towards "yes, change the default to cover subresources", but we should definitely mention it prominently in the patch notes. Are we on the same page there?

[ritazh]

Another thought is 'pods/ephemeralcontainers' should be added by default to fix open-policy-agent/gatekeeper-library#188

@mac-chaffee What kind of performance impact would this change have if we add all the other subresources by default?

[mac-chaffee]

@ritazh I'd expect the performance impact to be minimal since subresources are almost always called less-frequently than the parent resource (which gatekeeper already validates). I could imagine a handful of users who might call pods/exec or pods/log orders of magnitude more than pods (like maybe a highly-concurrent job-creating application), but those are the kinds of advanced users who I think should specify their own mutatingWebhookCustomRules anyway. A prominent section in the release notes will help those people.

If you all are feeling anxious about this change, I think these are our options:

1. Validate ALL known subresources (what this PR does)
   • Pros: Removes the risk of under-enforcement as mentioned here
   • Cons: Potential for performance impact
2. Validate ONLY enough subresources to get all gatekeeper-library policies working (pods/ephemeralcontainers, */scale)
   • Pros: Fixes the current security vulnerability with the least possible impact
   • Cons: Doesn't fix Support constraints on PodExecOptions #1056 or any other users whose (closed-source) policies are unknowingly being ignored right now

[maxsmythe]

IMO I'm for enforce everywhere by-default (warning the user via release notes).

In general, I think loud failures are easier to spot and mitigate than silent ones.

[mac-chaffee]

@maxsmythe I was thinking the same thing about loud vs. quiet failures 😁

Sounds good, I can get the static manifests updated tomorrow

[sozercan]

Thanks for finding these subresources! LGTM

[sozercan]

Looks like someone made a script to get these subresources here: https://stackoverflow.com/a/51289417

Here's a slightly modified version to dump these in case there are any added in the future:

#!/bin/bash
APIS=$(kubectl get --raw /apis | jq -r '[.groups | .[].name] | join(" ")')

# do core resources first, which are at a separate api location
api="core"
kubectl get --raw /api/v1 | jq -r --arg api "$api" '.resources | .[] | "\($api) \(.name): \(.verbs | join(" "))"'

# now do non-core resources
for api in $APIS; do
    version=$(kubectl get --raw /apis/$api | jq -r '.preferredVersion.version')
    kubectl get --raw /apis/$api/$version | jq -r --arg api "$api" '.resources | .[]? | "\($api) \(.name): \(.verbs | join(" "))"'
done

[ritazh]

LGTM

[maxsmythe]

LGTM

[codecov-commenter]

Codecov Report

Merging #2054 (1913765) into master (a7473ae) will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2054      +/-   ##
==========================================
+ Coverage   54.34%   54.50%   +0.16%     
==========================================
  Files         111      111              
  Lines        9470     9470              
==========================================
+ Hits         5146     5162      +16     
+ Misses       3933     3921      -12     
+ Partials      391      387       -4     

Flag	Coverage Δ
unittests	54.50% <ø> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/webhook/policy.go	37.53% <ø> (ø)
...onstrainttemplate/constrainttemplate_controller.go	58.99% <0.00%> (+3.83%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7473ae...1913765. Read the comment docs.

[ritazh]

Do we want to cut another release to bump the chart version? @sozercan @maxsmythe

[sozercan]

@ritazh v3.9.0-beta.2?

[ritazh]

sgtm

[tehlers320]

does this "*" need to come out of here?

[mac-chaffee]

No, the star refers to all non-sub-resources. For example "configmaps" or "secrets" or "pods". We still definitely want to trigger validation on those like we have done historically.

This PR does not reduce the amount of resources that gatekeeper validates; it expands that list to cover subresources.