kubernetes-sigs / kubebuilder

Kubebuilder - SDK for building Kubernetes APIs using CRDs
http://book.kubebuilder.io
Apache License 2.0
7.94k stars 1.45k forks source link

Inconsistent CA Injection for Conversion Webhooks #4285

Open camilamacedo86 opened 2 weeks ago

camilamacedo86 commented 2 weeks ago

What do you want to happen?

Context:

When creating conversion webhooks in Kubebuilder, we need to scaffold a patch to set the webhook's conversion path to /convert. The expected configuration in the CRD is as follows:

spec:
  conversion:
    strategy: Webhook
    webhook:
      clientConfig:
        service:
          name: project-webhook-service
          namespace: project-system
          path: /convert
      conversionReviewVersions:
      - v1

Additionally, if Cert-Manager is enabled, the CRD should be patched with a specific annotation to enable CA injection:

cert-manager.io/inject-ca-from: /

Both above are implementation which came from go/v3 and kustomize/v1 (legacy when vars were supported by kustomize and used in the project)

Problem:

In Kubebuilder Go/v4 with Kustomize v2, the CA injection patch fails because vars are no longer supported, resulting in unresolved placeholders in the annotation:

cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME

Note that in the config/default/kustomization.yaml it is partially addressed by using Kustomize's replacement feature (see lines L148-L177), which correctly injects CA details when uncommented. However, this approach introduces a new issue: the CA annotation is applied to all CRDs, not just those configured with conversion webhooks. This poses a security risk, as CA injection should only apply to CRDs explicitly requiring conversion webhooks.

Proposed Solution:

Proposed Solution:

  1. Update the Kubebuilder scaffold to use replacements in config/default/kustomization.yaml in a way that limits CA injection specifically to CRDs with conversion webhooks. Ensure that only CRDs with a defined conversion path (/convert) have the CA injection annotation.

This can be achieved by using markers to properly inject CRD values, ensuring that only specified CRDs are annotated. That mean we will scaffold by default

# - source: # Uncomment the following block if you have a ConversionWebhook (--conversion)
#     kind: Certificate
#     group: cert-manager.io
#     version: v1
#     name: serving-cert # This name should match the one in certificate.yaml
#     fieldPath: .metadata.namespace # Namespace of the certificate CR
#   targets: # Do not remove or uncomment the following scaffold marker; required to generate code for target CRD.
# +kubebuilder:scaffold:crdkustomizecainjectionns
# - source:
#     kind: Certificate
#     group: cert-manager.io
#     version: v1
#     name: serving-cert # This name should match the one in certificate.yaml
#     fieldPath: .metadata.name
#   targets: # Do not remove or uncomment the following scaffold marker; required to generate code for target CRD.
# +kubebuilder:scaffold:crdkustomizecainjectionname

Example Result for Two CRDs with Conversion:

// The following will inject the certificate namespace
   - source:
       kind: Certificate
       group: cert-manager.io
       version: v1
       name: serving-cert
       fieldPath: .metadata.namespace
     targets:
       - select:
           kind: CustomResourceDefinition
           name: firstmates.crew.testproject.org // <- SEE HERE WE ARE DEFINING THE CRD
         fieldPaths:
           - .metadata.annotations.[cert-manager.io/inject-ca-from]
         options:
           delimiter: '/'
           index: 0
           create: true
       - select:
           kind: CustomResourceDefinition
           name: secondmates.crew.testproject.org  // <- SEE HERE WE ARE DEFINING THE CRD
         fieldPaths:
           - .metadata.annotations.[cert-manager.io/inject-ca-from]
         options:
           delimiter: '/'
           index: 0
           create: true

// The following will inject the certficate name           
   - source:
       kind: Certificate
       group: cert-manager.io
       version: v1
       name: serving-cert
       fieldPath: .metadata.name
     targets:
       - select:
           kind: CustomResourceDefinition
           name: firstmates.crew.testproject.org  // <- SEE HERE WE ARE DEFINING THE CRD
         fieldPaths:
           - .metadata.annotations.[cert-manager.io/inject-ca-from]
         options:
           delimiter: '/'
           index: 1
           create: true
       - select:
           kind: CustomResourceDefinition
           name: secondmates.crew.testproject.org  // <- SEE HERE WE ARE DEFINING THE CRD
         fieldPaths:
           - .metadata.annotations.[cert-manager.io/inject-ca-from]
         options:
           delimiter: '/'
           index: 1
           create: true
  1. Remove the CA injection patch that does not work under config/crd/patches/cainjection_in_cronjobs.yaml,

Impact:

Without this fix, users enabling Cert-Manager will encounter unintended CA annotations across all CRDs, undermining security by applying conversion webhook annotations to CRDs that do not require them.

Additionally we need to remove config/crd/patches/cainjection_in_cronjobs.yaml, since it does not work.

camilamacedo86 commented 2 weeks ago

It is important because we have been adding e2e tests to ensure all features and options, including scaffolds and examples, as much as possible. The latest piece missing is the webhook conversion, which is not fully implemented yet but will be once we merge the PR: https://github.com/kubernetes-sigs/kubebuilder/pull/4254