Fix file permissions for key material (#17026)

* Fix file permissions for key material #17009 * added changelog * correct file permissions for private key used during preflight CSR * better http and transport keystore files generation * removed changelog --------- Co-authored-by: Jan Heise <[email protected]>
Graylog2 · Oct 23, 2023 · 066794f · 066794f
1 parent efc541d
commit 066794f
Show file tree

Hide file tree

Showing 6 changed files with 126 additions and 21 deletions.
diff --git a/...c/main/java/org/graylog/datanode/bootstrap/preflight/DataNodeConfigurationPeriodical.java b/...c/main/java/org/graylog/datanode/bootstrap/preflight/DataNodeConfigurationPeriodical.java
@@ -45,6 +45,7 @@
 import javax.inject.Named;
 import javax.inject.Singleton;
 import java.io.IOException;
+import java.nio.file.Path;
 import java.net.InetAddress;
 import java.security.KeyStore;
 import java.util.Collections;
@@ -78,7 +79,7 @@ public DataNodeConfigurationPeriodical(final DataNodeProvisioningService dataNod
                                            final CertificateAndPrivateKeyMerger certificateAndPrivateKeyMerger,
                                            final SmartKeystoreStorage keystoreStorage,
                                            final @Named("password_secret") String passwordSecret,
-                                           final DatanodeConfiguration datanodeConfiguration) {
+                                           final DatanodeConfiguration datanodeConfiguration) throws IOException {
         this.dataNodeProvisioningService = dataNodeProvisioningService;
         this.nodeService = nodeService;
         this.nodeId = nodeId;
@@ -88,7 +89,7 @@ public DataNodeConfigurationPeriodical(final DataNodeProvisioningService dataNod
         this.certificateAndPrivateKeyMerger = certificateAndPrivateKeyMerger;
         this.keystoreStorage = keystoreStorage;
         // TODO: merge with real storage
-        this.privateKeyEncryptedStorage = new PrivateKeyEncryptedFileStorage(datanodeConfiguration.datanodeDirectories().getConfigurationTargetDir().resolve("privateKey.cert"));
+        this.privateKeyEncryptedStorage = new PrivateKeyEncryptedFileStorage(datanodeConfiguration.datanodeDirectories().createConfigurationFile(Path.of("privateKey.cert")));
         this.passwordSecret = passwordSecret.toCharArray();
     }
 

diff --git a/data-node/src/main/java/org/graylog/datanode/bootstrap/preflight/OpensearchConfigSync.java b/data-node/src/main/java/org/graylog/datanode/bootstrap/preflight/OpensearchConfigSync.java
@@ -31,7 +31,11 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
 import java.util.Collections;
+import java.util.Set;
 
 public class OpensearchConfigSync implements PreflightCheck {
 
@@ -46,11 +50,10 @@ public OpensearchConfigSync(DatanodeConfiguration datanodeConfiguration) {
 
     @Override
     public void runCheck() throws PreflightCheckException {
-        final Path opensearchProcessConfigurationDir = configuration.datanodeDirectories().getOpensearchProcessConfigurationDir();
-        LOG.info("Directory used for Opensearch process configuration is {}", opensearchProcessConfigurationDir.toAbsolutePath());
-
         try {
-            Files.createDirectories(opensearchProcessConfigurationDir);
+
+            final Path opensearchProcessConfigurationDir = configuration.datanodeDirectories().createOpensearchProcessConfigurationDir();
+            LOG.info("Directory used for Opensearch process configuration is {}", opensearchProcessConfigurationDir.toAbsolutePath());
 
             // this is a directory in main/resources that holds all the initial configuration files needed by the opensearch
             // we manage this directory in git. Generally we assume that this is a read-only location and we need to copy
@@ -61,7 +64,6 @@ public void runCheck() throws PreflightCheckException {
             synchronizeConfig(sourceOfInitialConfiguration, opensearchProcessConfigurationDir);
         } catch (IOException | URISyntaxException e) {
             throw new RuntimeException("Failed to prepare opensearch config directory", e);
-
         }
     }
 

diff --git a/data-node/src/main/java/org/graylog/datanode/configuration/DatanodeDirectories.java b/data-node/src/main/java/org/graylog/datanode/configuration/DatanodeDirectories.java
@@ -18,12 +18,19 @@
 
 import org.graylog.datanode.Configuration;
 import org.graylog2.plugin.system.NodeId;
+import org.jetbrains.annotations.NotNull;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.annotation.Nullable;
+import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
 import java.util.Optional;
+import java.util.Set;
 
 /**
  * This is a collection of pointers to directories used to store data, logs and configuration of the managed opensearch.
@@ -40,8 +47,8 @@ public class DatanodeDirectories {
     private final Path configurationSourceDir;
     private final Path configurationTargetDir;
 
-    public DatanodeDirectories(String nodeName, Path dataTargetDir, Path logsTargetDir, @Nullable Path configurationSourceDir, Path configurationTargetDir) {
-        this.nodeId = nodeName;
+    public DatanodeDirectories(String nodeId, Path dataTargetDir, Path logsTargetDir, @Nullable Path configurationSourceDir, Path configurationTargetDir) {
+        this.nodeId = nodeId;
         this.dataTargetDir = dataTargetDir;
         this.logsTargetDir = logsTargetDir;
         this.configurationSourceDir = configurationSourceDir;
@@ -99,14 +106,26 @@ public Optional<Path> resolveConfigurationSourceFile(String filename) {
     /**
      * This directory is used by us to store all runtime-generated configuration of datanode. This
      * could be truststores, private keys, certificates and other generated config files.
-     *
      * We also synchronize and generate opensearch configuration into a subdir of this dir, see {@link #getOpensearchProcessConfigurationDir()}
      * Read-write permissions required.
      */
     public Path getConfigurationTargetDir() {
         return resolveNodeSubdir(configurationTargetDir);
     }
 
+    public Path createConfigurationFile(Path relativePath) throws IOException {
+        final Path resolvedPath = getConfigurationTargetDir().resolve(relativePath);
+        return createRestrictedAccessFile(resolvedPath);
+    }
+
+    @NotNull
+    private static Path createRestrictedAccessFile(Path resolvedPath) throws IOException {
+        Files.deleteIfExists(resolvedPath);
+        final Set<PosixFilePermission> permissions = Set.of(PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_READ);
+        final FileAttribute<Set<PosixFilePermission>> fileAttributes = PosixFilePermissions.asFileAttribute(permissions);
+        return Files.createFile(resolvedPath, fileAttributes);
+    }
+
     /**
      * This is a subdirectory of {@link #getConfigurationTargetDir()}. It's used by us to synchronize and generate opensearch
      * configuration. Opensearch is then instructed to accept this dir as its base configuration dir (OPENSEARCH_PATH_CONF env property).
@@ -116,6 +135,21 @@ public Path getOpensearchProcessConfigurationDir() {
         return resolveNodeSubdir(configurationTargetDir).resolve("opensearch");
     }
 
+    public Path createOpensearchProcessConfigurationDir() throws IOException {
+        final Path dir = getOpensearchProcessConfigurationDir();
+        // TODO: should we always delete existing process configuration dir and recreate if here? IMHO yes
+        final Set<PosixFilePermission> permissions = Set.of(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_READ);
+        final FileAttribute<Set<PosixFilePermission>> fileAttributes = PosixFilePermissions.asFileAttribute(permissions);
+        Files.createDirectories(dir, fileAttributes);
+        return dir;
+    }
+
+    public Path createOpensearchProcessConfigurationFile(Path relativePath) throws IOException {
+        final Path resolvedPath = getOpensearchProcessConfigurationDir().resolve(relativePath);
+        return createRestrictedAccessFile(resolvedPath);
+    }
+
+
     private Path resolveNodeSubdir(Path path) {
         return path.resolve(nodeId).toAbsolutePath();
     }

diff --git a/...ain/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java b/...ain/java/org/graylog/datanode/configuration/variants/OpensearchSecurityConfiguration.java
@@ -51,7 +51,7 @@ public class OpensearchSecurityConfiguration {
 
     private static final String KEYSTORE_FORMAT = "PKCS12";
     private static final String TRUSTSTORE_FORMAT = "PKCS12";
-    private static final String TRUSTSTORE_FILENAME = "datanode-truststore.p12";
+    private static final Path TRUSTSTORE_FILE = Path.of("datanode-truststore.p12");
 
     private final KeystoreInformation transportCertificate;
     private final KeystoreInformation httpCertificate;
@@ -81,7 +81,7 @@ public OpensearchSecurityConfiguration configure(DatanodeConfiguration datanodeC
 
             final Path opensearchConfigDir = datanodeConfiguration.datanodeDirectories().getOpensearchProcessConfigurationDir();
 
-            final Path trustStorePath = opensearchConfigDir.resolve(TRUSTSTORE_FILENAME);
+            final Path trustStorePath = datanodeConfiguration.datanodeDirectories().createOpensearchProcessConfigurationFile(TRUSTSTORE_FILE);
             final String truststorePassword = RandomStringUtils.randomAlphabetic(256);
 
             this.truststore = TruststoreCreator.newTruststore()
@@ -108,7 +108,7 @@ public Map<String, String> getProperties() throws GeneralSecurityException, IOEx
             config.put("plugins.security.ssl.transport.keystore_alias", CertConstants.DATANODE_KEY_ALIAS);
 
             config.put("plugins.security.ssl.transport.truststore_type", TRUSTSTORE_FORMAT);
-            config.put("plugins.security.ssl.transport.truststore_filepath", TRUSTSTORE_FILENAME);
+            config.put("plugins.security.ssl.transport.truststore_filepath", TRUSTSTORE_FILE.toString());
             config.put("plugins.security.ssl.transport.truststore_password", truststore.passwordAsString());
 
             config.put("plugins.security.ssl.http.enabled", "true");
@@ -119,7 +119,7 @@ public Map<String, String> getProperties() throws GeneralSecurityException, IOEx
             config.put("plugins.security.ssl.http.keystore_alias", CertConstants.DATANODE_KEY_ALIAS);
 
             config.put("plugins.security.ssl.http.truststore_type", TRUSTSTORE_FORMAT);
-            config.put("plugins.security.ssl.http.truststore_filepath", TRUSTSTORE_FILENAME);
+            config.put("plugins.security.ssl.http.truststore_filepath", TRUSTSTORE_FILE.toString());
             config.put("plugins.security.ssl.http.truststore_password", truststore.passwordAsString());
         } else {
             config.put("plugins.security.disabled", "true");

diff --git a/data-node/src/main/java/org/graylog/datanode/configuration/variants/SecureConfiguration.java b/data-node/src/main/java/org/graylog/datanode/configuration/variants/SecureConfiguration.java
@@ -16,10 +16,13 @@
  */
 package org.graylog.datanode.configuration.variants;
 
+import com.google.common.base.Suppliers;
 import org.graylog.datanode.configuration.DatanodeConfiguration;
 import org.graylog.security.certutil.keystore.storage.location.KeystoreFileLocation;
 
+import java.io.IOException;
 import java.nio.file.Path;
+import java.util.function.Supplier;
 
 public sealed abstract class SecureConfiguration implements SecurityConfigurationVariant permits MongoCertSecureConfiguration, UploadedCertFilesSecureConfiguration {
 
@@ -29,27 +32,44 @@ public sealed abstract class SecureConfiguration implements SecurityConfiguratio
      * The target configuration is regenerated during each startup, so it could also be a random filename
      * as long as we use the same name as a copy-target and opensearch config property.
      */
-    private static final String TARGET_DATANODE_HTTP_KEYSTORE_FILENAME = "http-keystore.p12";
+    private static final Path TARGET_DATANODE_HTTP_KEYSTORE_FILENAME = Path.of("http-keystore.p12");
     /**
      * This filename is used only internally - we copy user-provided certificates to this location and
      * we configure opensearch to read this file. It doesn't have to match naming provided by user.
      * The target configuration is regenerated during each startup, so it could also be a random filename
      * as long as we use the same name as a copy-target and opensearch config property.
      */
-    private static final String TARGET_DATANODE_TRANSPORT_KEYSTORE_FILENAME = "transport-keystore.p12";
+    private static final Path TARGET_DATANODE_TRANSPORT_KEYSTORE_FILENAME = Path.of("transport-keystore.p12");
 
-    private final Path opensearchProcessConfigurationDir;
+    private final Supplier<KeystoreFileLocation> httpKeystoreLocation;
+    private final Supplier<KeystoreFileLocation> transportKeystoreLocation;
 
-    public SecureConfiguration(final DatanodeConfiguration localConfiguration) {
-        this.opensearchProcessConfigurationDir = localConfiguration.datanodeDirectories().getOpensearchProcessConfigurationDir();
+    public SecureConfiguration(final DatanodeConfiguration datanodeConfiguration) {
+        this.httpKeystoreLocation = Suppliers.memoize(() -> {
+            try {
+                final Path filePath = datanodeConfiguration.datanodeDirectories().createOpensearchProcessConfigurationFile(TARGET_DATANODE_HTTP_KEYSTORE_FILENAME);
+                return new KeystoreFileLocation(filePath);
+            } catch (IOException e) {
+                throw new RuntimeException("Failed to create http keystore file", e);
+            }
+        });
+
+        this.transportKeystoreLocation = Suppliers.memoize(() -> {
+            try {
+                final Path filePath = datanodeConfiguration.datanodeDirectories().createOpensearchProcessConfigurationFile(TARGET_DATANODE_TRANSPORT_KEYSTORE_FILENAME);
+                return new KeystoreFileLocation(filePath);
+            } catch (IOException e) {
+                throw new RuntimeException("Failed to create transport keystore file", e);
+            }
+        });
     }
 
     KeystoreFileLocation getHttpKeystoreLocation() {
-        return new KeystoreFileLocation(opensearchProcessConfigurationDir.resolve(TARGET_DATANODE_HTTP_KEYSTORE_FILENAME));
+        return httpKeystoreLocation.get();
     }
 
 
     KeystoreFileLocation getTransportKeystoreLocation() {
-        return new KeystoreFileLocation(opensearchProcessConfigurationDir.resolve(TARGET_DATANODE_TRANSPORT_KEYSTORE_FILENAME));
+        return transportKeystoreLocation.get();
     }
 }
diff --git a/data-node/src/test/java/org/graylog/datanode/configuration/DatanodeDirectoriesTest.java b/data-node/src/test/java/org/graylog/datanode/configuration/DatanodeDirectoriesTest.java
@@ -0,0 +1,48 @@
+/*
+ * 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.graylog.datanode.configuration;
+
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+
+class DatanodeDirectoriesTest {
+
+    @Test
+    void testConfigDirPermissions(@TempDir Path dataDir, @TempDir Path logsDir, @TempDir Path configSourceDir, @TempDir Path configTargetDir) throws IOException {
+        final DatanodeDirectories datanodeDirectories = new DatanodeDirectories("12345", dataDir, logsDir, configSourceDir, configTargetDir);
+        final Path dir = datanodeDirectories.createOpensearchProcessConfigurationDir();
+        Assertions.assertThat(Files.getPosixFilePermissions(dir)).
+                contains(
+                        PosixFilePermission.OWNER_EXECUTE,
+                        PosixFilePermission.OWNER_WRITE,
+                        PosixFilePermission.OWNER_READ
+                );
+
+        final Path keyFile = datanodeDirectories.createOpensearchProcessConfigurationFile(Path.of("my-secret-file.key"));
+        Assertions.assertThat(Files.getPosixFilePermissions(keyFile)).
+                contains(
+                        PosixFilePermission.OWNER_WRITE,
+                        PosixFilePermission.OWNER_READ
+                );
+    }
+}