cert-manager / approver-policy

approver-policy is a cert-manager approver that allows users to define policies that restrict what certificates can be requested.
https://cert-manager.io/docs/policy/approval/approver-policy/
Apache License 2.0
66 stars 23 forks source link

Add certificate request username/serviceaccount as an option for CEL validation #506

Open jamesglennan opened 2 weeks ago

jamesglennan commented 2 weeks ago

Hi Team,

I wanted to ask about getting some of the capability we get in the csi-driver-spiffe approver where it checks the service account (username field) that requested a certificate before approving the certificate request.

Background

Currently the CEL validator only provides the name and namespace as fields to be used for validating- we want to be able to extract the serviceaccount name as well, so we can approve certificates that contain fields related to the requestor

Why?

We are looking to take advantage of the csi-driver and issue certificates that both contain dnsNames for a given pod/service and also the URIs associated with a given ServiceAccount's SPIFFE SVID. We have a limitation with our setup- expecting a single certificate for both client and server communication. As such we want to migrate away from the csi-driver-spiffe, but we want to retain the approval policy that it offers. By having the serviceaccount name available to us, we can use CEL to validate our approvals with something like self.endswith(cr.namespace + '/sa/' + cr.serviceaccount) or self.matches('spiffe://trust.domain/ns/' + cr.namespace + '/sa/' + cr.serviceaccount)

Solutions

I had a quick look last week in what would need to be changed- namely adding a field to the struct/protobuf and parsing the username field. Obviously I would love to hear feedback on this idea and get some input from the team about this problem.

Technical Questions

Would we just parse the username field similar to the csi-driver-spiffe-approver and insert it into a field called ServiceAccount? Surely there are circumstances where the requesting username isn't in the format system:serviceaccount:ns:name and in those cases is it appropriate to leave the field blank. Do we provide both, have a field for username and serviceaccount, or offer some other pre-parsed version?

I look forward to discussing- I would be happy to help with the actually implementing (if the solution isn't crazy complicated) and please let me know if there's any extra details or questions about this.

SgtCoDFish commented 2 weeks ago

Copying my details from slack

It's important to note that csi-driver will use its own ServiceAccount to create CertificateRequest resources by default, in contrast to csi-driver-spiffe which uses the pod's SA.

csi-driver can be configured to use the pod's SA using the app.driver.useTokenRequest=true Helm value at install time.

For illustration, here's a CR created by csi-driver without useTokenRequest:

apiVersion: cert-manager.io/v1
kind: CertificateRequest
metadata:
  creationTimestamp: "2024-09-30T08:26:02Z"
  generation: 1
  labels:
    csi.cert-manager.io/node-id-hash: 645bccb866
    csi.cert-manager.io/volume-id-hash: 6f88bc479f
  name: cd157e73-c4db-45be-8eed-6413b088a5ea
  namespace: sandbox
  ownerReferences:
  - apiVersion: v1
    kind: Pod
    name: my-csi-app
    uid: 0a99cc62-09ca-455a-b9f0-d4a0d0aa9ff7
  resourceVersion: "2190"
  uid: 144758d8-8952-40a8-bd48-5a79c3552abf
spec:
  duration: 2160h0m0s
  extra:
    authentication.kubernetes.io/credential-id:
    - JTI=1eb171b4-fdc1-4b79-aceb-6e506f6ddb02
    authentication.kubernetes.io/node-name:
    - kind-control-plane
    authentication.kubernetes.io/node-uid:
    - 2b5d2cc2-95c4-4532-940f-d513d2a904a0
    authentication.kubernetes.io/pod-name:
    - cert-manager-csi-driver-5kft9
    authentication.kubernetes.io/pod-uid:
    - 82e76b3d-782e-4b6e-aab2-6959bf401174
  groups:
  - system:serviceaccounts
  - system:serviceaccounts:cert-manager
  - system:authenticated
  issuerRef:
    group: cert-manager.io
    kind: Issuer
    name: intermediate-ca-issuer-2
  request: ...
  uid: e7ff6c47-863a-4adb-aade-d73a9aa43c27
  usages:
  - digital signature
  - key encipherment
  username: system:serviceaccount:cert-manager:cert-manager-csi-driver

And here's one where I did set useTokenRequest:

apiVersion: cert-manager.io/v1
kind: CertificateRequest
metadata:
  creationTimestamp: "2024-09-30T09:01:36Z"
  generation: 1
  labels:
    csi.cert-manager.io/node-id-hash: 645bccb866
    csi.cert-manager.io/volume-id-hash: 64974d7c4c
  name: a675ee8b-16a5-488a-8dd0-974cd37faa16
  namespace: sandbox
  ownerReferences:
  - apiVersion: v1
    kind: Pod
    name: my-csi-app
    uid: a7043139-2c06-4518-aa6c-10ebc2984e60
  resourceVersion: "3472"
  uid: e4eee729-35e6-4308-812a-99288802de72
spec:
  duration: 2160h0m0s
  extra:
    authentication.kubernetes.io/credential-id:
    - JTI=22838f7f-fdd5-4ebe-86e0-3f284b362c60
    authentication.kubernetes.io/node-name:
    - kind-control-plane
    authentication.kubernetes.io/node-uid:
    - 2b5d2cc2-95c4-4532-940f-d513d2a904a0
    authentication.kubernetes.io/pod-name:
    - my-csi-app
    authentication.kubernetes.io/pod-uid:
    - a7043139-2c06-4518-aa6c-10ebc2984e60
  groups:
  - system:serviceaccounts
  - system:serviceaccounts:sandbox
  - system:authenticated
  issuerRef:
    group: cert-manager.io
    kind: Issuer
    name: intermediate-ca-issuer-2
  request: ...
  uid: 9b0be553-dbcf-4f13-9811-6050479a8630
  usages:
  - digital signature
  - key encipherment
  username: system:serviceaccount:sandbox:my-pod-sa

In both cases, the important part is the last line.

Here's the YAML for creating a pod when useTokenRequest is enabled:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: my-pod-sa
  namespace: sandbox

---

kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: pod-cr-create
  namespace: sandbox
rules:
- apiGroups: ["cert-manager.io"]
  resources: ["certificaterequests"]
  verbs: [ "create" ]

---

kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: pod-cr-create
  namespace: sandbox
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: pod-cr-create
subjects:
- kind: ServiceAccount
  name: my-pod-sa

---

apiVersion: v1
kind: Pod
metadata:
  name: my-csi-app
  namespace: sandbox
  labels:
    app: my-csi-app
spec:
  serviceAccountName: my-pod-sa
  containers:
    - name: my-frontend
      image: busybox
      volumeMounts:
      - mountPath: "/tls"
        name: tls
      command: [ "sleep", "1000000" ]
  volumes:
    - name: tls
      csi:
        driver: csi.cert-manager.io
        readOnly: true
        volumeAttributes:
              csi.cert-manager.io/issuer-name: intermediate-ca-issuer-2
              csi.cert-manager.io/dns-names: ${POD_NAME}.${POD_NAMESPACE}.svc.cluster.local
erikgb commented 2 weeks ago

Thanks for opening this issue, @jamesglennan. Your use case makes a lot of sense, and implementing this should not be a complex task. Let's agree on some principles first: In my opinion, the information we provide from certificate requests to use in approver-policy CEL expression should be literal values from the request - leaving no room for misunderstanding. So in this case, I would suggest a new username field on the certificate request protobuf representation containing the literal value of the .spec.username field. To support your concrete use case better, I would suggest adding a simple library containing helpers to extract the service account name. Similar to the upstream Kubernetes URL library. In your case, the approver policy expression can look like this:

apiVersion: policy.cert-manager.io/v1alpha1
kind: CertificateRequestPolicy
spec:
  allowed:
    uris:
      validations:
        - rule: self.matches('spiffe://trust.domain/ns/' + cr.namespace + '/sa/' + serviceAccount(cr.username).getName())

WDYT?

jamesglennan commented 1 week ago

I think thats a really good pattern to follow- I completely agree that we should present the literal values as they are in the CertRequest. I'll take a look at the k8s url library and see if i can make sense of it and I'll come back if I have any questions.

Would it make sense to have 2 functions getName and getNamespace to start? Im thinking of a circumstance where a service account with the same name tries to create a certificate in another namespace- you would only be validating that the name is correct.

jamesglennan commented 1 week ago

I've got a first pass at this in #514 Let me know if you think I'm on the right track, or want me to change something. I haven't actually compiled this and run it in a real cluster yet just FYI