Fix renewal of user provided CAs #6180

ppatierno · 2022-01-13T15:20:27Z

Type of change

Bugfix

Description

This PR fixes #5466.
Today, when a user provides his own CA certificate (cluster or client) but then he tries to follow the renew process as we documented, the procedure doesn't work at all.
The current code doesn't have logic for the CO to detect that a new CA certificate was provided by the user (by following the documentation and having a Secret with both the new CA certificate together with the old one CA-).
With this PR, this first detection issue is resolved by adding the thumbprint of the cluster CA certificate, as an annotation, on all the Secret(s) that contains server certificates signed by that CA certificate. For example, the <cluster>-zookeeper-nodes, <cluster>-kafka-brokers and the same for the CO and EO (TO and UO) related Secret(s).

annotations:
    clients-ca-thumbprint: hCyUdeqWt6gXDUKYdAtaFYDiPoZlWAwCjW7liqtWkps=

Just for the Kafka brokers related Secret(s) there is also a corresponding annotation with the thumbprint of the clients CA certificate.

annotations:
    clients-ca-thumbprint: hCyUdeqWt6gXDUKYdAtaFYDiPoZlWAwCjW7liqtWkps=
    cluster-ca-thumbprint: DoNswf/bRfs6nF1IVx7PhbOVXKvJvjv/wOxuQY8eoTM=

During the periodic reconcile, the CO is able to detect that the thumbprint of the provided CA certificate is different from the one stored in such Secret(s).
When the change is detected, it just sets the renewal type as REPLACE_KEY, starting the same flow which runs when the CA certificates are handled by Strimzi itself (of course a part the CA certificate generation that is skipped).
So it runs the right pods rolls to first trust the new CA certificate and then generating the new server certificates for the different components (CO itself, Kafka, ZooKeeper, TO, UO).

In order to do so there are a couple of things to take into account that needs the documentation to be updated because the user has to follow them in order to renew the CA certificate properly:

The CN (Common Name) of the current CA certificate and the renewed one have to be different. This is because when they will be both trusted, the openssl tool used by stunnel in the tls-sidecar (in the TO) is not able to use both of them to trust the ZooKeeper server certificates but just one of them, so the verification would fail. When the Strimzi auto-creation of CA certificate is used, this is exactly what happens; a new CA is created with a suffix of v1 (compared to the previous v0) in the CN to make them different, so manual renewal should follow same path.
The Secret containing the CA certificate has to have the strimzi.io/ca-cert-generation annotation as well as the one containing the key has to have the strimzi.io/ca-key-generation. This generation should start from 0 and increased on each renewal; even this time this is exactly how the auto-renewal process works in Strimzi doing that for you.

So the above steps need to be documented to make "handmade" renewal the same as the "auto" renewal from Strimzi operator.

Fixing this way, I made a couple of assumptions:

when renewing the CA certificate, the user will always create a new key (as we describe in the documentation) so the reason why the fix inject into the flow with a REPLACE_KEY renewal type.
the CA- is not deleted by the CO automatically when it expires (as it happens for Strimzi generated CA certificate). It's the user to delete it from the Secret.

Documentation will come with a different PR.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Make sure all tests pass
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ClusterCa.java

scholzj · 2022-01-14T00:03:11Z

The Secret containing the CA certificate has to have the strimzi.io/ca-cert-generation annotation as well as the one containing the key has to have the strimzi.io/ca-key-generation. This generation should start from 0 and increased on each renewal; even this time this is exactly how the auto-renewal process works in Strimzi doing that for you.

So, how does this deal with existing clusters which will not have the generation set? You can easily tell new users to set it. But we need to make sure that when an existing user wich custom CA secret without the generation upgrades, it doesn't fall all over.

.checkstyle/checkstyle.xml

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ClusterCa.java

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

scholzj · 2022-01-14T00:10:37Z

Just for the Kafka brokers related Secret(s) there is also a corresponding annotation with the thumbprint of the clients CA certificate.

This seems a bit weird ... since the secret has nothing to do with the Clients CA. Will it stay in sync? Because the secret will not change when the Clients CA is renewed.

ppatierno · 2022-01-17T14:21:49Z

This seems a bit weird ... since the secret has nothing to do with the Clients CA. Will it stay in sync? Because the secret will not change when the Clients CA is renewed.

But the thumbprint is update; the flow follows the Secret creation as usual by creating the same one but with a different thumbprint. I just added that to the usual build Secret creation in this case.

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java

tombentley · 2022-01-19T13:13:48Z

operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java

+            /*
+            if (isCaCertThumbprintChanged(secret)) {
+                reasons.add("new certificate generated");
+            }
+            */


I guess this should be removed?

operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ClusterCa.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ClusterCa.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

operator-common/src/main/java/io/strimzi/operator/cluster/model/ClientsCa.java

scholzj

One nit wich I think you should address. LGTM otherwise.

operator-common/src/main/java/io/strimzi/operator/cluster/model/ClientsCa.java

scholzj · 2022-01-21T17:25:05Z

/azp run regression

azure-pipelines · 2022-01-21T17:25:17Z

Azure Pipelines successfully started running 1 pipeline(s).

see-quick

The errors from Azure are not related to this PR and should be fixed here #6245

tombentley

Reading the companion doc PR (#6240) made me wonder whether we should really be using an extra annotation for the thumbprint when we already have the generation annotation. I'd like to understand if the generation on its own is enough to track the changing dependency (and if not why not).

ppatierno · 2022-01-24T11:24:02Z

@tombentley it's a good point ... thinking more about it, the generation annotation is on the CA certificate Secret only so when it's replaced by a new CA with a newer generation we lost that information and the CO cannot infer that CA is changed. The thumbprint annotation is on the Secrets (ZK, K, EO, CO, ...) containing server certificates signed with the CA certificate so that on reconcile we can compare the new CA thumbprint with the one on those Secrets. Using the generation could work but not out of box, we would need to store it on the Secrets (instead of the thumbprint) as well to be able to make a comparison and detect the CA certificate update.

ppatierno · 2022-01-24T14:54:05Z

@tombentley @scholzj I changed the code by saving the strimzi.io/cluster-ca-cert-generation and strimzi.io/clients-ca-cert-generation (only for Kafka) on Secrets instead of the thumbprint. I run some tests and of course CO is able to detect change because the user increments the generation on renewal. I don't see any drawbacks.

operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java

...rator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java

cluster-operator/src/test/java/io/strimzi/operator/cluster/model/CaRenewalTest.java

operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java

tombentley

Thanks @ppatierno

ppatierno · 2022-01-24T18:06:00Z

/azp run regression

azure-pipelines · 2022-01-24T18:06:14Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Paolo Patierno <[email protected]> Fixed unit test Signed-off-by: Paolo Patierno <[email protected]> Fixed NPE when no thumbprint annotation is on Secret Signed-off-by: Paolo Patierno <[email protected]> Factored out a common method to check CA cert thumbprint is changed Defined a method to return the annotation for CA cert thumbprint Signed-off-by: Paolo Patierno <[email protected]> Addressed different comments Signed-off-by: Paolo Patierno <[email protected]> Changed printing CA certificate thumbprint to DEBUG log level Signed-off-by: Paolo Patierno <[email protected]>

Signed-off-by: Paolo Patierno <[email protected]>

Trivial methods refactoring Signed-off-by: Paolo Patierno <[email protected]>

ppatierno · 2022-01-25T09:50:02Z

/azp run regression

azure-pipelines · 2022-01-25T09:50:14Z

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno requested review from tombentley and scholzj January 13, 2022 15:20

ppatierno added this to the 0.28.0 milestone Jan 13, 2022

see-quick reviewed Jan 13, 2022

View reviewed changes

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ClusterCa.java Outdated Show resolved Hide resolved

scholzj reviewed Jan 14, 2022

View reviewed changes

ppatierno marked this pull request as ready for review January 17, 2022 14:47

ppatierno force-pushed the fix-user-ca-renewal branch 3 times, most recently from 9889067 to 04b20dc Compare January 18, 2022 14:50

tombentley reviewed Jan 19, 2022

View reviewed changes

scholzj reviewed Jan 19, 2022

View reviewed changes

ppatierno force-pushed the fix-user-ca-renewal branch 2 times, most recently from 1541ad0 to 61ca1be Compare January 21, 2022 08:12

ppatierno mentioned this pull request Jan 21, 2022

Fix documentation about install and renew your own CA certificates #6240

Merged

1 task

scholzj approved these changes Jan 21, 2022

View reviewed changes

operator-common/src/main/java/io/strimzi/operator/cluster/model/ClientsCa.java Outdated Show resolved Hide resolved

see-quick approved these changes Jan 22, 2022

View reviewed changes

tombentley reviewed Jan 24, 2022

View reviewed changes

tombentley approved these changes Jan 24, 2022

View reviewed changes

scholzj approved these changes Jan 24, 2022

View reviewed changes

ppatierno added 3 commits January 25, 2022 09:38

Renamed ClientsCa method to set broker secret

2dc4932

Signed-off-by: Paolo Patierno <[email protected]>

Refactored to use CA certificate generation instead of thumbprint

3448225

Signed-off-by: Paolo Patierno <[email protected]>

Factored out a MockedCa for unit tests

dd9e48b

Trivial methods refactoring Signed-off-by: Paolo Patierno <[email protected]>

ppatierno force-pushed the fix-user-ca-renewal branch from eba09a9 to dd9e48b Compare January 25, 2022 08:39

ppatierno merged commit 6b20f3e into strimzi:main Jan 25, 2022

ppatierno deleted the fix-user-ca-renewal branch January 25, 2022 15:59

ppatierno changed the title ~~Added fix for renewal of user provided CAs~~ Fix renewal of user provided CAs Jan 25, 2022

scholzj mentioned this pull request Jul 24, 2024

Avoid unnecessary rolling updates when replacing custom CA #10377

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix renewal of user provided CAs #6180

Fix renewal of user provided CAs #6180

ppatierno commented Jan 13, 2022 •

edited

Loading

scholzj commented Jan 14, 2022

scholzj commented Jan 14, 2022

ppatierno commented Jan 17, 2022

tombentley Jan 19, 2022

scholzj left a comment

scholzj commented Jan 21, 2022

azure-pipelines bot commented Jan 21, 2022

see-quick left a comment

tombentley left a comment

ppatierno commented Jan 24, 2022

ppatierno commented Jan 24, 2022

tombentley left a comment

ppatierno commented Jan 24, 2022

azure-pipelines bot commented Jan 24, 2022

ppatierno commented Jan 25, 2022

azure-pipelines bot commented Jan 25, 2022

Fix renewal of user provided CAs #6180

Fix renewal of user provided CAs #6180

Conversation

ppatierno commented Jan 13, 2022 • edited Loading

Type of change

Description

Checklist

scholzj commented Jan 14, 2022

scholzj commented Jan 14, 2022

ppatierno commented Jan 17, 2022

tombentley Jan 19, 2022

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

scholzj commented Jan 21, 2022

azure-pipelines bot commented Jan 21, 2022

see-quick left a comment

Choose a reason for hiding this comment

tombentley left a comment

Choose a reason for hiding this comment

ppatierno commented Jan 24, 2022

ppatierno commented Jan 24, 2022

tombentley left a comment

Choose a reason for hiding this comment

ppatierno commented Jan 24, 2022

azure-pipelines bot commented Jan 24, 2022

ppatierno commented Jan 25, 2022

azure-pipelines bot commented Jan 25, 2022

ppatierno commented Jan 13, 2022 •

edited

Loading