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"] 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 57950d5dea11..5cbb5b1f07c3 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 @@ -22,6 +22,7 @@ import org.graylog.security.certutil.csr.InMemoryKeystoreInformation; import org.graylog.security.certutil.csr.KeystoreInformation; +import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.nio.file.Path; @@ -34,6 +35,7 @@ import java.security.cert.X509Certificate; import java.util.Arrays; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; public class TruststoreCreator { @@ -59,22 +61,33 @@ public static TruststoreCreator newEmpty() { } } - public TruststoreCreator addRootCert(final String name, KeystoreInformation keystoreInformation, - final String alias) throws GeneralSecurityException { - final X509Certificate rootCert; - try { - rootCert = findRootCert(keystoreInformation, alias); - } catch (Exception e) { - throw new RuntimeException(e); - } - this.truststore.setCertificateEntry(name, rootCert); - return this; + /** + * 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 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(KeystoreInformation keystoreInformation, + final String alias) throws IOException, GeneralSecurityException { + final KeyStore keystore = keystoreInformation.loadKeystore(); + final Certificate[] chain = keystore.getCertificateChain(alias); + final List x509Certs = toX509Certs(chain); + return addCertificates(x509Certs); + } + + @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); } @@ -82,6 +95,24 @@ public TruststoreCreator addCertificates(List trustedCertificat return this; } + /** + * 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())) { diff --git a/data-node/src/main/java/org/graylog/datanode/opensearch/configuration/beans/impl/OpensearchSecurityConfigurationBean.java b/data-node/src/main/java/org/graylog/datanode/opensearch/configuration/beans/impl/OpensearchSecurityConfigurationBean.java index f026d2bda49f..b48bbddd446e 100644 --- a/data-node/src/main/java/org/graylog/datanode/opensearch/configuration/beans/impl/OpensearchSecurityConfigurationBean.java +++ b/data-node/src/main/java/org/graylog/datanode/opensearch/configuration/beans/impl/OpensearchSecurityConfigurationBean.java @@ -36,6 +36,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.IOException; import java.nio.file.Path; import java.security.GeneralSecurityException; import java.security.KeyStore; @@ -112,10 +113,10 @@ public DatanodeConfigurationPart buildConfigurationPart(OpensearchConfigurationP try { configurationBuilder.httpCertificate(cert); configurationBuilder.withConfigFile(new KeystoreConfigFile(Path.of(TARGET_DATANODE_HTTP_KEYSTORE_FILENAME), cert)); - truststoreCreator.addRootCert("http-cert", cert, CertConstants.DATANODE_KEY_ALIAS); + truststoreCreator.addFromKeystore(cert, CertConstants.DATANODE_KEY_ALIAS); logCertificateInformation("HTTP certificate", cert); - } catch (GeneralSecurityException e) { - throw new RuntimeException(e); + } catch (GeneralSecurityException | IOException e) { + throw new OpensearchConfigurationException(e); } }); @@ -123,10 +124,10 @@ public DatanodeConfigurationPart buildConfigurationPart(OpensearchConfigurationP try { configurationBuilder.transportCertificate(cert); configurationBuilder.withConfigFile(new KeystoreConfigFile(Path.of(TARGET_DATANODE_TRANSPORT_KEYSTORE_FILENAME), cert)); - truststoreCreator.addRootCert("transport-cert", cert, CertConstants.DATANODE_KEY_ALIAS); + truststoreCreator.addFromKeystore(cert, CertConstants.DATANODE_KEY_ALIAS); logCertificateInformation("Transport certificate", cert); - } catch (GeneralSecurityException e) { - throw new RuntimeException(e); + } catch (GeneralSecurityException | IOException e) { + throw new OpensearchConfigurationException(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..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 @@ -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,11 +49,15 @@ 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; +import java.util.Collections; import java.util.Date; import java.util.List; import java.util.Optional; @@ -61,12 +73,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") + .addFromKeystore(boot, "boot") .persist(tempDir.resolve("truststore.sec"), "caramba! caramba!".toCharArray()); @@ -79,11 +91,11 @@ void testTrustStoreCreation(@TempDir Path tempDir) throws Exception { final KeyStore keyStore = keyStoreOptional.get(); assertThat(ImmutableList.copyOf(keyStore.aliases().asIterator())) - .containsOnly("root", "boot"); + .containsOnly("cn=root", "cn=boot"); - final Certificate rootCert = keyStore.getCertificate("root"); + final Certificate rootCert = keyStore.getCertificate("cn=root"); verifyCertificate(rootCert, "CN=ROOT", BigInteger.ONE); - final Certificate bootCert = keyStore.getCertificate("boot"); + final Certificate bootCert = keyStore.getCertificate("cn=boot"); verifyCertificate(bootCert, "CN=BOOT", BigInteger.TWO); } @@ -98,7 +110,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 +123,64 @@ 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(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); + } + + @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); + 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 +193,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 +212,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..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 @@ -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(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(); }