cert-manager / cert-manager

Automatically provision and manage TLS certificates in Kubernetes
https://cert-manager.io
Apache License 2.0
11.9k stars 2.05k forks source link

Venafi Issuer Read `caBundle` from Configmap or Secret #5514

Closed hawksight closed 2 months ago

hawksight commented 1 year ago

Is your feature request related to a problem? Please describe.

I would like to be able to specify the caBundle from the contents of a secret or configmap resource.

In my Issuer / ClusterIssuer resource you have a caBundle field that must be specified as a base64 encoded string, eg.

spec:
  venafi:
    tpp:
      caBundle: <B64_ENCODED_STRING>
      credentialsRef:
        name: tpp-token
      url: https://my-server.com/vedsdk/
    zone: Certificates\public

This becomes a configuration management issue when you have many issuers in a cluster using the same TPP endpoint, as you now have many places to copy the same CA information.

If using the cert-manager trust project, it would be a good practice to define the caBundle once and distribute that to all relevant namespaces in a cluster. However Trust outputs to either a Configmap or Secret resource.

Between the Issuer and the trust Bundle there is a disconnect.

Describe the solution you'd like

I could image for the Venafi issuer, you could have something like:

spec:
  venafi:
    tpp:
      caBundleRef: # A new key could be either or with the existing `caBundle`
        type: configMap  # configMap | secret | bundle
        name:  tpp-trust-bundle
        key: ca-certificates.crt
      credentialsRef:
        name: tpp-token
      url: https://my-server.com/vedsdk/
    zone: Certificates\public

Describe alternatives you've considered

The alternative is really to copy an paste the output into the Issuer configuration. I'm not sure there are any others.

Additional context

Most consumers of the Venafi integration would generally use few or only 1 actual TPP endpoint with a single CA source. By enabling the reference of a configMap or Secret resource, then this means users can effectively distribute their TPP CA via trust bundle.

Environment details (remove if not applicable):

N/A

/kind feature

hawksight commented 1 year ago

I've seen that in v1.10.0 release there is a small enhancement for the Vault Issuer: https://github.com/cert-manager/cert-manager/pull/5387 adding caBundleSecretRef.

This is very similar except it just covers the Vault issuer and only from a secret, no configmap support.

jetstack-bot commented 1 year ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Send feedback to jetstack. /lifecycle stale

hawksight commented 1 year ago

/remove-lifecycle stale

jetstack-bot commented 1 year ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Send feedback to jetstack. /lifecycle stale

hawksight commented 1 year ago

/remove-lifecycle stale

sankalp-at-gh commented 1 year ago

Hi @hawksight, could you assign this issue to me? I'd like to give it a try.

hawksight commented 1 year ago

/assign sankalp-at-gh

hawksight commented 1 year ago

Hey @sankalp-at-gh I didn't think I could but apparently I can. I've assigned you now.

sankalp-at-gh commented 1 year ago

Hey @sankalp-at-gh I didn't think I could but apparently I can. I've assigned you now.

That's awesome, appreciate your help. Looking forward to my first contribution to this repo.

SimeonPoot commented 1 year ago

Would be great if we can have this feature!

AppalaKarthik commented 1 year ago

Design Details

Add Secret, ConfigMap, ClusterTrustBundle optional fields to the spec which can be used to refer a key to find the caBundle. Below is the example for venafi issuer.

This method is compatible with existing API's

spec:
  venafi:
    tpp:
      caBundle: <B64_ENCODED_STRING>
      caBundleSecretRef:
        name: <>
        key: <>
      caBundleConfigMapRef:
        name: <>
        key: <>
      clusterTrustBundleRef:
        name: <>
        key: <>
      credentialsRef:
        name: tpp-token
      url: https://my-server.com/vedsdk/
    zone: Certificates\public

Please provide your thoughts on it

hawksight commented 1 year ago

@AppalaKarthik I think that I would remove the clusterTrustBundleRef from the above. The trust Bundle is a cluster level resource, so may not be accessible from an Issuer without extensive RBAC. It also isn't inteded to be consumed directly I don't think, instead you would make reference to the ConfigMap that it creates in each namespace selected and use the ca.crt from that configmap. That configmap will update when the Bundle is updated.

So I propose that you would have:

spec:
  venafi:
    tpp:
      caBundle: <B64_ENCODED_STRING>
      caBundleSecretRef:
        name: <>
        key: <>
      caBundleConfigMapRef:
        name: <>
        key: <>
      credentialsRef:
        name: tpp-token
      url: https://my-server.com/vedsdk/
    zone: Certificates\public
AppalaKarthik commented 1 year ago

@hawksight Makes sense to me. Became little over ambitious there. Like you said configmap covers it anyways

sankalp-at-gh commented 1 year ago

@AppalaKarthik I think that I would remove the clusterTrustBundleRef from the above. The trust Bundle is a cluster level resource, so may not be accessible from an Issuer without extensive RBAC. It also isn't inteded to be consumed directly I don't think, instead you would make reference to the ConfigMap that it creates in each namespace selected and use the ca.crt from that configmap. That configmap will update when the Bundle is updated.

So I propose that you would have:

spec:
  venafi:
    tpp:
      caBundle: <B64_ENCODED_STRING>
      caBundleSecretRef:
        name: <>
        key: <>
      caBundleConfigMapRef:
        name: <>
        key: <>
      credentialsRef:
        name: tpp-token
      url: https://my-server.com/vedsdk/
    zone: Certificates\public

@hawksight , seems there is a bit confusion here. The idea in @AppalaKarthik's approval was to support the kubernetes native ClusterTrustBundles and not Trust Manager from cert-manager project, which indeed creates configmaps in target namespaces. Please let me know if you think otherwise.

As far as access for ClusterTrustBundles is concerned, I see below mentioned in https://kubernetes.io/docs/reference/access-authn-authz/certificate-signing-requests/#ctb-common, from which I assume there would not be a need for extensive RBAC(I might be wrong too, needs more testing). And seems it doesn't create any configmaps in namespaces.

ClusterTrustBundle objects should be considered world-readable within the cluster. If your cluster uses RBAC authorization, all ServiceAccounts have a default grant that allows them to get, list, and watch all ClusterTrustBundle objects. If you use your own authorization mechanism and you have enabled ClusterTrustBundles in your cluster, you should set up an equivalent rule to make these objects public within the cluster, so that they work as intended.

Please let us know if support for ClusterTrustBundles can be slated for discussion at a later point and if we could proceed with proposal for just Secrets and ConfigMaps. We initially discussed about supporting ClusterTrustBundles when @irbekrm brought it up in one of the standups.

Ref: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3257-trust-anchor-sets

hawksight commented 1 year ago

Hey @sankalp-at-gh - you are correct that I had defaulted to trust-manager and not realised about the new Cluster Trust Bundle resource. Thank you for pointing that out and the detail provided. I don't know much about this feature yet so I'll be sure to have a good look into it.

Just wanted to say I am not a maintainer, but I am very interested in this issue.

From an initial look, I think it makes sense to have this as an option in the Issuer configuration in the long term. However it is an alpha feature in 1.27 and therefore needs enables by users. It might also need some additional CI things to fully test adding this.

Please let us know if support for ClusterTrustBundles can be slated for discussion at a later point and if we could proceed with proposal for just Secrets and ConfigMaps.

Yes I agree with this, lets move forward with Secret and ConfigMap support first.

Cluster Trust Bundles probably needs some more discussion but I do think it's on people's minds that cert-manager would like to integrate with this feature.

erikgb commented 1 year ago

Looking at what's needed to do this, in context of the sibling issue (https://github.com/cert-manager/cert-manager/issues/6117), I am not convinced this is a "good first issue". It will require modification to some internal controller mechanics and extension of RBAC. 😊

sankalp-at-gh commented 1 year ago

Looking at what's needed to do this, in context of the sibling issue (#6117), I am not convinced this is a "good first issue". It will require modification to some internal controller mechanics and extension of RBAC. 😊

agreed ! Even though I'm familiar with making such changes, just the amount of time I needed to make these changes makes me feel this is a bit more for a "good first issue" too.

Please expect a PR soon as I'm almost done with the changes 😊

sankalp-at-gh commented 10 months ago

Hello @erikgb,@hawksight, opened up above pull today. Might need some help in taking it forward for a review. Thanks

jetstack-bot commented 7 months ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Send feedback to jetstack. /lifecycle stale

hawksight commented 7 months ago

/remove-lifecycle stale

cert-manager-bot commented 4 months ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. /lifecycle stale

hawksight commented 4 months ago

/remove-lifecycle stale

sankalp-at-gh commented 3 months ago

Hi @hawksight / @inteon , I have opened up PR #7036 which only supports secretref and includes all suggested fixes in #6454 . Could you please review the PR and if it looks good, could you help me with how to go about updating documentation. Thanks !!

hawksight commented 3 months ago

Hey @sankalp-at-gh - It looks really good to me. Thank your for taking the time to submit this.

Regarding the documentation, we have the cert-manager website here:

The page that comes to mind for me would be here:

And adding even a commented out example of using the caBundleSecretRef:

apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
  name: tpp-issuer
  namespace: <NAMESPACE YOU WANT TO ISSUE CERTIFICATES IN>
spec:
  venafi:
    zone: \VED\Policy\devops\cert-manager # Set this to the Venafi policy folder you want to use
    tpp:
      url: https://tpp.venafi.example/vedsdk # Change this to the URL of your TPP instance
      caBundle: <base64 encoded string of caBundle PEM file, or empty to use system root CAs>
      # Use only caBundle above or the caBundleSecretRef below
      # caBundleSecretRef:
      #  name: custom-tpp-ca
      #  key: ca.crt
      credentialsRef:
        name: tpp-secret

When you raise a PR for the docs update, please raise it against the release-next branch, and not master :)

sankalp-at-gh commented 3 months ago

Perfect, I'll keep the doc updates going while this PR is reviewed 🙂 Thank you @hawksight !

wallrj commented 1 month ago

@hawksight @sankalp-at-gh @SimeonPoot A pre-release is now available containing the fix for this issue. Please test and feedback if you have time.