Better handling of intermediate CAs in datanode truststore

changelog/unreleased/pr-21062.toml

@@ -0,0 +1,4 @@
+type = "c"
+message = "Better handling of intermediate CAs in datanode truststore"
+
+pulls = ["21062"]

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,29 +61,58 @@ 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<X509Certificate> x509Certs = toX509Certs(chain);
+        return addCertificates(x509Certs);
+    }
+
+    @Nonnull
+    private static List<X509Certificate> toX509Certs(Certificate[] certs) {
+        return Arrays.stream(certs)
+                .filter(c -> c instanceof X509Certificate)
+                .map(c -> (X509Certificate) c)
+                .toList();
     }
 
     public TruststoreCreator addCertificates(List<X509Certificate> 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);
             }
         });
         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())) {

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,21 +113,21 @@ 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);
             }
         });
 
         transportCert.ifPresent(cert -> {
             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);
             }
         });
 

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,24 +29,35 @@
 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;
 import java.security.GeneralSecurityException;
 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);
     }
 }

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<String> 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<String> 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) {

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);

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;
     }