Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor cert handling #9463

Merged
merged 7 commits into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,117 @@
*/
public class SecretCertProvider {

/**
* A certificate entry in a ConfigMap. Each entry contains an entry name and data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* A certificate entry in a ConfigMap. Each entry contains an entry name and data.
* A certificate entry in a Secret. Each entry contains an entry name and data.

*/
public enum SecretEntry {
/**
* A 64-bit encoded X509 Certificate
*/
CRT(".crt"),
/**
* Entity private key
*/
KEY(".key"),
/**
* Entity certificate and key as a P12 keystore
*/
P12_KEYSTORE(".p12"),
/**
* P12 keystore password
*/
P12_KEYSTORE_PASSWORD(".password");

final String suffix;

SecretEntry(String suffix) {
this.suffix = suffix;
}

/**
* @return The suffix of the entry name in the Secret
*/
public String getSuffix() {
return suffix;
}

}

/**
* Constructs a Map containing the provided certificates to be stored in a Kubernetes Secret.
*
* @param certificates to store
* @return Map of certificate identifier to base64 encoded certificate or key
*/
public static Map<String, String> buildSecretData(Map<String, CertAndKey> certificates) {
Map<String, String> data = new HashMap<>(certificates.size() * 4);
certificates.forEach((keyCertName, certAndKey) -> {
data.put(keyCertName + SecretEntry.KEY.getSuffix(), certAndKey.keyAsBase64String());
data.put(keyCertName + SecretEntry.CRT.getSuffix(), certAndKey.certAsBase64String());
data.put(keyCertName + SecretEntry.P12_KEYSTORE.getSuffix(), certAndKey.keyStoreAsBase64String());
data.put(keyCertName + SecretEntry.P12_KEYSTORE_PASSWORD.getSuffix(), certAndKey.storePasswordAsBase64String());
});
return data;
}

private static byte[] decodeFromSecret(Secret secret, String key) {
if (secret.getData().get(key) != null && !secret.getData().get(key).isEmpty()) {
return Base64.getDecoder().decode(secret.getData().get(key));
} else {
return new byte[]{};
}
}

/**
* Extracts the KeyStore from the Secret as a CertAndKey
* @param secret to extract certificate and key from
* @param keyCertName name of the KeyStore
* @return the KeyStore as a CertAndKey. Returned object has empty truststore and
* may have empty key, cert or keystore and null store password.
*/
public static CertAndKey keyStoreCertAndKey(Secret secret, String keyCertName) {
byte[] passwordBytes = decodeFromSecret(secret, keyCertName + SecretEntry.P12_KEYSTORE_PASSWORD.getSuffix());
String password = passwordBytes.length == 0 ? null : new String(passwordBytes, StandardCharsets.US_ASCII);
return new CertAndKey(
decodeFromSecret(secret, keyCertName + SecretEntry.KEY.getSuffix()),
decodeFromSecret(secret, keyCertName + SecretEntry.CRT.getSuffix()),
new byte[]{},
decodeFromSecret(secret, keyCertName + SecretEntry.P12_KEYSTORE.getSuffix()),
password
);
}

/**
* Compares two Secrets with certificates and checks whether any value for a key which exists in both Secrets
* changed. This method is used to evaluate whether rolling update of existing brokers is needed when secrets with
* certificates change. It separates changes for existing certificates with other changes to the secret such as
* added or removed certificates (scale-up or scale-down).
*
* @param current Existing secret
* @param desired Desired secret
*
* @return True if there is a key which exists in the data sections of both secrets and which changed.
*/
public static boolean doExistingCertificatesDiffer(Secret current, Secret desired) {
Map<String, String> currentData = current.getData();
Map<String, String> desiredData = desired.getData();

if (currentData == null) {
return true;
} else {
for (Map.Entry<String, String> entry : currentData.entrySet()) {
String desiredValue = desiredData.get(entry.getKey());
if (entry.getValue() != null
&& desiredValue != null
&& !entry.getValue().equals(desiredValue)) {
return true;
}
}
}

return false;
}

/**
* Create a Kubernetes secret containing the provided private key and related certificate
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
* Copyright Strimzi authors.
* License: Apache License 2.0 (see the file LICENSE or http://apache.org/licenses/LICENSE-2.0.html).
*/
package io.strimzi.certs;

import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.kubernetes.api.model.SecretBuilder;
import org.junit.jupiter.api.Test;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

public class SecretCertProviderTest {

katheris marked this conversation as resolved.
Show resolved Hide resolved
@Test
public void testExistingCertificatesDiffer() {
Secret defaultSecret = new SecretBuilder()
.withNewMetadata()
.withName("my-secret")
.endMetadata()
.addToData("my-cluster-kafka-0.crt", "Certificate0")
.addToData("my-cluster-kafka-0.key", "Key0")
.addToData("my-cluster-kafka-1.crt", "Certificate1")
.addToData("my-cluster-kafka-1.key", "Key1")
.addToData("my-cluster-kafka-2.crt", "Certificate2")
.addToData("my-cluster-kafka-2.key", "Key2")
.build();

Secret sameAsDefaultSecret = new SecretBuilder()
.withNewMetadata()
.withName("my-secret")
.endMetadata()
.addToData("my-cluster-kafka-0.crt", "Certificate0")
.addToData("my-cluster-kafka-0.key", "Key0")
.addToData("my-cluster-kafka-1.crt", "Certificate1")
.addToData("my-cluster-kafka-1.key", "Key1")
.addToData("my-cluster-kafka-2.crt", "Certificate2")
.addToData("my-cluster-kafka-2.key", "Key2")
.build();

Secret scaleDownSecret = new SecretBuilder()
.withNewMetadata()
.withName("my-secret")
.endMetadata()
.addToData("my-cluster-kafka-0.crt", "Certificate0")
.addToData("my-cluster-kafka-0.key", "Key0")
.build();

Secret scaleUpSecret = new SecretBuilder()
.withNewMetadata()
.withName("my-secret")
.endMetadata()
.addToData("my-cluster-kafka-0.crt", "Certificate0")
.addToData("my-cluster-kafka-0.key", "Key0")
.addToData("my-cluster-kafka-1.crt", "Certificate1")
.addToData("my-cluster-kafka-1.key", "Key1")
.addToData("my-cluster-kafka-2.crt", "Certificate2")
.addToData("my-cluster-kafka-2.key", "Key2")
.addToData("my-cluster-kafka-3.crt", "Certificate3")
.addToData("my-cluster-kafka-3.key", "Key3")
.addToData("my-cluster-kafka-4.crt", "Certificate4")
.addToData("my-cluster-kafka-4.key", "Key4")
.build();

Secret changedSecret = new SecretBuilder()
.withNewMetadata()
.withName("my-secret")
.endMetadata()
.addToData("my-cluster-kafka-0.crt", "Certificate0")
.addToData("my-cluster-kafka-0.key", "Key0")
.addToData("my-cluster-kafka-1.crt", "Certificate1")
.addToData("my-cluster-kafka-1.key", "NewKey1")
.addToData("my-cluster-kafka-2.crt", "Certificate2")
.addToData("my-cluster-kafka-2.key", "Key2")
.build();

Secret changedScaleUpSecret = new SecretBuilder()
.withNewMetadata()
.withName("my-secret")
.endMetadata()
.addToData("my-cluster-kafka-0.crt", "Certificate0")
.addToData("my-cluster-kafka-0.key", "Key0")
.addToData("my-cluster-kafka-1.crt", "Certificate1")
.addToData("my-cluster-kafka-1.key", "Key1")
.addToData("my-cluster-kafka-2.crt", "NewCertificate2")
.addToData("my-cluster-kafka-2.key", "Key2")
.addToData("my-cluster-kafka-3.crt", "Certificate3")
.addToData("my-cluster-kafka-3.key", "Key3")
.addToData("my-cluster-kafka-4.crt", "Certificate4")
.addToData("my-cluster-kafka-4.key", "Key4")
.build();

Secret changedScaleDownSecret = new SecretBuilder()
.withNewMetadata()
.withName("my-secret")
.endMetadata()
.addToData("my-cluster-kafka-0.crt", "NewCertificate0")
.addToData("my-cluster-kafka-0.key", "NewKey0")
.build();

assertThat(SecretCertProvider.doExistingCertificatesDiffer(defaultSecret, defaultSecret), is(false));
assertThat(SecretCertProvider.doExistingCertificatesDiffer(defaultSecret, sameAsDefaultSecret), is(false));
assertThat(SecretCertProvider.doExistingCertificatesDiffer(defaultSecret, scaleDownSecret), is(false));
assertThat(SecretCertProvider.doExistingCertificatesDiffer(defaultSecret, scaleUpSecret), is(false));
assertThat(SecretCertProvider.doExistingCertificatesDiffer(defaultSecret, changedSecret), is(true));
assertThat(SecretCertProvider.doExistingCertificatesDiffer(defaultSecret, changedScaleUpSecret), is(true));
assertThat(SecretCertProvider.doExistingCertificatesDiffer(defaultSecret, changedScaleDownSecret), is(true));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,32 @@
*/
package io.strimzi.operator.cluster.model;

import io.fabric8.kubernetes.api.model.OwnerReference;
import io.fabric8.kubernetes.api.model.Secret;
import io.strimzi.certs.CertAndKey;
import io.strimzi.certs.SecretCertProvider;
import io.strimzi.operator.common.Reconciliation;
import io.strimzi.operator.common.ReconciliationLogger;
import io.strimzi.operator.common.Util;
import io.strimzi.operator.common.model.Ca;
import io.strimzi.operator.common.model.Labels;

import java.io.IOException;
import java.math.BigInteger;
import java.security.cert.CertificateEncodingException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import static java.util.Collections.emptyMap;

/**
* Certificate utility methods
*/
public class CertUtils {

katheris marked this conversation as resolved.
Show resolved Hide resolved
protected static final ReconciliationLogger LOGGER = ReconciliationLogger.create(CertUtils.class.getName());

/**
* Generates a short SHA1-hash (a hash stub) of the certificate which is used to track when the certificate changes and rolling update needs to be triggered.
*
Expand Down Expand Up @@ -42,4 +57,67 @@ public static String getCertificateThumbprint(Secret certSecret, String key) {
throw new RuntimeException("Failed to get certificate thumbprint of " + key + " from Secret " + certSecret.getMetadata().getName(), e);
}
}

/**
* Builds a clusterCa certificate secret for the different Strimzi components (TO, UO, KE, ...)
*
* @param reconciliation Reconciliation marker
* @param clusterCa The Cluster CA
* @param secret Kubernetes Secret
katheris marked this conversation as resolved.
Show resolved Hide resolved
* @param namespace Namespace
* @param secretName Name of the Kubernetes secret
* @param commonName Common Name of the certificate
* @param keyCertName Key under which the certificate will be stored in the new Secret
* @param labels Labels
* @param ownerReference Owner reference
* @param isMaintenanceTimeWindowsSatisfied Flag whether we are inside a maintenance window or not
*
* @return Newly built Secret
*/
public static Secret buildTrustedCertificateSecret(Reconciliation reconciliation, ClusterCa clusterCa, Secret secret, String namespace, String secretName,
String commonName, String keyCertName, Labels labels, OwnerReference ownerReference, boolean isMaintenanceTimeWindowsSatisfied) {
CertAndKey certAndKey = null;
boolean shouldBeRegenerated = false;
List<String> reasons = new ArrayList<>(2);

if (secret == null) {
reasons.add("certificate doesn't exist yet");
shouldBeRegenerated = true;
} else {
if (clusterCa.keyCreated() || clusterCa.certRenewed() ||
(isMaintenanceTimeWindowsSatisfied && clusterCa.isExpiring(secret, keyCertName + SecretCertProvider.SecretEntry.CRT.getSuffix())) ||
clusterCa.hasCaCertGenerationChanged(secret)) {
katheris marked this conversation as resolved.
Show resolved Hide resolved
reasons.add("certificate needs to be renewed");
shouldBeRegenerated = true;
}
}

if (shouldBeRegenerated) {
LOGGER.debugCr(reconciliation, "Certificate for pod {} need to be regenerated because: {}", keyCertName, String.join(", ", reasons));

try {
certAndKey = clusterCa.generateSignedCert(commonName, Ca.IO_STRIMZI);
} catch (IOException e) {
LOGGER.warnCr(reconciliation, "Error while generating certificates", e);
}

LOGGER.debugCr(reconciliation, "End generating certificates");
} else {
CertAndKey keyStoreCertAndKey = SecretCertProvider.keyStoreCertAndKey(secret, keyCertName);
if (keyStoreCertAndKey.keyStore().length != 0 &&
keyStoreCertAndKey.storePassword() != null) {
certAndKey = keyStoreCertAndKey;
} else {
try {
// coming from an older operator version, the secret exists but without keystore and password
certAndKey = clusterCa.addKeyAndCertToKeyStore(commonName,
keyStoreCertAndKey.key(),
keyStoreCertAndKey.cert());
} catch (IOException e) {
LOGGER.errorCr(reconciliation, "Error generating the keystore for {}", keyCertName, e);
}
}
}
return ModelUtils.createSecret(secretName, namespace, labels, ownerReference, SecretCertProvider.buildSecretData(Collections.singletonMap(keyCertName, certAndKey)), clusterCa.caCertGenerationFullAnnotation(), emptyMap());
katheris marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading
Loading