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.

Sign up for GitHub

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

Jump to bottom

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
@camilamacedo86
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.

Comments

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 3, 2024 •
edited
Loading

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

@camilamacedo86 camilamacedo86 linked a pull request Nov 3, 2024 that will close this issue
Open
@camilamacedo86 camilamacedo86 self-assigned this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@camilamacedo86 camilamacedo86

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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🐛 (kustomize/v2, go/v4): Fix ca injection for conversion webhooks camilamacedo86/kubebuilder
1 participant
@camilamacedo86