fixup! Logging and cleanup

jfreden · jfreden · commit 50150130b083 · 2025-09-05T12:17:11.000+02:00
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/CrossClusterApiKeySigner.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/CrossClusterApiKeySigner.java
@@ -32,10 +32,10 @@
 import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import javax.net.ssl.X509KeyManager;
 
@@ -45,6 +45,7 @@
 public class CrossClusterApiKeySigner {
     private final Logger logger = LogManager.getLogger(getClass());
     private final Environment environment;
+    private static final Map<String, String> SIGNATURE_ALGORITHM_BY_TYPE = Map.of("RSA", "SHA256withRSA", "EC", "SHA256withECDSA");
 
     private final Map<String, SigningConfig> signingConfigByClusterAlias = new ConcurrentHashMap<>();
 
@@ -69,6 +70,8 @@ public void loadSigningConfig(String clusterAlias, @Nullable Settings settings,
                 updateSecureSettings
             );
 
+            logger.trace("Loading signing config for [{}] with settings [{}]", clusterAlias, effectiveSettings);
+
             SigningConfig signingConfig = new SigningConfig(null, null, effectiveSettings);
             if (effectiveSettings.getByPrefix(SETTINGS_PART_SIGNING).isEmpty() == false) {
                 try {
@@ -80,16 +83,20 @@ public void loadSigningConfig(String clusterAlias, @Nullable Settings settings,
                     );
                     if (keyConfig.hasKeyMaterial()) {
                         String alias = effectiveSettings.get(SETTINGS_PART_SIGNING + "." + KEYSTORE_ALIAS_SUFFIX);
-                        var keyPair = Strings.isNullOrEmpty(alias)
-                            ? buildKeyPairForCluster(keyConfig)
-                            : buildKeyPairForCluster(keyConfig, alias);
+                        var keyPair = Strings.isNullOrEmpty(alias) ? buildKeyPair(keyConfig) : buildKeyPair(keyConfig, alias);
                         if (keyPair != null) {
+                            logger.trace("Key pair [{}] found for [{}]", keyPair, clusterAlias);
                             signingConfig = new SigningConfig(keyPair, keyConfig.getDependentFiles(), effectiveSettings);
                         }
+                    } else {
+                        logger.error(Strings.format("No signing credentials found in signing config for cluster [%s]", clusterAlias));
                     }
                 } catch (Exception e) {
+                    // Since this can be called by the settings applier we don't want to surface an error here
                     logger.error(Strings.format("Failed to load signing config for cluster [%s]", clusterAlias), e);
                 }
+            } else {
+                logger.trace("No signing settings found for [{}]", clusterAlias);
             }
             return signingConfig;
         });
@@ -98,6 +105,7 @@ public void loadSigningConfig(String clusterAlias, @Nullable Settings settings,
     public X509CertificateSignature sign(String clusterAlias, String... headers) {
         SigningConfig signingConfig = signingConfigByClusterAlias.get(clusterAlias);
         if (signingConfig == null || signingConfig.keyPair() == null) {
+            logger.trace("No signing config found for [{}] returning empty signature", clusterAlias);
             return null;
         }
         var keyPair = signingConfig.keyPair();
@@ -110,7 +118,10 @@ public X509CertificateSignature sign(String clusterAlias, String... headers) {
             final byte[] sigBytes = signature.sign();
             return new X509CertificateSignature(keyPair.certificate, algorithm, new BytesArray(sigBytes));
         } catch (GeneralSecurityException e) {
-            throw new ElasticsearchSecurityException("Failed to sign cross cluster headers", e);
+            throw new ElasticsearchSecurityException(
+                Strings.format("Failed to sign cross cluster headers for cluster [%s]", clusterAlias),
+                e
+            );
         }
     }
 
@@ -153,13 +164,14 @@ void reloadSigningConfigs(Set<String> clusterAliases) {
         clusterAliases.forEach(alias -> loadSigningConfig(alias, null, true));
     }
 
-    private X509KeyPair buildKeyPairForCluster(SslKeyConfig keyConfig) {
+    private X509KeyPair buildKeyPair(SslKeyConfig keyConfig) {
         final X509KeyManager keyManager = keyConfig.createKeyManager();
         if (keyManager == null) {
             return null;
         }
 
-        final Set<String> aliases = Stream.of("RSA", "EC")
+        final Set<String> aliases = SIGNATURE_ALGORITHM_BY_TYPE.keySet()
+            .stream()
             .map(keyType -> keyManager.getServerAliases(keyType, null))
             .filter(Objects::nonNull)
             .flatMap(Arrays::stream)
@@ -182,12 +194,24 @@ private X509KeyPair buildKeyPairForCluster(SslKeyConfig keyConfig) {
         };
     }
 
-    private X509KeyPair buildKeyPairForCluster(SslKeyConfig keyConfig, String alias) {
+    private X509KeyPair buildKeyPair(SslKeyConfig keyConfig, String alias) {
         final X509KeyManager keyManager = keyConfig.createKeyManager();
         if (keyManager == null) {
             return null;
         }
 
+        final String keyType = keyManager.getPrivateKey(alias).getAlgorithm();
+        if (SIGNATURE_ALGORITHM_BY_TYPE.containsKey(keyType) == false) {
+            throw new IllegalStateException(
+                Strings.format(
+                    "The key associated with alias [%s] uses unsupported key algorithm type [%s], only %s is supported",
+                    alias,
+                    keyType,
+                    SIGNATURE_ALGORITHM_BY_TYPE.keySet()
+                )
+            );
+        }
+
         final X509Certificate[] chain = keyManager.getCertificateChain(alias);
         logger.trace("KeyConfig [{}] has entry for alias: [{}] [{}]", keyConfig, alias, chain != null);
 
@@ -208,16 +232,20 @@ private static String calculateFingerprint(X509Certificate certificate) {
 
     private record X509KeyPair(X509Certificate certificate, PrivateKey privateKey, String signatureAlgorithm, String fingerprint) {
         X509KeyPair(X509Certificate certificate, PrivateKey privateKey) {
-            this(certificate, privateKey, switch (privateKey.getAlgorithm()) {
-                case "RSA" -> "SHA256withRSA";
-                case "EC" -> "SHA256withECDSA";
-                default -> throw new IllegalArgumentException(
-                    "Unsupported Key Type [" + privateKey.getAlgorithm() + "] for [" + privateKey + "]"
-                );
-            }, calculateFingerprint(certificate));
+            this(
+                certificate,
+                privateKey,
+                Optional.ofNullable(SIGNATURE_ALGORITHM_BY_TYPE.get(privateKey.getAlgorithm()))
+                    .orElseThrow(
+                        () -> new IllegalArgumentException(
+                            "Unsupported Key Type [" + privateKey.getAlgorithm() + "] for [" + privateKey + "]"
+                        )
+                    ),
+                calculateFingerprint(certificate)
+            );
         }
     }
 
-    private record SigningConfig(@Nullable X509KeyPair keyPair, Collection<Path> dependentFiles, @Nullable Settings settings) {}
+    private record SigningConfig(@Nullable X509KeyPair keyPair, @Nullable Collection<Path> dependentFiles, @Nullable Settings settings) {}
 
 }
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/CrossClusterApiKeySignerReloader.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/CrossClusterApiKeySignerReloader.java
@@ -14,6 +14,7 @@
 import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.Strings;
 import org.elasticsearch.transport.RemoteClusterSettings;
 import org.elasticsearch.watcher.FileChangesListener;
 import org.elasticsearch.watcher.FileWatcher;
@@ -26,7 +27,6 @@
 import java.nio.file.Path;
 import java.security.GeneralSecurityException;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -35,11 +35,17 @@
 import static org.elasticsearch.xpack.security.transport.CrossClusterApiKeySignerSettings.getDynamicSettings;
 import static org.elasticsearch.xpack.security.transport.CrossClusterApiKeySignerSettings.getSecureSettings;
 
+/**
+ * Responsible for reloading a provided {@link CrossClusterApiKeySigner} when updates are received from the following sources:
+ * - Dynamic cluster settings
+ * - Reloadable secure settings
+ * - File content changes in any of the files pointed to by the cluster settings
+ */
 public final class CrossClusterApiKeySignerReloader implements ReloadableSecurityComponent {
 
     private static final Logger logger = LogManager.getLogger(CrossClusterApiKeySignerReloader.class);
     private final CrossClusterApiKeySigner apiKeySigner;
-    private final Set<Path> monitoredPaths = new HashSet<>();
+    private final Map<Path, ChangeListener> monitoredPathToChangeListener = new HashMap<>();
 
     public CrossClusterApiKeySignerReloader(
         ResourceWatcherService resourceWatcherService,
@@ -48,8 +54,8 @@ public CrossClusterApiKeySignerReloader(
     ) {
         this.apiKeySigner = apiKeySigner;
         clusterSettings.addAffixGroupUpdateConsumer(getDynamicSettings(), (key, val) -> {
-            logger.info("Updating cross cluster signing configuration for key [{}] with settings [{}]", key, val);
             apiKeySigner.loadSigningConfig(key, val.getByPrefix(RemoteClusterSettings.REMOTE_CLUSTER_SETTINGS_PREFIX + key + "."), false);
+            logger.info("Updated signing configuration for [{}] due to update of cluster settings [{}]", key, val);
             watchDependentFilesForClusterAliases(
                 apiKeySigner::reloadSigningConfigs,
                 resourceWatcherService,
@@ -70,18 +76,23 @@ private void watchDependentFilesForClusterAliases(
         Map<Path, Set<String>> dependentFilesToClusterAliases
     ) {
         dependentFilesToClusterAliases.forEach((path, clusterAliases) -> {
-            if (monitoredPaths.contains(path)) {
+            var existingChangeListener = monitoredPathToChangeListener.get(path);
+            if (existingChangeListener != null) {
+                logger.trace("Found existing listener for file [{}], adding clusterAliases {}", path, clusterAliases);
+                existingChangeListener.addClusterAliases(clusterAliases);
                 return;
             }
 
+            logger.trace("Adding listener for file [{}] for clusters {}", path, clusterAliases);
+
             ChangeListener changeListener = new ChangeListener(clusterAliases, path, reloadConsumer);
             FileWatcher fileWatcher = new FileWatcher(path);
             fileWatcher.addListener(changeListener);
             try {
                 resourceWatcherService.add(fileWatcher, Frequency.HIGH);
-                monitoredPaths.add(path);
+                monitoredPathToChangeListener.put(path, changeListener);
             } catch (IOException | SecurityException e) {
-                logger.error("failed to start watching file [{}] - {}", path, e);
+                logger.error(Strings.format("failed to start watching file [%s]", path), e);
             }
         });
     }
@@ -90,6 +101,10 @@ private record ChangeListener(Set<String> clusterAliases, Path file, Consumer<Se
         implements
             FileChangesListener {
 
+        public void addClusterAliases(Set<String> clusterAliases) {
+            this.clusterAliases.addAll(clusterAliases);
+        }
+
         @Override
         public void onFileCreated(Path file) {
             onFileChanged(file);
@@ -104,7 +119,7 @@ public void onFileDeleted(Path file) {
         public void onFileChanged(Path file) {
             if (this.file.equals(file)) {
                 reloadConsumer.accept(this.clusterAliases);
-                logger.info("Updated signing configuration for [{}] config(s) using file [{}]", clusterAliases.size(), file);
+                logger.info("Updated signing configuration for [{}] config(s) due to update of file [{}]", clusterAliases.size(), file);
             }
         }
     }
@@ -121,10 +136,11 @@ public void reload(Settings settings) {
                     // Only update signing config if settings were found, since empty config means config deletion
                     if (settingsForCluster.isEmpty() == false) {
                         apiKeySigner.loadSigningConfig(clusterAlias, settingsForCluster, true);
+                        logger.info("Updated signing configuration for [{}] due to reload of secure settings", clusterAlias);
                     }
                 });
         } catch (GeneralSecurityException e) {
-            logger.error("Keystore exception while reloading CrossClusterApiKeySigner", e);
+            logger.error("Keystore exception while reloading signing configuration after reload of secure settings", e);
         }
     }
 
@@ -194,9 +210,6 @@ public void writeTo(StreamOutput out) throws IOException {
         };
     }
 
-    /**
-     * A single-purpose record for the internal implementation of extractSecureSettings
-     */
     private record SecureSettingValue(SecureString value, byte[] sha256Digest) {}
 
 }
