Skip to content

Commit

Permalink
Refactored to use CA certificate generation instead of thumbprint
Browse files Browse the repository at this point in the history 
Signed-off-by: Paolo Patierno <[email protected]>
  • Loading branch information
@ppatierno
ppatierno committed Jan 25, 2022
1 parent 2dc4932 commit 3448225
Show file tree
Hide file tree
Showing 8 changed files with 57 additions and 43 deletions.
14 changes: 7 additions & 7 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 @@ -242,18 +242,18 @@ public Map<String, CertAndKey> generateBrokerCerts(Kafka kafka, Set<String> exte
}

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

@SuppressWarnings("BooleanExpressionComplexity")
@Override
protected boolean isCaCertThumbprintChanged() {
protected boolean isCaCertGenerationChanged() {
// 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 isCaCertThumbprintChanged(zkNodesSecret) || isCaCertThumbprintChanged(brokersSecret) ||
isCaCertThumbprintChanged(entityTopicOperatorSecret) || isCaCertThumbprintChanged(entityUserOperatorSecret) ||
isCaCertThumbprintChanged(kafkaExporterSecret) || isCaCertThumbprintChanged(cruiseControlSecret) ||
isCaCertThumbprintChanged(clusterOperatorSecret);
return isCaCertGenerationChanged(zkNodesSecret) || isCaCertGenerationChanged(brokersSecret) ||
isCaCertGenerationChanged(entityTopicOperatorSecret) || isCaCertGenerationChanged(entityUserOperatorSecret) ||
isCaCertGenerationChanged(kafkaExporterSecret) || isCaCertGenerationChanged(cruiseControlSecret) ||
isCaCertGenerationChanged(clusterOperatorSecret);
}
}
2 changes: 1 addition & 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 @@ -693,7 +693,7 @@ public Secret generateSecret(Kafka kafka, ClusterCa clusterCa, boolean isMainten
data.put(keyCertName + ".password", cert.storePasswordAsBase64String());

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

/**
Expand Down
5 changes: 3 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 @@ -1371,9 +1371,10 @@ public Secret generateBrokersSecret(ClusterCa clusterCa, ClientsCa clientsCa) {
data.put(KafkaCluster.kafkaPodName(cluster, i) + ".p12", cert.keyStoreAsBase64String());
data.put(KafkaCluster.kafkaPodName(cluster, i) + ".password", cert.storePasswordAsBase64String());
}

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

Expand Down
4 changes: 2 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 @@ -146,7 +146,7 @@ public static Secret buildSecret(Reconciliation reconciliation, ClusterCa cluste
} else {
if (clusterCa.keyCreated() || clusterCa.certRenewed() ||
(isMaintenanceTimeWindowsSatisfied && clusterCa.isExpiring(secret, keyCertName + ".crt")) ||
clusterCa.isCaCertThumbprintChanged(secret)) {
clusterCa.isCaCertGenerationChanged(secret)) {
reasons.add("certificate needs to be renewed");
shouldBeRegenerated = true;
}
Expand Down Expand Up @@ -194,7 +194,7 @@ public static Secret buildSecret(Reconciliation reconciliation, ClusterCa cluste
}

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

public static Secret createSecret(String name, String namespace, Labels labels, OwnerReference ownerReference,
Expand Down
3 changes: 2 additions & 1 deletion 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 @@ -589,8 +589,9 @@ public Secret generateNodesSecret(ClusterCa clusterCa) {
data.put(ZookeeperCluster.zookeeperPodName(cluster, i) + ".p12", cert.keyStoreAsBase64String());
data.put(ZookeeperCluster.zookeeperPodName(cluster, i) + ".password", cert.storePasswordAsBase64String());
}

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

@Override
Expand Down
16 changes: 8 additions & 8 deletions cluster-operator/src/test/java/io/strimzi/operator/cluster/model/CaRenewalTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,12 @@ protected CertAndKey generateSignedCert(Subject subject,
}

@Override
protected boolean isCaCertThumbprintChanged() {
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertThumbprintAnnotation() {
protected String caCertGenerationAnnotation() {
return null;
}
};
Expand Down Expand Up @@ -125,12 +125,12 @@ protected CertAndKey generateSignedCert(Subject subject,
}

@Override
protected boolean isCaCertThumbprintChanged() {
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertThumbprintAnnotation() {
protected String caCertGenerationAnnotation() {
return null;
}
};
Expand Down Expand Up @@ -221,12 +221,12 @@ protected CertAndKey generateSignedCert(Subject subject,
}

@Override
protected boolean isCaCertThumbprintChanged() {
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertThumbprintAnnotation() {
protected String caCertGenerationAnnotation() {
return null;
}
};
Expand Down Expand Up @@ -317,12 +317,12 @@ protected CertAndKey generateSignedCert(Subject subject,
}

@Override
protected boolean isCaCertThumbprintChanged() {
protected boolean isCaCertGenerationChanged() {
return false;
}

@Override
protected String caCertThumbprintAnnotation() {
protected String caCertGenerationAnnotation() {
return null;
}
};
Expand Down
48 changes: 30 additions & 18 deletions operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,6 @@ public abstract class Ca {
public static final String ANNO_STRIMZI_IO_CA_CERT_GENERATION = Annotations.STRIMZI_DOMAIN + "ca-cert-generation";
public static final String ANNO_STRIMZI_IO_CLUSTER_CA_CERT_GENERATION = Annotations.STRIMZI_DOMAIN + "cluster-ca-cert-generation";
public static final String ANNO_STRIMZI_IO_CLIENTS_CA_CERT_GENERATION = Annotations.STRIMZI_DOMAIN + "clients-ca-cert-generation";
public static final String ANNO_STRIMZI_IO_CLUSTER_CA_THUMBPRINT = Annotations.STRIMZI_DOMAIN + "cluster-ca-thumbprint";
public static final String ANNO_STRIMZI_IO_CLIENTS_CA_THUMBPRINT = Annotations.STRIMZI_DOMAIN + "clients-ca-thumbprint";
public static final int INIT_GENERATION = 0;

private final PasswordGenerator passwordGenerator;
Expand Down Expand Up @@ -512,12 +510,12 @@ public void createRenewOrReplace(String namespace, String clusterName, Map<Strin
X509Certificate currentCert = cert(caCertSecret, CA_CRT);
Map<String, String> certData;
Map<String, String> keyData;
int caCertGeneration = caCertSecret != null ? Annotations.intAnnotation(caCertSecret, ANNO_STRIMZI_IO_CA_CERT_GENERATION, INIT_GENERATION) : INIT_GENERATION;
int caKeyGeneration = caKeySecret != null ? Annotations.intAnnotation(caKeySecret, ANNO_STRIMZI_IO_CA_KEY_GENERATION, INIT_GENERATION) : INIT_GENERATION;
int caCertGeneration = certGeneration();
int caKeyGeneration = keyGeneration();
if (!generateCa) {
certData = caCertSecret != null ? caCertSecret.getData() : emptyMap();
keyData = caKeySecret != null ? singletonMap(CA_KEY, caKeySecret.getData().get(CA_KEY)) : emptyMap();
renewalType = isCaCertThumbprintChanged() ? RenewalType.REPLACE_KEY : RenewalType.NOOP;
renewalType = isCaCertGenerationChanged() ? RenewalType.REPLACE_KEY : RenewalType.NOOP;
caCertsRemoved = false;
} else {
this.renewalType = shouldCreateOrRenew(currentCert, namespace, clusterName, maintenanceWindowSatisfied);
Expand Down Expand Up @@ -777,6 +775,20 @@ public boolean keyCreated() {
return renewalType.equals(RenewalType.CREATE);
}

/**
* @return the generation of the current CA certificate
*/
public int certGeneration() {
return caCertSecret != null ? Annotations.intAnnotation(caCertSecret, ANNO_STRIMZI_IO_CA_CERT_GENERATION, INIT_GENERATION) : INIT_GENERATION;
}

/**
* @return the generation of the current CA key
*/
public int keyGeneration() {
return caKeySecret != null ? Annotations.intAnnotation(caKeySecret, ANNO_STRIMZI_IO_CA_KEY_GENERATION, INIT_GENERATION) : INIT_GENERATION;
}

private int removeExpiredCerts(Map<String, String> newData) {
Iterator<Map.Entry<String, String>> iter = newData.entrySet().iterator();
List<String> removed = new ArrayList<>();
Expand Down Expand Up @@ -1026,34 +1038,34 @@ private void renewCaCert(Subject subject, Map<String, String> certData) {
}

/**
* @return the name of the annotation bringing the thumbprint of the specific CA certificate type (cluster or clients)
* @return the name of the annotation bringing the generation of the specific CA certificate type (cluster or clients)
* on the Secrets containing certificates signed by that CA (i.e ZooKeeper nodes, Kafka brokers, ...)
*/
protected abstract String caCertThumbprintAnnotation();
protected abstract String caCertGenerationAnnotation();

/**
* @return if the current (cluster or clients) CA certificate thumbprint is changed compared to the the one
* @return if the current (cluster or clients) CA certificate generation is changed compared to the the one
* brought on Secrets containing certificates signed by that CA (i.e ZooKeeper nodes, Kafka brokers, ...)
*/
protected abstract boolean isCaCertThumbprintChanged();
protected abstract boolean isCaCertGenerationChanged();

/**
* It checks if the current (cluster or clients) CA certificate thumbprint is changed compared to the the one
* It checks if the current (cluster or clients) CA certificate generation is changed compared to the one
* brought by the corresponding annotation on the provided Secret (i.e ZooKeeper nodes, Kafka brokers, ...)
*
* @param secret Secret containing certificates signed by the current (clients or cluster) CA
* @return if the current (cluster or clients) CA certificate thumbprint is changed compared to the the one
* @return if the current (cluster or clients) CA certificate generation is changed compared to the one
* brought by the corresponding annotation on the provided Secret
*/
protected boolean isCaCertThumbprintChanged(Secret secret) {
protected boolean isCaCertGenerationChanged(Secret secret) {
if (secret != null) {
String caCertThumbprintAnno = Optional.ofNullable(secret.getMetadata().getAnnotations())
.map(annotations -> annotations.get(caCertThumbprintAnnotation()))
String caCertGenerationAnno = Optional.ofNullable(secret.getMetadata().getAnnotations())
.map(annotations -> annotations.get(caCertGenerationAnnotation()))
.orElse(null);
String currentCaCertThumbprint = currentCaCertThumbprint();
LOGGER.debugOp("Secret {}/{} thumbprint anno = {}, current CA thumbprint = {}",
secret.getMetadata().getNamespace(), secret.getMetadata().getName(), caCertThumbprintAnno, currentCaCertThumbprint);
return caCertThumbprintAnno != null && !caCertThumbprintAnno.equals(currentCaCertThumbprint);
int currentCaCertGeneration = certGeneration();
LOGGER.debugOp("Secret {}/{} generation anno = {}, current CA generation = {}",
secret.getMetadata().getNamespace(), secret.getMetadata().getName(), caCertGenerationAnno, currentCaCertGeneration);
return caCertGenerationAnno != null && Integer.parseInt(caCertGenerationAnno) != currentCaCertGeneration;
}
return false;
}
Expand Down
8 changes: 4 additions & 4 deletions operator-common/src/main/java/io/strimzi/operator/cluster/model/ClientsCa.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ public void initBrokerSecret(Secret secret) {
}

@Override
protected String caCertThumbprintAnnotation() {
return ANNO_STRIMZI_IO_CLIENTS_CA_THUMBPRINT;
protected String caCertGenerationAnnotation() {
return ANNO_STRIMZI_IO_CLIENTS_CA_CERT_GENERATION;
}

@Override
protected boolean isCaCertThumbprintChanged() {
return isCaCertThumbprintChanged(brokersSecret);
protected boolean isCaCertGenerationChanged() {
return isCaCertGenerationChanged(brokersSecret);
}

@Override
Expand Down

0 comments on commit 3448225

Please sign in to comment.