-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
External Certificate Manager #135
Conversation
087-external-certificate-manager.md
Outdated
|
||
The proposal makes a few assumptions: | ||
* Strimzi will not be responsible for installing cert-manager, but we will document the versions of cert-manager that we have tested with. | ||
* Strimzi will not be responsible for creating `Issuer` or `ClusterIssuer` custom resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess this is because we want to keep for self-signed certs our current way and not to add another option that will mostly add just support burden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because there are lots of different issuers that work with cert-manager. So rather than Strimzi having to actively support all the different types, I've proposed that the user creates the Issuer
or ClusterIssuer
and handles supplying a Secret with the trusted certificates for the issuer they have chosen. That way Strimzi can work with any cert-manager issuer integrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we need to provide any guidelines on conventions in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just need to mention in in the docs properly as users could be confused from different projects where integration of CM works without creating any Issuer (afaiu operator creates self-sign Issuer when it is not created by users).
087-external-certificate-manager.md
Outdated
|
||
If the user has enabled this feature, when Strimzi needs to issue a certificate, instead of using the existing internal mechanism it will create a `Certificate` custom resource. | ||
Strimzi will wait during the reconciliation loop for the `Certificate` status to indicate that the certificate has been issued before continuing. | ||
When issuing cluster certificates (e.g for Kafka etc), once the certificate has been issued, Strimzi will annotate the cert-manager provided Secret with the `strimzi.io/server-cert-hash` annotation with the value being the hash of the certificate in the Secret. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issuing cluster certificates (e.g for Kafka etc)
- I wonder if it would be useful to include the secret names for these certificates as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposal Kate, it looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some nits. But the main thing missing seems to be how you plan to roll out the trust to the new CAs. There is no reference to strimzi.io/ca-key-generation
or to rotation of private keys, so it is not clear how do you expect to handle it.
087-external-certificate-manager.md
Outdated
Although it is nice that it can manage certificates, it would be beneficial if the certificates could be managed by a dedicated certificate manager, such as [cert-manager](https://cert-manager.io/). | ||
This is a feature that is often requested, especially because many organizations have specific compliance requirements with regard to certificates, for example: | ||
* Requiring that CA private keys are not shared. | ||
* Requiring that self-signed certificates cannot be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this really helps because the CA will be anyway bootstrapped as self-signed as it is today in most cases, and there is not much we can do about it.
087-external-certificate-manager.md
Outdated
3. Create a `Secret` containing the CAs for Strimzi to trust. | ||
Users can optionally use [trust-manager](https://cert-manager.io/docs/trust/trust-manager/) to create this Secret, but they are responsible for installing trust-manager, creating the `Bundle` CR and annotating the resulting Secret with the Strimzi cert annotation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should instead (or next to this) describe the expectation of how the Secret should look like? IIRC, trust-manager creates a Secret will all CAs bundled into a single file? Is that supported / expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this needs some clarification. Maybe a sequence diagram with the PKI generation process including user, CO, trust-manager and cert-manager would also help.
087-external-certificate-manager.md
Outdated
For cluster certificates (e.g. for Kafka etc), Strimzi will track and handle these changes using the `strimzi.io/server-cert-hash` annotation. | ||
During the reconciliation loop, even if all cluster end-entity certificates have been issued, Strimzi will patch the certificate Secrets with the correct `strimzi.io/server-cert-hash` annotation. | ||
The value of this annotation can then be compared with the value on the pods to determine whether the pods need to be restarted to pick up a new Secret. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow the need to annotate the Secrets here. Normally, during the reconciliation:
- You take the hash of the certiicate
- Use the Hash to annotate the Pod in the Deployment / StrimziPodSet
- Either Kubernetes or Strimzi takes care of rolling the pod based on the Pod annotations being different
087-external-certificate-manager.md
Outdated
|
||
### Issuing certificates | ||
|
||
If the user has enabled this feature, when Strimzi needs to issue a certificate, instead of using the existing internal mechanism it will create a `Certificate` custom resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would be very very useful to have this implementation behind an interface and support a mechanism for loading alternative implementations for other external certificate managers. This would allow users to integrate with other external certificate managers
087-external-certificate-manager.md
Outdated
certificateExpirationPolicy: <renew-certificate|replace-key> | ||
certificateIssuer: | ||
type: <internal|cert-manager.io> # (1) | ||
issuerRef: # (2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issuerRef element looks to be cert-manager specific rather than something relevant to any external cert manager which could become an issue when supporting other external managers (as mentioned above).
Perhaps the certificateIssuer should instead have a certManager specific sub-element, and add different similar elements in the future that are specific to other certificate managers, i.e. something like:
clusterCa:
certificateIssuer:
certManager:
issuerRef:
name: <string>
kind: <Issuer|ClusterIssuer>
group: <string> # cert-manager.io by default
someOtherManager: <-- future addition -->
managerSpecificConfig:
...
oneOf:
- properties
certManager{}
someOtherManager{}
Or alternatively just allow a map of values to be specified, but that would be less user friendly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The property
certificateIssuer.issuerRef
will only be used by Strimzi ifcertificateIssuer.type
is set tocert-manager.io
.
From the above phrase it looks like the intention is to make issuerRef
cert-manger specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I made a few suggested changes to wording for clarity and readability. There are also a couple of questions for further clarification.
087-external-certificate-manager.md
Outdated
|
||
The proposal makes a few assumptions: | ||
* Strimzi will not be responsible for installing cert-manager, but we will document the versions of cert-manager that we have tested with. | ||
* Strimzi will not be responsible for creating `Issuer` or `ClusterIssuer` custom resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we need to provide any guidelines on conventions in the docs?
@scholzj @tinaselenge @fvaleri @PaulRMellor @ppatierno @Frawless Thanks for all your comments. I've pushed an update to the proposal, the main changes apart from wording tweaks are:
I've left the User operator/clients CA part as TODO at the moment but I would appreciate any feedback on the cluster CA part of the proposal. I also do have a full diagram of a key replacement, but am still working on making it display in a way that's viewable. Let me know if that would be useful, or if the existing diagrams are clear enough. |
@MichaelMorrisEst thanks for your comments. On the extensibility of the design, I've tried to write both the CRD and the way it's implemented such that we could add other certificate management options in future. However, I wasn't planning for it to be something a user can add at deploy time. My expectation is that we would add it directly to the codebase. The reason being that it is easier to reason about what is supported and make sure it's properly tested. Is that what you were expecting when you were asking about alternative implementations? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates Kate.
The plus of this solution is to build up on the existing rollout logic, which is proven and well tested.
New diagrams are great. I see why you didn't put them inline, but maybe we can link them at the end of related sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. I left some more comments, mainly about the formal side and some possibly missing scenarios being described.
087-external-certificate-manager.md
Outdated
publicCert: # (3) | ||
secretName: <string> | ||
certificate: <string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this support a list with multiple Secrets? Should it be ready to handle other forms of certificates such as ConfigMap? E.g.:
publicCert: # (3)
- fromSecret:
secretName: <string>
certificate: <string>
This would:
- Allow to possibly load trusted certificates from more Secrets - would that be useful when migrating to a new CA/Issuer or something like that?
- Allow to extend the type in the future to something else than Secret (we can start with Secret only today, but we will have the option)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current proposal doesn't allow for more than one certificate here. The proposed flow requires us to associate a generation id with one certificate. This field is for the latest trusted certificate, Strimzi will keep the previous certificate until it is sure everything trusts the new one. So I don't think we need to support more than one here? But could add it incase this changes in future, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't think we need to support more than one here? But could add it incase this changes in future, wdyt?
Hmm 🤔. I don't know. It obviously might be useful one day in the future ... but at the same time, if you need exactly one today, it would make it harder to validate things etc. ... and all of that for something what we might never use. So not sure what is the best approach in such a case. 🤷
As an open source project, it will only ever be possible to add open source certificate management options directly in the Strimzi code base. I think it would be very useful to enable other options to be integrated without code changes. |
@MichaelMorrisEst I think open-source project is one aspect to consider. The other one is that it needs to have enough demand to justify the effort and maintenance. So, I think the extensibility makes sense. The way that could be done would be to add some additional type I guess there are some dirty alternatives ...
These seem a bit more hacky. But I think they would provide a stable interface. The whole problem moves to entirely different level when you would need to change the certificate handling completely including for example not storing them in Kubernetes Secrets (but loading them from something like Vault). As that would require the plugabiity not only in the operator code but also Kafka (Config Providers? Shell scripts?), Cruise Control (Shell scripts) or Kafka Exporter (Shell scripts) and so on. I'm not sure we would ever be able to get there - but who knows? |
@scholzj Of the other suggestions the dependency on Cert Manager for the first suggestion limits its applicability, the second (custom operator for Cert Manager custom resources) looks more workable but it does seem a bit hacky |
@ppatierno @scholzj I've pushed some updates and added the Clients CA/User Operator section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more nits. mostly nits apart from some things around the User OPerator.
087-external-certificate-manager.md
Outdated
publicCert: # (3) | ||
secretName: <string> | ||
certificate: <string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't think we need to support more than one here? But could add it incase this changes in future, wdyt?
Hmm 🤔. I don't know. It obviously might be useful one day in the future ... but at the same time, if you need exactly one today, it would make it harder to validate things etc. ... and all of that for something what we might never use. So not sure what is the best approach in such a case. 🤷
087-external-certificate-manager.md
Outdated
During the next reconciliation Strimzi will: | ||
1. Copy over the new CA public cert (keeping the old one as described previously) and update the hash and generation annotations. | ||
2. Roll the Kafka pods to trust the new CA cert, incrementing the `strimzi.io/cluster-ca-key-generation` annotation on the pods. | ||
3. Copy over the new Kafka certificates, updating the `strimzi.io/cluster-ca-cert-generation` annotations on the Secrets. | ||
4. Roll the Kafka pods to use the new Kafka certificates, updating the `strimzi.io/cluster-ca-cert-generation` annotations on the Pods. | ||
5. Then on the next reconciliation, since the Kafka pods now have correct cert and key generation, copy over the new operator certificate. | ||
6. Since the Kafka pods now have correct cert generation, remove the old CA public cert from the `<CLUSTER_NAME>-cluster-ca-cert` Secret | ||
|
||
>  | ||
> | ||
> Fig 2: Existing and proposed workflow when the user provides a new cluster CA public cert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you cover at which stage will this be done? Will that be done somewhere at the beginning of today's CaReconciler (and thus the first reocnciliation after unpausing will immediately start taking the steps to replace the CA)? Or soemwhere later in the reconciliation and essentially just prepare things for the next reconciliation?
087-external-certificate-manager.md
Outdated
> | ||
> Fig 1: Proposed workflow when cert-manager issues new component end-entity certificates | ||
|
||
#### Handling CA key replacements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any difference between CA key replacement here and simple CA public key renewal? Given the later seems not to be covered in any other section, I assume all public key renewals are handled in the same way as key replacements (as we do with custom CAs today)? Assuming that is the case, it would be worth clarifying it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment matches pretty well with my concern about increasing the key generation on cert renewal when we don't really know if the key is changed or not.
087-external-certificate-manager.md
Outdated
When a `KafkaUser` with `spec.authentication.type` set to `tls` is creates Strimzi will create a `Certificate` custom resource. | ||
Strimzi will specify the required CN/SANS in the `Certificate` resource for the user certificate. | ||
Strimzi will specify the Secret for the certificate to be stored in as the existing name for the certificate Secret suffixed with `-cm`, for example `<USER_NAME>-cm`. | ||
Strimzi will request the certificate in both PEM and PKCS12 format. | ||
Strimzi will wait for the usual operation timeout during the reconciliation loop for the `Certificate` status to indicate that the certificate has been issued before continuing. | ||
Once the certificate has been issued, Strimzi will copy the certificate across from the cert-manager provided Secret into its own existing Secret. | ||
Strimzi will also copy across the ClientsCA public certificate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still want to do the copying here? Or do we want the user to get it from the CM or (whoever actually produces it) directly? For the server certificates and CAs, we depend on them so we want to copy them out. The user certificate secrets actually exist only for the users. We do not care about them much otherwise. So I wonder if keeping them twice makes sense?
But maybe we still need it to track the generations etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @katheris.
087-external-certificate-manager.md
Outdated
The option `cert-manager.io` will only be valid if `generateCertificateAuthority` is set to `false`. | ||
2. The property `certManagerIssuerRef` will only be used by Strimzi if `type` is set to `cert-manager.io`. | ||
The `name`, `kind`, and `group` properties will be copied over into the `Certificate` custom resource Strimzi creates. | ||
3. The property `publicCert` will only be used by Strimzi if `type` is set to `cert-manager.io`. | ||
The `secretName` and `certificate` properties will be used to locate the CA public certificate that must be trusted by Strimzi components in order to trust the end-entity certificates that cert-manager issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leverage the new CEL validation feature to do these checks at the API level. Not sure you want to mention this implementation detail, but I wanted to leave a note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, thanks :)
087-external-certificate-manager.md
Outdated
> | ||
> Fig 1: Proposed workflow when cert-manager issues new component end-entity certificates | ||
|
||
#### Handling CA key replacements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment matches pretty well with my concern about increasing the key generation on cert renewal when we don't really know if the key is changed or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not security SME, but the feature proposal sounds good to me. +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments, Kate.
I found the addition of the diagrams useful in showing the new flows.
lgtm, nice work @katheris |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the patience and effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you very much for the awesome work @katheris! And sorry to be so pedantic :-)
This proposal has now 4 (binding) +1 and 2 (non-binding) +1. If there are no more comments by the end of tomorrow I will merge it. @katheris can you changed the index on the file names, images and update the index file as well please? (from 87 it should be 99) |
Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
* Update to copy certificates from cert-manager Secrets, not mount them. * Handle trust rollout more directly. * Apply wording suggestions. * Add diagrams. Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
* Update the API to use strimzi.io as the default type. * Update the user operator section to use the cert-manager Secret directly, rather than creating a copy. Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
* Address review comments from PaulRMellor, ppatierno and fvaleri. * Update Cluster CA flow so user does not need to pause Strimzi when replacing the CA private key. Instead use cert path validation to identify when a key has been replaced and when new end-entity certificates are not trusted yet. Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
Signed-off-by: Katherine Stanley <11195226+katheris@users.noreply.github.com>
4552065
to
f34e106
Compare
Done :) |
I got the wrong index ... congrats for making the 100th proposal! :-) |
This proposal now has 5 (binding) +1 and 2 (non-binding) +1. Approved! |
This proposal aims to allow Strimzi users to use an external certificate manager, specifically cert-manager, to manage certificates.
Related to strimzi/strimzi-kafka-operator#929