Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent CA Injection for Conversion Webhooks #4285

Open
camilamacedo86 opened this issue Nov 3, 2024 · 1 comment · May be fixed by #4282
Open

Inconsistent CA Injection for Conversion Webhooks #4285

camilamacedo86 opened this issue Nov 3, 2024 · 1 comment · May be fixed by #4282
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 3, 2024

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: /

  • Currently, the webhook path patch is scaffolded in config/crd/patches/webhook_in_cronjobs.yaml, as demonstrated here: Webhook Patch Example.

  • Similarly, a CA injection patch for CRDs is scaffolded in config/crd/patches/cainjection_in_cronjobs.yaml, as seen here:
    CA Injection Patch Example.

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 camilamacedo86 added kind/feature Categorizes issue or PR as related to a new feature. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Nov 3, 2024
@camilamacedo86
Copy link
Member Author

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: #4254

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant