Skip to content

Commit

Permalink
Added fix for renewal of user provided CAs (#6180)
Browse files Browse the repository at this point in the history 
* Added fix for renewal of user provided CAs

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]>

* Renamed ClientsCa method to set broker secret

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

* Refactored to use CA certificate generation instead of thumbprint

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

* Factored out a MockedCa for unit tests
Trivial methods refactoring

Signed-off-by: Paolo Patierno <[email protected]>
  • Loading branch information
@ppatierno
ppatierno authored Jan 25, 2022
1 parent c8793a2 commit 6b20f3e
Show file tree
Hide file tree
Showing 14 changed files with 195 additions and 146 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* Add support for Kafka 3.1.0; remove Kafka 2.8.0 and 2.8.1
* Update Open Policy Agent authorizer to 1.4.0 and add support for enabling metrics
* Added the option `createBootstrapService` in the Kafka Spec to disable the creation of the bootstrap service for the Load Balancer Type Listener. It will save the cost of one load balancer resource, specially in the public cloud.
* Fix renewing your own CA certificates [#5466](https://github.com/strimzi/strimzi-kafka-operator/issues/5466)

## 0.27.0

Expand Down
4 changes: 4 additions & 0 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/AbstractModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -979,6 +979,10 @@ protected Secret createSecret(String name, Map<String, String> data) {
return ModelUtils.createSecret(name, namespace, labels, createOwnerReference(), data, emptyMap(), emptyMap());
}

protected Secret createSecret(String name, Map<String, String> data, Map<String, String> customAnnotations) {
return ModelUtils.createSecret(name, namespace, labels, createOwnerReference(), data, customAnnotations, emptyMap());
}

protected Secret createJmxSecret(String name, Map<String, String> data) {
return ModelUtils.createSecret(name, namespace, labels, createOwnerReference(), data, templateJmxSecretAnnotations, templateJmxSecretLabels);
}
Expand Down
15 changes: 15 additions & 0 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ClusterCa.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,4 +241,19 @@ public Map<String, CertAndKey> generateBrokerCerts(Kafka kafka, Set<String> exte
isMaintenanceTimeWindowsSatisfied);
}

@Override
protected String caCertGenerationAnnotation() {
return ANNO_STRIMZI_IO_CLUSTER_CA_CERT_GENERATION;
}

@SuppressWarnings("BooleanExpressionComplexity")
@Override
protected boolean hasCaCertGenerationChanged() {
// at least one Secret has a different cluster CA certificate thumbprint.
// it is useful when a renewal cluster CA certificate process needs to be recovered after an operator crash
return hasCaCertGenerationChanged(zkNodesSecret) || hasCaCertGenerationChanged(brokersSecret) ||
hasCaCertGenerationChanged(entityTopicOperatorSecret) || hasCaCertGenerationChanged(entityUserOperatorSecret) ||
hasCaCertGenerationChanged(kafkaExporterSecret) || hasCaCertGenerationChanged(cruiseControlSecret) ||
hasCaCertGenerationChanged(clusterOperatorSecret);
}
}
3 changes: 2 additions & 1 deletion cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,8 @@ public Secret generateSecret(Kafka kafka, ClusterCa clusterCa, boolean isMainten
data.put(keyCertName + ".p12", cert.keyStoreAsBase64String());
data.put(keyCertName + ".password", cert.storePasswordAsBase64String());

return createSecret(CruiseControl.secretName(cluster), data);
return createSecret(CruiseControl.secretName(cluster), data,
Collections.singletonMap(clusterCa.caCertGenerationAnnotation(), String.valueOf(clusterCa.certGeneration())));
}

/**
Expand Down
10 changes: 8 additions & 2 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1357,9 +1357,11 @@ public StrimziPodSet generatePodSet(int replicas,
* internal communication with Zookeeper.
* It also contains the related Kafka brokers private keys.
*
* @param clusterCa The CA for cluster certificates
* @param clientsCa The CA for clients certificates
* @return The generated Secret
*/
public Secret generateBrokersSecret() {
public Secret generateBrokersSecret(ClusterCa clusterCa, ClientsCa clientsCa) {

Map<String, String> data = new HashMap<>(replicas * 4);
for (int i = 0; i < replicas; i++) {
Expand All @@ -1369,7 +1371,11 @@ public Secret generateBrokersSecret() {
data.put(KafkaCluster.kafkaPodName(cluster, i) + ".p12", cert.keyStoreAsBase64String());
data.put(KafkaCluster.kafkaPodName(cluster, i) + ".password", cert.storePasswordAsBase64String());
}
return createSecret(KafkaCluster.brokersSecretName(cluster), data);

Map<String, String> annotations = Map.of(
clusterCa.caCertGenerationAnnotation(), String.valueOf(clusterCa.certGeneration()),
clientsCa.caCertGenerationAnnotation(), String.valueOf(clientsCa.certGeneration()));
return createSecret(KafkaCluster.brokersSecretName(cluster), data, annotations);
}

/**
Expand Down
7 changes: 5 additions & 2 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ public static Secret buildSecret(Reconciliation reconciliation, ClusterCa cluste
reasons.add("certificate doesn't exist yet");
shouldBeRegenerated = true;
} else {
if (clusterCa.keyCreated() || clusterCa.certRenewed() || (isMaintenanceTimeWindowsSatisfied && clusterCa.isExpiring(secret, keyCertName + ".crt"))) {
if (clusterCa.keyCreated() || clusterCa.certRenewed() ||
(isMaintenanceTimeWindowsSatisfied && clusterCa.isExpiring(secret, keyCertName + ".crt")) ||
clusterCa.hasCaCertGenerationChanged(secret)) {
reasons.add("certificate needs to be renewed");
shouldBeRegenerated = true;
}
Expand Down Expand Up @@ -191,7 +193,8 @@ public static Secret buildSecret(Reconciliation reconciliation, ClusterCa cluste
data.put(keyCertName + ".password", certAndKey.storePasswordAsBase64String());
}

return createSecret(secretName, namespace, labels, ownerReference, data, emptyMap(), emptyMap());
return createSecret(secretName, namespace, labels, ownerReference, data,
Collections.singletonMap(clusterCa.caCertGenerationAnnotation(), String.valueOf(clusterCa.certGeneration())), emptyMap());
}

public static Secret createSecret(String name, String namespace, Labels labels, OwnerReference ownerReference,
Expand Down
7 changes: 5 additions & 2 deletions cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ZookeeperCluster.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,9 +576,10 @@ public void generateCertificates(Kafka kafka, ClusterCa clusterCa, boolean isMai
* internal communication with Kafka.
* It also contains the related Zookeeper nodes private keys.
*
* @param clusterCa The CA for cluster certificates
* @return The generated Secret.
*/
public Secret generateNodesSecret() {
public Secret generateNodesSecret(ClusterCa clusterCa) {

Map<String, String> data = new HashMap<>(replicas * 4);
for (int i = 0; i < replicas; i++) {
Expand All @@ -588,7 +589,9 @@ public Secret generateNodesSecret() {
data.put(ZookeeperCluster.zookeeperPodName(cluster, i) + ".p12", cert.keyStoreAsBase64String());
data.put(ZookeeperCluster.zookeeperPodName(cluster, i) + ".password", cert.storePasswordAsBase64String());
}
return createSecret(ZookeeperCluster.nodesSecretName(cluster), data);

return createSecret(ZookeeperCluster.nodesSecretName(cluster), data,
Collections.singletonMap(clusterCa.caCertGenerationAnnotation(), String.valueOf(clusterCa.certGeneration())));
}

@Override
Expand Down
11 changes: 7 additions & 4 deletions ...or/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaAssemblyOperator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,7 @@ Future<ReconciliationState> reconcileCas(Supplier<Date> dateSupplier) {
Secret clusterCaKeySecret = null;
Secret clientsCaCertSecret = null;
Secret clientsCaKeySecret = null;
Secret brokersSecret = null;
List<Secret> clusterSecrets = secretOperations.list(reconciliation.namespace(), selectorLabels);
for (Secret secret : clusterSecrets) {
String secretName = secret.getMetadata().getName();
Expand All @@ -622,6 +623,8 @@ Future<ReconciliationState> reconcileCas(Supplier<Date> dateSupplier) {
clientsCaCertSecret = secret;
} else if (secretName.equals(clientsCaKeyName)) {
clientsCaKeySecret = secret;
} else if (secretName.equals(KafkaCluster.brokersSecretName(name))) {
brokersSecret = secret;
}
}
OwnerReference ownerRef = new OwnerReferenceBuilder()
Expand Down Expand Up @@ -654,14 +657,13 @@ Future<ReconciliationState> reconcileCas(Supplier<Date> dateSupplier) {
ModelUtils.getCertificateValidity(clusterCaConfig),
ModelUtils.getRenewalDays(clusterCaConfig),
clusterCaConfig == null || clusterCaConfig.isGenerateCertificateAuthority(), clusterCaConfig != null ? clusterCaConfig.getCertificateExpirationPolicy() : null);
this.clusterCa.initCaSecrets(clusterSecrets);
clusterCa.createRenewOrReplace(
reconciliation.namespace(), reconciliation.name(), caLabels.toMap(),
clusterCaCertLabels, clusterCaCertAnnotations,
clusterCaConfig != null && !clusterCaConfig.isGenerateSecretOwnerReference() ? null : ownerRef,
isMaintenanceTimeWindowsSatisfied(dateSupplier));

this.clusterCa.initCaSecrets(clusterSecrets);

CertificateAuthority clientsCaConfig = kafkaAssembly.getSpec().getClientsCa();

// When we are not supposed to generate the CA but it does not exist, we should just throw an error
Expand All @@ -674,6 +676,7 @@ Future<ReconciliationState> reconcileCas(Supplier<Date> dateSupplier) {
ModelUtils.getCertificateValidity(clientsCaConfig),
ModelUtils.getRenewalDays(clientsCaConfig),
clientsCaConfig == null || clientsCaConfig.isGenerateCertificateAuthority(), clientsCaConfig != null ? clientsCaConfig.getCertificateExpirationPolicy() : null);
this.clientsCa.initBrokerSecret(brokersSecret);
clientsCa.createRenewOrReplace(reconciliation.namespace(), reconciliation.name(),
caLabels.toMap(), emptyMap(), emptyMap(),
clientsCaConfig != null && !clientsCaConfig.isGenerateSecretOwnerReference() ? null : ownerRef,
Expand Down Expand Up @@ -1357,7 +1360,7 @@ Future<Boolean> updateCertificateSecretWithDiff(String secretName, Secret secret
}

Future<ReconciliationState> zkNodesSecret() {
return updateCertificateSecretWithDiff(ZookeeperCluster.nodesSecretName(name), zkCluster.generateNodesSecret())
return updateCertificateSecretWithDiff(ZookeeperCluster.nodesSecretName(name), zkCluster.generateNodesSecret(clusterCa))
.map(changed -> {
existingZookeeperCertsChanged = changed;
return this;
Expand Down Expand Up @@ -2771,7 +2774,7 @@ Future<ReconciliationState> kafkaAncillaryCm() {
}

Future<ReconciliationState> kafkaBrokersSecret() {
return updateCertificateSecretWithDiff(KafkaCluster.brokersSecretName(name), kafkaCluster.generateBrokersSecret())
return updateCertificateSecretWithDiff(KafkaCluster.brokersSecretName(name), kafkaCluster.generateBrokersSecret(clusterCa, clientsCa))
.map(changed -> {
existingKafkaCertsChanged = changed;
return this;
Expand Down
Loading

0 comments on commit 6b20f3e

Please sign in to comment.