From 73fc652c34cc58e8a5940449e5c169d95c4c1701 Mon Sep 17 00:00:00 2001 From: Tomas Dvorak Date: Wed, 27 Nov 2024 08:45:12 +0100 Subject: [PATCH 1/3] TruststoreCreator adds the whole certchain, code cleanup --- .../DatanodeTrustManagerProvider.java | 6 +- .../configuration/TruststoreCreator.java | 60 ++++++-------- .../OpensearchSecurityConfiguration.java | 5 +- .../opensearch/OpensearchProcessImpl.java | 6 +- .../configuration/TruststoreCreatorTest.java | 82 +++++++++++++++---- .../DatanodeSecurityTestUtils.java | 30 ++----- .../csr/FilesystemKeystoreInformation.java | 6 +- .../csr/InMemoryKeystoreInformation.java | 2 +- .../certutil/csr/KeystoreInformation.java | 4 +- 9 files changed, 110 insertions(+), 91 deletions(-) diff --git a/data-node/src/main/java/org/graylog/datanode/configuration/DatanodeTrustManagerProvider.java b/data-node/src/main/java/org/graylog/datanode/configuration/DatanodeTrustManagerProvider.java index 299150d8e094..68b385a72a7f 100644 --- a/data-node/src/main/java/org/graylog/datanode/configuration/DatanodeTrustManagerProvider.java +++ b/data-node/src/main/java/org/graylog/datanode/configuration/DatanodeTrustManagerProvider.java @@ -28,10 +28,8 @@ import javax.net.ssl.X509TrustManager; import java.io.IOException; +import java.security.GeneralSecurityException; import java.security.KeyStore; -import java.security.KeyStoreException; -import java.security.NoSuchAlgorithmException; -import java.security.cert.CertificateException; import java.util.List; import java.util.Optional; @@ -54,7 +52,7 @@ public void onOpensearchConfigurationChange(OpensearchConfigurationChangeEvent e .map(t -> { try { return t.loadKeystore(); - } catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException ex) { + } catch (IOException | GeneralSecurityException ex) { throw new RuntimeException(ex); } }) diff --git a/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java b/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java index cea4e2415dd1..f42c3862d1ef 100644 --- a/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java +++ b/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java @@ -19,10 +19,8 @@ import jakarta.annotation.Nonnull; import org.graylog.security.certutil.CertConstants; import org.graylog.security.certutil.csr.FilesystemKeystoreInformation; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.graylog.security.certutil.csr.KeystoreInformation; -import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.nio.file.Path; @@ -35,6 +33,7 @@ import java.security.cert.X509Certificate; import java.util.Arrays; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; public class TruststoreCreator { @@ -60,10 +59,29 @@ public static TruststoreCreator newEmpty() { } } - public TruststoreCreator addRootCert(final String name, FilesystemKeystoreInformation keystoreInformation, - final String alias) throws IOException, GeneralSecurityException { - final X509Certificate rootCert = findRootCert(keystoreInformation.location(), keystoreInformation.password(), alias); - this.truststore.setCertificateEntry(name, rootCert); + /** + * Originally we added only the root(=selfsigned) certificate to the truststore. But this causes problems with + * usage of intermediate CAs. There is nothing wrong adding the whole cert chain to the truststore. + * + * @param newAliasPrefix new alias prefix used for the truststore. We'll append _i, where i is the index of the cert in the chain + * @param keystoreInformation access to the keystore, to obtain certificate chains by the given alias + * @param alias which certificate chain should we extract from the provided keystore + */ + public TruststoreCreator addFromKeystore(final String newAliasPrefix, KeystoreInformation keystoreInformation, + final String alias) throws IOException, GeneralSecurityException { + final KeyStore keystore = keystoreInformation.loadKeystore(); + final Certificate[] certs = keystore.getCertificateChain(alias); + + AtomicInteger certCounter = new AtomicInteger(0); + Arrays.stream(certs) + .forEach(cert -> { + try { + this.truststore.setCertificateEntry(newAliasPrefix + "_" + certCounter.getAndIncrement(), cert); + } catch (KeyStoreException e) { + throw new RuntimeException(e); + } + }); + return this; } @@ -90,32 +108,4 @@ public FilesystemKeystoreInformation persist(final Path truststorePath, final ch public KeyStore getTruststore() { return this.truststore; } - - - private static X509Certificate findRootCert(Path keystorePath, - char[] password, - final String alias) throws IOException, GeneralSecurityException { - final KeyStore keystore = loadKeystore(keystorePath, password); - final Certificate[] certs = keystore.getCertificateChain(alias); - - return Arrays.stream(certs) - .filter(cert -> cert instanceof X509Certificate) - .map(cert -> (X509Certificate) cert) - .filter(cert -> isRootCaCertificate(cert) || certs.length == 1) - .findFirst() - .orElseThrow(() -> new KeyStoreException("Keystore does not contain root X509Certificate in the certificate chain!")); - } - - private static boolean isRootCaCertificate(X509Certificate cert) { - return cert.getSubjectX500Principal().equals(cert.getIssuerX500Principal()); - } - - private static KeyStore loadKeystore(final Path keystorePath, - final char[] password) throws IOException, GeneralSecurityException { - KeyStore nodeKeystore = KeyStore.getInstance(CertConstants.PKCS12); - try (final FileInputStream is = new FileInputStream(keystorePath.toFile())) { - nodeKeystore.load(is, password); - } - return nodeKeystore; - } } diff --git a/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java b/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java index 67220ab537db..e63572c2b6d0 100644 --- a/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java +++ b/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java @@ -40,7 +40,6 @@ import java.security.cert.Certificate; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; -import java.util.Base64; import java.util.Enumeration; import java.util.List; import java.util.Map; @@ -89,8 +88,8 @@ public OpensearchSecurityConfiguration configure(DatanodeConfiguration datanodeC final String truststorePassword = RandomStringUtils.randomAlphabetic(256); this.truststore = TruststoreCreator.newDefaultJvm() - .addRootCert("datanode-transport-chain-CA-root", transportCertificate, CertConstants.DATANODE_KEY_ALIAS) - .addRootCert("datanode-http-chain-CA-root", httpCertificate, CertConstants.DATANODE_KEY_ALIAS) + .addFromKeystore("datanode-transport-chain-CA-root", transportCertificate, CertConstants.DATANODE_KEY_ALIAS) + .addFromKeystore("datanode-http-chain-CA-root", httpCertificate, CertConstants.DATANODE_KEY_ALIAS) .addCertificates(trustedCertificates) .persist(trustStorePath, truststorePassword.toCharArray()); diff --git a/data-node/src/main/java/org/graylog/datanode/opensearch/OpensearchProcessImpl.java b/data-node/src/main/java/org/graylog/datanode/opensearch/OpensearchProcessImpl.java index 9704815533c0..958afc04ef1a 100644 --- a/data-node/src/main/java/org/graylog/datanode/opensearch/OpensearchProcessImpl.java +++ b/data-node/src/main/java/org/graylog/datanode/opensearch/OpensearchProcessImpl.java @@ -69,9 +69,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; -import java.security.KeyStoreException; -import java.security.NoSuchAlgorithmException; -import java.security.cert.CertificateException; +import java.security.GeneralSecurityException; import java.util.List; import java.util.Locale; import java.util.Objects; @@ -151,7 +149,7 @@ private RestHighLevelClient createRestClient(OpensearchConfiguration configurati private X509TrustManager createAggregatedTrustManager(FilesystemKeystoreInformation truststore) { try { return new TrustManagerAggregator(List.of(this.trustManager, TrustManagerAggregator.trustManagerFromKeystore(truststore.loadKeystore()))); - } catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException e) { + } catch (IOException | GeneralSecurityException e) { throw new RuntimeException(e); } } diff --git a/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java b/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java index 4520a7a0602e..8cb41ee9dff8 100644 --- a/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java +++ b/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; +import jakarta.annotation.Nonnull; import org.apache.commons.lang3.RandomStringUtils; import org.assertj.core.api.Assertions; import org.bouncycastle.asn1.x500.X500Name; @@ -28,12 +29,19 @@ import org.bouncycastle.operator.OperatorCreationException; import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder; import org.graylog.security.certutil.CertConstants; +import org.graylog.security.certutil.CertRequest; +import org.graylog.security.certutil.CertificateGenerator; +import org.graylog.security.certutil.KeyPair; import org.graylog.security.certutil.csr.FilesystemKeystoreInformation; +import org.graylog.security.certutil.csr.InMemoryKeystoreInformation; +import org.graylog.security.certutil.csr.KeystoreInformation; import org.graylog.security.certutil.keystore.storage.KeystoreFileStorage; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import java.io.FileOutputStream; +import javax.net.ssl.TrustManager; +import javax.net.ssl.TrustManagerFactory; +import javax.net.ssl.X509TrustManager; import java.io.IOException; import java.math.BigInteger; import java.nio.file.Path; @@ -41,8 +49,11 @@ import java.security.KeyPairGenerator; import java.security.KeyStore; import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; import java.security.cert.Certificate; +import java.security.cert.CertificateException; import java.security.cert.X509Certificate; +import java.time.Duration; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.ArrayList; @@ -61,12 +72,12 @@ public class TruststoreCreatorTest { @Test void testTrustStoreCreation(@TempDir Path tempDir) throws Exception { - final FilesystemKeystoreInformation root = createKeystore(tempDir.resolve("root.p12"), "root", "CN=ROOT", BigInteger.ONE); - final FilesystemKeystoreInformation boot = createKeystore(tempDir.resolve("boot.p12"), "boot", "CN=BOOT", BigInteger.TWO); + final KeystoreInformation root = createKeystore(tempDir.resolve("root.p12"), "root", "CN=ROOT", BigInteger.ONE); + final KeystoreInformation boot = createKeystore(tempDir.resolve("boot.p12"), "boot", "CN=BOOT", BigInteger.TWO); final FilesystemKeystoreInformation truststore = TruststoreCreator.newEmpty() - .addRootCert("root", root, "root") - .addRootCert("boot", boot, "boot") + .addFromKeystore("root", root, "root") + .addFromKeystore("boot", boot, "boot") .persist(tempDir.resolve("truststore.sec"), "caramba! caramba!".toCharArray()); @@ -79,11 +90,11 @@ void testTrustStoreCreation(@TempDir Path tempDir) throws Exception { final KeyStore keyStore = keyStoreOptional.get(); assertThat(ImmutableList.copyOf(keyStore.aliases().asIterator())) - .containsOnly("root", "boot"); + .containsOnly("root_0", "boot_0"); - final Certificate rootCert = keyStore.getCertificate("root"); + final Certificate rootCert = keyStore.getCertificate("root_0"); verifyCertificate(rootCert, "CN=ROOT", BigInteger.ONE); - final Certificate bootCert = keyStore.getCertificate("boot"); + final Certificate bootCert = keyStore.getCertificate("boot_0"); verifyCertificate(bootCert, "CN=BOOT", BigInteger.TWO); } @@ -98,7 +109,7 @@ void testDefaultJvm() throws KeyStoreException { @Test void testAdditionalCertificates(@TempDir Path tempDir) throws GeneralSecurityException, IOException, OperatorCreationException { - final FilesystemKeystoreInformation root = createKeystore(tempDir.resolve("root.p12"), "something-unknown", "CN=ROOT", BigInteger.ONE); + final KeystoreInformation root = createKeystore(tempDir.resolve("root.p12"), "something-unknown", "CN=ROOT", BigInteger.ONE); final X509Certificate cert = (X509Certificate) root.loadKeystore().getCertificate("something-unknown"); final FilesystemKeystoreInformation truststore = TruststoreCreator.newEmpty() @@ -111,7 +122,45 @@ void testAdditionalCertificates(@TempDir Path tempDir) throws GeneralSecurityExc Assertions.assertThat(alias) .isNotNull() .isEqualTo("cn=root"); + } + + @Test + void testIntermediateCa() throws Exception { + final KeyPair ca = CertificateGenerator.generate(CertRequest.selfSigned("my-ca").isCA(true).validity(Duration.ofDays(100))); + final KeyPair intermediateCa = CertificateGenerator.generate(CertRequest.signed("intermediate", ca).isCA(true).validity(Duration.ofDays(100))); + final KeyPair nodeKeys = CertificateGenerator.generate(CertRequest.signed("my-node", intermediateCa).isCA(false).validity(Duration.ofDays(100))); + + + final InMemoryKeystoreInformation keystoreInformation = createInMemoryKeystore(nodeKeys, intermediateCa); + + final KeyStore truststore = TruststoreCreator.newEmpty() + .addFromKeystore("my-node", keystoreInformation, "my-node") + .getTruststore(); + + final X509TrustManager defaultTrustManager = createTrustManager(truststore); + Assertions.assertThatNoException().isThrownBy(() -> defaultTrustManager.checkServerTrusted(new X509Certificate[]{nodeKeys.certificate()}, "RSA")); + + final KeyPair fakeNodeKeys = CertificateGenerator.generate(CertRequest.selfSigned("my-fake-node").isCA(false).validity(Duration.ofDays(100))); + Assertions.assertThatThrownBy(() -> defaultTrustManager.checkServerTrusted(new X509Certificate[]{fakeNodeKeys.certificate()}, "RSA")) + .isInstanceOf(CertificateException.class); + } + + private static X509TrustManager createTrustManager(KeyStore caTruststore) throws NoSuchAlgorithmException, KeyStoreException { + final TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + tmf.init(caTruststore); + final TrustManager[] trustManagers = tmf.getTrustManagers(); + return (X509TrustManager) trustManagers[0]; + } + + @SuppressWarnings("deprecation") + @Nonnull + private static InMemoryKeystoreInformation createInMemoryKeystore(KeyPair nodeKeys, KeyPair intermediate) throws KeyStoreException, IOException, NoSuchAlgorithmException, CertificateException { + final char[] password = RandomStringUtils.randomAlphabetic(256).toCharArray(); + KeyStore keystore = KeyStore.getInstance(CertConstants.PKCS12); + keystore.load(null, null); + keystore.setKeyEntry("my-node", nodeKeys.privateKey(), password, new Certificate[]{nodeKeys.certificate(), intermediate.certificate()}); + return new InMemoryKeystoreInformation(keystore, password); } private void verifyCertificate(final Certificate rootCert, final String cnName, final BigInteger serialNumber) { @@ -124,7 +173,8 @@ private void verifyCertificate(final Certificate rootCert, final String cnName, assertEquals(cnName, x509Certificate.getIssuerX500Principal().getName()); } - private FilesystemKeystoreInformation createKeystore(Path path, String alias, final String cnName, final BigInteger serialNumber) throws GeneralSecurityException, OperatorCreationException, IOException { + @SuppressWarnings("deprecation") + private KeystoreInformation createKeystore(Path path, String alias, final String cnName, final BigInteger serialNumber) throws GeneralSecurityException, OperatorCreationException, IOException { KeyPairGenerator keyGen = KeyPairGenerator.getInstance(KEY_GENERATION_ALGORITHM); java.security.KeyPair certKeyPair = keyGen.generateKeyPair(); X500Name name = new X500Name(cnName); @@ -142,17 +192,13 @@ private FilesystemKeystoreInformation createKeystore(Path path, String alias, fi final X509Certificate signedCert = new JcaX509CertificateConverter().getCertificate(certHolder); - KeyStore trustStore = KeyStore.getInstance(CertConstants.PKCS12); - trustStore.load(null, null); + KeyStore keyStore = KeyStore.getInstance(CertConstants.PKCS12); + keyStore.load(null, null); final char[] password = RandomStringUtils.randomAlphabetic(256).toCharArray(); - trustStore.setKeyEntry(alias, certKeyPair.getPrivate(), password, new Certificate[]{signedCert}); - + keyStore.setKeyEntry(alias, certKeyPair.getPrivate(), password, new Certificate[]{signedCert}); - try (final FileOutputStream fileOutputStream = new FileOutputStream(path.toFile())) { - trustStore.store(fileOutputStream, password); - } - return new FilesystemKeystoreInformation(path, password); + return new InMemoryKeystoreInformation(keyStore, password); } } diff --git a/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java b/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java index 8bd3bf09ce6f..da0ceb702bbc 100644 --- a/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java +++ b/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java @@ -17,41 +17,29 @@ package org.graylog.datanode.integration; import org.apache.commons.lang3.RandomStringUtils; +import org.graylog.datanode.configuration.TruststoreCreator; import org.graylog.security.certutil.CertutilCa; import org.graylog.security.certutil.CertutilCert; import org.graylog.security.certutil.CertutilHttp; import org.graylog.security.certutil.console.TestableConsole; import org.graylog.security.certutil.csr.FilesystemKeystoreInformation; +import org.graylog.security.certutil.csr.KeystoreInformation; -import java.io.FileInputStream; import java.io.IOException; import java.nio.file.Path; import java.security.GeneralSecurityException; import java.security.KeyStore; -import java.security.cert.Certificate; -import java.security.cert.X509Certificate; import java.util.Enumeration; public class DatanodeSecurityTestUtils { - public static KeyStore buildTruststore(FilesystemKeystoreInformation ca) throws IOException, GeneralSecurityException { - try (FileInputStream fis = new FileInputStream(ca.location().toFile())) { - - KeyStore caKeystore = KeyStore.getInstance("PKCS12"); - caKeystore.load(fis, ca.password()); - - KeyStore trustStore = KeyStore.getInstance("PKCS12"); - trustStore.load(null, null); - - final Enumeration aliases = caKeystore.aliases(); - while (aliases.hasMoreElements()) { - final String alias = aliases.nextElement(); - final Certificate cert = caKeystore.getCertificate(alias); - if (cert instanceof final X509Certificate x509Certificate) { - trustStore.setCertificateEntry(alias, x509Certificate); - } - } - return trustStore; + public static KeyStore buildTruststore(KeystoreInformation ca) throws IOException, GeneralSecurityException { + final TruststoreCreator truststoreCreator = TruststoreCreator.newEmpty(); + final Enumeration aliases = ca.loadKeystore().aliases(); + while (aliases.hasMoreElements()) { + final String alias = aliases.nextElement(); + truststoreCreator.addFromKeystore(alias, ca, alias); } + return truststoreCreator.getTruststore(); } public static FilesystemKeystoreInformation generateCa(Path dir) { diff --git a/graylog2-server/src/main/java/org/graylog/security/certutil/csr/FilesystemKeystoreInformation.java b/graylog2-server/src/main/java/org/graylog/security/certutil/csr/FilesystemKeystoreInformation.java index f46d7127bb5f..2fe48db6beff 100644 --- a/graylog2-server/src/main/java/org/graylog/security/certutil/csr/FilesystemKeystoreInformation.java +++ b/graylog2-server/src/main/java/org/graylog/security/certutil/csr/FilesystemKeystoreInformation.java @@ -19,10 +19,8 @@ import java.io.FileInputStream; import java.io.IOException; import java.nio.file.Path; +import java.security.GeneralSecurityException; import java.security.KeyStore; -import java.security.KeyStoreException; -import java.security.NoSuchAlgorithmException; -import java.security.cert.CertificateException; import java.util.Arrays; import java.util.Objects; @@ -38,7 +36,7 @@ public FilesystemKeystoreInformation(Path location, char[] password) { } @Override - public KeyStore loadKeystore() throws KeyStoreException, IOException, CertificateException, NoSuchAlgorithmException { + public KeyStore loadKeystore() throws IOException, GeneralSecurityException { KeyStore keyStore = KeyStore.getInstance(PKCS12); try (FileInputStream fis = new FileInputStream(location.toFile())) { keyStore.load(fis, password); diff --git a/graylog2-server/src/main/java/org/graylog/security/certutil/csr/InMemoryKeystoreInformation.java b/graylog2-server/src/main/java/org/graylog/security/certutil/csr/InMemoryKeystoreInformation.java index f4f216ddaf3b..dad8af8b0a9d 100644 --- a/graylog2-server/src/main/java/org/graylog/security/certutil/csr/InMemoryKeystoreInformation.java +++ b/graylog2-server/src/main/java/org/graylog/security/certutil/csr/InMemoryKeystoreInformation.java @@ -29,7 +29,7 @@ public InMemoryKeystoreInformation(KeyStore keyStore, char[] password) { } @Override - public KeyStore loadKeystore() throws Exception { + public KeyStore loadKeystore() { return keyStore; } diff --git a/graylog2-server/src/main/java/org/graylog/security/certutil/csr/KeystoreInformation.java b/graylog2-server/src/main/java/org/graylog/security/certutil/csr/KeystoreInformation.java index 746eb4ef773d..424d94309e30 100644 --- a/graylog2-server/src/main/java/org/graylog/security/certutil/csr/KeystoreInformation.java +++ b/graylog2-server/src/main/java/org/graylog/security/certutil/csr/KeystoreInformation.java @@ -16,11 +16,13 @@ */ package org.graylog.security.certutil.csr; +import java.io.IOException; +import java.security.GeneralSecurityException; import java.security.KeyStore; public interface KeystoreInformation { - KeyStore loadKeystore() throws Exception; + KeyStore loadKeystore() throws IOException, GeneralSecurityException; char[] password(); } From 2874f55ed537c8368eb15d2bc2fe0d99e6d6052d Mon Sep 17 00:00:00 2001 From: Tomas Dvorak Date: Wed, 27 Nov 2024 11:21:19 +0100 Subject: [PATCH 2/3] further truststore simplifications, unique aliases to prevent overriding already added certs --- .../configuration/TruststoreCreator.java | 46 ++++++++++++------- .../OpensearchSecurityConfiguration.java | 4 +- .../configuration/TruststoreCreatorTest.java | 32 ++++++++++--- .../DatanodeSecurityTestUtils.java | 2 +- 4 files changed, 59 insertions(+), 25 deletions(-) diff --git a/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java b/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java index f42c3862d1ef..daaad9e4cb92 100644 --- a/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java +++ b/data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java @@ -63,32 +63,29 @@ public static TruststoreCreator newEmpty() { * Originally we added only the root(=selfsigned) certificate to the truststore. But this causes problems with * usage of intermediate CAs. There is nothing wrong adding the whole cert chain to the truststore. * - * @param newAliasPrefix new alias prefix used for the truststore. We'll append _i, where i is the index of the cert in the chain * @param keystoreInformation access to the keystore, to obtain certificate chains by the given alias * @param alias which certificate chain should we extract from the provided keystore */ - public TruststoreCreator addFromKeystore(final String newAliasPrefix, KeystoreInformation keystoreInformation, + public TruststoreCreator addFromKeystore(KeystoreInformation keystoreInformation, final String alias) throws IOException, GeneralSecurityException { final KeyStore keystore = keystoreInformation.loadKeystore(); - final Certificate[] certs = keystore.getCertificateChain(alias); - - AtomicInteger certCounter = new AtomicInteger(0); - Arrays.stream(certs) - .forEach(cert -> { - try { - this.truststore.setCertificateEntry(newAliasPrefix + "_" + certCounter.getAndIncrement(), cert); - } catch (KeyStoreException e) { - throw new RuntimeException(e); - } - }); + final Certificate[] chain = keystore.getCertificateChain(alias); + final List x509Certs = toX509Certs(chain); + return addCertificates(x509Certs); + } - return this; + @Nonnull + private static List toX509Certs(Certificate[] certs) { + return Arrays.stream(certs) + .filter(c -> c instanceof X509Certificate) + .map(c -> (X509Certificate) c) + .toList(); } public TruststoreCreator addCertificates(List trustedCertificates) { trustedCertificates.forEach(cert -> { try { - this.truststore.setCertificateEntry(cert.getSubjectX500Principal().getName(), cert); + this.truststore.setCertificateEntry(generateAlias(this.truststore, cert), cert); } catch (KeyStoreException e) { throw new RuntimeException(e); } @@ -96,8 +93,25 @@ public TruststoreCreator addCertificates(List trustedCertificat return this; } - public FilesystemKeystoreInformation persist(final Path truststorePath, final char[] truststorePassword) throws IOException, GeneralSecurityException { + /** + * Alias has no meaning for the trust and validation purposes in the truststore. It's there only for managing + * the truststore content. We just need to make sure that we are using unique aliases, otherwise the + * truststore would override already present certificates. + * + * If there is no collision, we use the cname as given in the cert. In case of collisions, we'll append _i, + * where is index an incremented till it's unique in the truststore. + */ + private static String generateAlias(KeyStore truststore, X509Certificate cert) throws KeyStoreException { + AtomicInteger counter = new AtomicInteger(1); + final String cname = cert.getSubjectX500Principal().getName(); + String alias = cname; + while (truststore.containsAlias(alias)) { + alias = cname + "_" + counter.getAndIncrement(); + } + return alias; + } + public FilesystemKeystoreInformation persist(final Path truststorePath, final char[] truststorePassword) throws IOException, GeneralSecurityException { try (final FileOutputStream fileOutputStream = new FileOutputStream(truststorePath.toFile())) { this.truststore.store(fileOutputStream, truststorePassword); } diff --git a/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java b/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java index e63572c2b6d0..b9afc13dd010 100644 --- a/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java +++ b/data-node/src/main/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java @@ -88,8 +88,8 @@ public OpensearchSecurityConfiguration configure(DatanodeConfiguration datanodeC final String truststorePassword = RandomStringUtils.randomAlphabetic(256); this.truststore = TruststoreCreator.newDefaultJvm() - .addFromKeystore("datanode-transport-chain-CA-root", transportCertificate, CertConstants.DATANODE_KEY_ALIAS) - .addFromKeystore("datanode-http-chain-CA-root", httpCertificate, CertConstants.DATANODE_KEY_ALIAS) + .addFromKeystore(transportCertificate, CertConstants.DATANODE_KEY_ALIAS) + .addFromKeystore(httpCertificate, CertConstants.DATANODE_KEY_ALIAS) .addCertificates(trustedCertificates) .persist(trustStorePath, truststorePassword.toCharArray()); diff --git a/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java b/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java index 8cb41ee9dff8..715b7410b596 100644 --- a/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java +++ b/data-node/src/test/java/org/graylog/datanode/configuration/TruststoreCreatorTest.java @@ -57,6 +57,7 @@ import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.ArrayList; +import java.util.Collections; import java.util.Date; import java.util.List; import java.util.Optional; @@ -76,8 +77,8 @@ void testTrustStoreCreation(@TempDir Path tempDir) throws Exception { final KeystoreInformation boot = createKeystore(tempDir.resolve("boot.p12"), "boot", "CN=BOOT", BigInteger.TWO); final FilesystemKeystoreInformation truststore = TruststoreCreator.newEmpty() - .addFromKeystore("root", root, "root") - .addFromKeystore("boot", boot, "boot") + .addFromKeystore(root, "root") + .addFromKeystore(boot, "boot") .persist(tempDir.resolve("truststore.sec"), "caramba! caramba!".toCharArray()); @@ -90,11 +91,11 @@ void testTrustStoreCreation(@TempDir Path tempDir) throws Exception { final KeyStore keyStore = keyStoreOptional.get(); assertThat(ImmutableList.copyOf(keyStore.aliases().asIterator())) - .containsOnly("root_0", "boot_0"); + .containsOnly("cn=root", "cn=boot"); - final Certificate rootCert = keyStore.getCertificate("root_0"); + final Certificate rootCert = keyStore.getCertificate("cn=root"); verifyCertificate(rootCert, "CN=ROOT", BigInteger.ONE); - final Certificate bootCert = keyStore.getCertificate("boot_0"); + final Certificate bootCert = keyStore.getCertificate("cn=boot"); verifyCertificate(bootCert, "CN=BOOT", BigInteger.TWO); } @@ -134,7 +135,7 @@ void testIntermediateCa() throws Exception { final InMemoryKeystoreInformation keystoreInformation = createInMemoryKeystore(nodeKeys, intermediateCa); final KeyStore truststore = TruststoreCreator.newEmpty() - .addFromKeystore("my-node", keystoreInformation, "my-node") + .addFromKeystore(keystoreInformation, "my-node") .getTruststore(); final X509TrustManager defaultTrustManager = createTrustManager(truststore); @@ -146,6 +147,25 @@ void testIntermediateCa() throws Exception { .isInstanceOf(CertificateException.class); } + @Test + void testDuplicateCname() throws Exception { + final KeyPair ca1 = CertificateGenerator.generate(CertRequest.selfSigned("my-ca").isCA(true).validity(Duration.ofDays(90))); + final KeyPair ca2 = CertificateGenerator.generate(CertRequest.selfSigned("my-ca").isCA(true).validity(Duration.ofDays(90))); + final KeyPair ca3 = CertificateGenerator.generate(CertRequest.selfSigned("my-ca").isCA(true).validity(Duration.ofDays(90))); + + final KeyStore truststore = TruststoreCreator.newEmpty() + .addCertificates(List.of(ca1.certificate())) + .addCertificates(List.of(ca2.certificate())) + .addCertificates(List.of(ca3.certificate())) + .getTruststore(); + + Assertions.assertThat(Collections.list(truststore.aliases())) + .hasSize(3) + .contains("cn=my-ca") + .contains("cn=my-ca_1") + .contains("cn=my-ca_2"); + } + private static X509TrustManager createTrustManager(KeyStore caTruststore) throws NoSuchAlgorithmException, KeyStoreException { final TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); tmf.init(caTruststore); diff --git a/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java b/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java index da0ceb702bbc..b4237d5a60ad 100644 --- a/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java +++ b/data-node/src/test/java/org/graylog/datanode/integration/DatanodeSecurityTestUtils.java @@ -37,7 +37,7 @@ public static KeyStore buildTruststore(KeystoreInformation ca) throws IOExceptio final Enumeration aliases = ca.loadKeystore().aliases(); while (aliases.hasMoreElements()) { final String alias = aliases.nextElement(); - truststoreCreator.addFromKeystore(alias, ca, alias); + truststoreCreator.addFromKeystore(ca, alias); } return truststoreCreator.getTruststore(); } From 947f9f7d7278402cdf06bcdf4ca33645b6faa99d Mon Sep 17 00:00:00 2001 From: Tomas Dvorak Date: Wed, 27 Nov 2024 13:16:40 +0100 Subject: [PATCH 3/3] added changelog --- changelog/unreleased/pr-21062.toml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelog/unreleased/pr-21062.toml diff --git a/changelog/unreleased/pr-21062.toml b/changelog/unreleased/pr-21062.toml new file mode 100644 index 000000000000..0df01e48c0a3 --- /dev/null +++ b/changelog/unreleased/pr-21062.toml @@ -0,0 +1,4 @@ +type = "c" +message = "Better handling of intermediate CAs in datanode truststore" + +pulls = ["21062"]