Backport: better handling of intermediate CAs in datanode truststore (#…

…21656)

* Better handling of intermediate CAs in datanode truststore

* added changelog

* removed dead code

* fixed test

---------

Co-authored-by: Matthias Oesterheld <33032967+moesterheld@users.noreply.github.com>

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/variants/OpensearchSecurityConfiguration.java

@@ -22,10 +22,10 @@
 import com.google.common.collect.ImmutableMap;
 import org.apache.commons.lang3.RandomStringUtils;
 import org.graylog.datanode.configuration.DatanodeConfiguration;
-import org.graylog.datanode.configuration.TruststoreCreator;
 import org.graylog.security.certutil.CertConstants;
 import org.graylog.security.certutil.csr.FilesystemKeystoreInformation;
 import org.graylog2.security.JwtSecret;
+import org.graylog2.security.TruststoreCreator;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -89,8 +89,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(transportCertificate, CertConstants.DATANODE_KEY_ALIAS)
+                    .addFromKeystore(httpCertificate, CertConstants.DATANODE_KEY_ALIAS)
                     .addCertificates(trustedCertificates)
                     .persist(trustStorePath, truststorePassword.toCharArray());
 

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

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

data-node/src/main/java/org/graylog/datanode/configuration/TruststoreCreator.java → graylog2-server/src/main/java/org/graylog2/security/TruststoreCreator.java

@@ -14,15 +14,14 @@
  * along with this program. If not, see
  * <http://www.mongodb.com/licensing/server-side-public-license>.
  */
-package org.graylog.datanode.configuration;
+package org.graylog2.security;
 
 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.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;
@@ -35,6 +34,7 @@
 import java.security.cert.X509Certificate;
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
 
 public class TruststoreCreator {
 
@@ -60,24 +60,58 @@ 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);
-        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())) {
@@ -90,32 +124,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;
-    }
 }

graylog2-server/src/main/java/org/graylog2/security/TruststoreUtils.java

@@ -0,0 +1,97 @@
+/*
+ * Copyright (C) 2020 Graylog, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the Server Side Public License, version 1,
+ * as published by MongoDB, Inc.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * Server Side Public License for more details.
+ *
+ * You should have received a copy of the Server Side Public License
+ * along with this program. If not, see
+ * <http://www.mongodb.com/licensing/server-side-public-license>.
+ */
+package org.graylog2.security;
+
+import jakarta.annotation.Nonnull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.cert.CertificateException;
+import java.util.Optional;
+import java.util.stream.Stream;
+
+public class TruststoreUtils {
+
+    private static final Logger LOG = LoggerFactory.getLogger(TruststoreUtils.class);
+
+    public static Optional<KeyStore> loadJvmTruststore() {
+        return jvmTruststoreLocation().map(location -> {
+
+            String password = jvmTruststorePassword();
+            String type = jvmTruststoreType();
+
+            LOG.info("Detected existing JVM truststore: " + location.toAbsolutePath() + " of type " + type);
+
+            try {
+                KeyStore trustStore = KeyStore.getInstance(type);
+                try (InputStream is = Files.newInputStream(location)) {
+                    trustStore.load(is, password.toCharArray());
+                }
+                return trustStore;
+            } catch (KeyStoreException | CertificateException | IOException | NoSuchAlgorithmException e) {
+                throw new RuntimeException(e);
+            }
+        });
+    }
+
+    private static String jvmTruststoreType() {
+        return Optional.ofNullable(System.getProperty("javax.net.ssl.trustStoreType"))
+                .filter(type -> !type.isEmpty())
+                .orElseGet(KeyStore::getDefaultType);
+    }
+
+    private static String jvmTruststorePassword() {
+        return Optional.ofNullable(System.getProperty("javax.net.ssl.trustStorePassword"))
+                .filter(p -> !p.isEmpty())
+                .orElse("changeit");
+    }
+
+    private static Optional<Path> jvmTruststoreLocation() {
+        String javaHome = System.getProperty("java.home");
+        return Stream.of(
+                        truststoreSystemProperty(),
+                        libSecurityFile(javaHome, "jssecacerts"),
+                        libSecurityFile(javaHome, "cacerts")
+                )
+                .filter(Optional::isPresent)
+                .map(Optional::get)
+                .filter(TruststoreUtils::isReadableFile)
+                .findFirst();
+    }
+
+    @Nonnull
+    private static Optional<Path> libSecurityFile(String javaHome, String filename) {
+        return Optional.ofNullable(javaHome).map(home -> Paths.get(home, "lib", "security", filename));
+    }
+
+    @Nonnull
+    private static Optional<Path> truststoreSystemProperty() {
+        return Optional.ofNullable(System.getProperty("javax.net.ssl.trustStore")).filter(p -> !p.isEmpty()).map(Paths::get);
+    }
+
+    private static boolean isReadableFile(Path path) {
+        return Files.exists(path) && Files.isRegularFile(path) && Files.isReadable(path);
+    }
+}