CEL in Admission(ValidatingAdmissionPolicy): explicitly excluding resources from validatingadmissionpolicy interception

[cici37]

What would you like to be added?

Currently, the vap only prevent itself and the binding from being intercepted: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission.go#L189-L197 We would love to revisit this before going to GA to make sure if we wanna explicitly prevent any other resources as well.

Thanks for the suggestion from @liggitt : "propose excluding tokenreviews / subjectaccessreviews from validatingadmissionpolicy interception"

Why is this needed?

It's too easy to brick delegated authn / authz in a cluster if user-configurable policy can break subjectaccessreview / tokenreview specifically.

[cici37]

/cc @liggitt @jpbetz @deads2k

[cici37]

This is discussed in today's sig apimachinery CEL meeting: https://docs.google.com/document/d/1Z7p5183OsJ1enJA4-WTk98g7agwSILqKUYxwVHZHilE/edit?tab=t.0#bookmark=id.3n5wj74otles I'll bring this to both sig apimachinery and sig auth meeting.

[cici37]

Notes from sig-auth meeting:

- Admission should not be able to break read requests (i.e. token review)
- Probably should not handle virtual resources since they are not persisted
- Add integration test that makes sure that virtual resources cannot be intercepted

[deads2k]

Add integration test that makes sure that virtual resources cannot be intercepted

All current and future virtual resources.

[cici37]

Thanks! Although that will give webhook more power than in-process validation which sounds a little weird but I guess it could be addressed in a separate KEP since webhook is GAed for long time -_-

[enj]

@kubernetes/sig-auth-leads I did not realize it at the time of the SIG Auth meeting, but we plan on using ValidatingAdmissionPolicy to intercept the virtual token request resource (serviceaccounts/token) in https://github.com/kubernetes-sigs/secrets-store-csi-driver/pull/1364 for limiting the secret sync controller to a specific audience:

apiVersion: admissionregistration.k8s.io/v1beta1
kind: ValidatingAdmissionPolicy
metadata:
  name: "secret-sync-controller-validate-token-policy"
spec:
  failurePolicy: Fail
  matchConditions:
    - name: 'userIsController'
      expression: "request.userInfo.username == 'system:serviceaccount:foo-ns:bar-sa'"
  matchConstraints:
    resourceRules:
    - apiGroups:   [""]
      apiVersions: ["v1"]
      operations:  ["CREATE"]
      resources:   ["serviceaccounts/token"]
  variables:
  - name: hasCorrectAudience
    expression: "object.spec.audiences.size() == 1 && object.spec.audiences.exists(w, w == 'expected-audience'))"
  validations:
  - expression: "variables.hasCorrectAudience == true"

A blanket restriction on not allowing virtual resources to be intercepted would break that.

[cici37]

Thanks for sharing the use case! I am always curious on the boundary of responsibility between giving user enough flexibility/power and have them set up restrictions as needed and take responsibility VS k8s offering restrictions for auth/security concerns. Look forward to hearing others' insights on this :)

[liggitt]

hoisting discussion from slack to something more permanent

@liggitt

• tokenreview / subjectaccessreview seem way clearer to exempt, since they are truly read-only (they don't create anything) and breaking them can brick existing state (edited)
• tokenrequest actually does create a new thing, it's just not persisted, so letting admission guard it makes more sense to me... webhook admission could still get itself in trouble there if it broke creating tokens needed to keep controllers healthy which were needed to run pods which were needed to back the webhook admission...
• that's less of an issue for VAP as long as kube-apiserver doesn't start taking dependencies on service account tokens internally
• but I admit the tokenrequest counter example makes our "all virtual resources" rationale less cut and dry

@deads2k

• Shall we start with a list of all virtual resources and an integration test that prevents them being added until added to a whitelist with explanation?

@liggitt

• that seems reasonable though I'd still like a rationale we could use to guide exceptions
• well... would we call subresources like pods/attach and pods/exec virtual? I guess those get modeled as Connect operations to admission
• thinking out loud about what makes me consider subjectaccessreview and tokenreview readonly and pods/exec, pods/attach, and tokenrequest not
  1. doesn't persist anything in etcd
  2. doesn't affect cluster state (pods/exec, pods/attach)
  3. doesn't instantiate new things (whether they are persisted or not), like serviceaccounts/token does

@enj

• So we use those rules to define 'virtual' meaning self subject review would also be virtual for example

@liggitt

• I think so, yeah

---

I think my takeaway is that every new virtual resource needs to be run through a decision process and put into an include or exclude list... putting clear guidelines about the rationale and putting a test in place that requires new built-in virtual resources be categorized would make sense to me

[cici37]

As a summary, all virtual resources should go in either a allowlist or a blocklist. For any virtual resources in blocklist, VAP should not be able to intercept it. And there should be integration tests added to guarantee the behavior. Thanks!

[cici37]

As mentioned last week, @jiahuif would like to take up this issue and is working on a PR. Thanks! /assign @jiahuif /unassign

[jiahuif]

Will work on it while my other PRs are under review, PR eta next week.