Fix tests

Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin · Aug 23, 2024 · b353893 · b353893
1 parent 8351e3d
commit b353893
Show file tree

Hide file tree

Showing 11 changed files with 150 additions and 80 deletions.
diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiAction.java
@@ -11,10 +11,17 @@
 
 package org.opensearch.security.dlic.rest.api;
 
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+
 import org.opensearch.OpenSearchSecurityException;
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.core.rest.RestStatus;
@@ -29,12 +36,6 @@
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.threadpool.ThreadPool;
 
-import java.io.IOException;
-import java.util.List;
-import java.util.Map;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
-
 import static org.opensearch.security.dlic.rest.api.Responses.badRequest;
 import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage;
 import static org.opensearch.security.dlic.rest.api.Responses.ok;
@@ -151,9 +152,7 @@ protected void loadCertificates(final RestChannel channel) throws IOException {
                 .field(
                     "http_certificates_list",
                     generateCertDetailList(
-                        sslSettingsManager.sslContextHandler(SSL_HTTP_PREFIX)
-                            .map(SslContextHandler::keyMaterialCertificates)
-                            .orElse(null)
+                        sslSettingsManager.sslContextHandler(SSL_HTTP_PREFIX).map(SslContextHandler::keyMaterialCertificates).orElse(null)
                     )
                 )
                 .field(

diff --git a/.../java/org/opensearch/security/dlic/rest/api/ssl/TransportCertificatesInfoNodesAction.java b/.../java/org/opensearch/security/dlic/rest/api/ssl/TransportCertificatesInfoNodesAction.java
@@ -11,6 +11,12 @@
 
 package org.opensearch.security.dlic.rest.api.ssl;
 
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
 import org.opensearch.action.FailedNodeException;
 import org.opensearch.action.support.ActionFilters;
 import org.opensearch.action.support.nodes.TransportNodesAction;
@@ -25,12 +31,6 @@
 import org.opensearch.transport.TransportRequest;
 import org.opensearch.transport.TransportService;
 
-import java.io.IOException;
-import java.util.List;
-import java.util.Map;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
-
 import static org.opensearch.security.ssl.util.SSLConfigConstants.SSL_HTTP_PREFIX;
 import static org.opensearch.security.ssl.util.SSLConfigConstants.SSL_TRANSPORT_PREFIX;
 

diff --git a/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java b/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java
@@ -17,11 +17,24 @@
 
 package org.opensearch.security.ssl;
 
+import java.nio.file.Path;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.function.Function;
+import java.util.function.Supplier;
+
 import com.fasterxml.jackson.databind.InjectableValues;
-import io.netty.handler.ssl.OpenSsl;
-import io.netty.util.internal.PlatformDependent;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+
 import org.opensearch.OpenSearchException;
 import org.opensearch.SpecialPermission;
 import org.opensearch.Version;
@@ -78,19 +91,8 @@
 import org.opensearch.transport.netty4.ssl.SecureNetty4Transport;
 import org.opensearch.watcher.ResourceWatcherService;
 
-import java.nio.file.Path;
-import java.security.AccessController;
-import java.security.PrivilegedAction;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.function.Function;
-import java.util.function.Supplier;
+import io.netty.handler.ssl.OpenSsl;
+import io.netty.util.internal.PlatformDependent;
 
 //For ES5 this class has only effect when SSL only plugin is installed
 public class OpenSearchSecuritySSLPlugin extends Plugin implements SystemIndexPlugin, NetworkPlugin {
@@ -305,7 +307,15 @@ public List<RestHandler> getRestHandlers(
         final List<RestHandler> handlers = new ArrayList<RestHandler>(1);
 
         if (!client) {
-            handlers.add(new SecuritySSLInfoAction(settings, configPath, restController, sslSettingsManager, Objects.requireNonNull(principalExtractor)));
+            handlers.add(
+                new SecuritySSLInfoAction(
+                    settings,
+                    configPath,
+                    restController,
+                    sslSettingsManager,
+                    Objects.requireNonNull(principalExtractor)
+                )
+            );
         }
 
         return handlers;
@@ -668,7 +678,9 @@ public List<String> getSettingsFilter() {
 
     @Override
     public Optional<SecureSettingsFactory> getSecureSettingFactory(Settings settings) {
-        return Optional.of(new OpenSearchSecureSettingsFactory(threadPool, sslSettingsManager, NOOP_SSL_EXCEPTION_HANDLER, securityRestHandler));
+        return Optional.of(
+            new OpenSearchSecureSettingsFactory(threadPool, sslSettingsManager, NOOP_SSL_EXCEPTION_HANDLER, securityRestHandler)
+        );
     }
 
     protected Settings migrateSettings(Settings settings) {

diff --git a/src/main/java/org/opensearch/security/ssl/SslContextHandler.java b/src/main/java/org/opensearch/security/ssl/SslContextHandler.java
@@ -14,7 +14,6 @@
 import java.nio.charset.StandardCharsets;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
-import java.util.Date;
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -70,6 +69,11 @@ Stream<Certificate> keyMaterialCertificates(final List<Certificate> certificates
 
     void reloadSslContext() throws CertificateException {
         final var newCertificates = sslConfiguration.certificates();
+
+        if (sameCertificates(newCertificates)) {
+            return;
+        }
+
         validateNewCertificates(newCertificates);
         invalidateSessions();
         if (sslContext.isClient()) {
@@ -93,20 +97,10 @@ private boolean sameCertificates(final List<Certificate> newCertificates) {
         return currentCertSignatureSet.equals(newCertSignatureSet);
     }
 
-    private boolean hasValidExpiryDates(final List<Certificate> newCertificates) {
-        // Get the earliest expiry date for current certificates
-        final Date earliestExpiryDate = keyMaterialCertificates().map(Certificate::x509Certificate)
-            .map(X509Certificate::getNotAfter)
-            .min(Date::compareTo)
-            .get();
-        // New certificates that expire before or on the same date as the current ones are invalid.
-        boolean newCertsExpireBeforeCurrentCerts = keyMaterialCertificates(newCertificates).map(Certificate::x509Certificate)
-            .anyMatch(c -> {
-                Date notAfterDate = c.getNotAfter();
-                return notAfterDate.before(earliestExpiryDate) || notAfterDate.equals(earliestExpiryDate);
-            });
-
-        return !newCertsExpireBeforeCurrentCerts;
+    private void validateDates(final List<Certificate> newCertificates) throws CertificateException {
+        for (final var certificate : newCertificates) {
+            certificate.x509Certificate().checkValidity();
+        }
     }
 
     private void validateSubjectDns(final List<Certificate> newCertificates) throws CertificateException {
@@ -154,12 +148,7 @@ private void validateSans(final List<Certificate> newCertificates) throws Certif
     }
 
     private void validateNewCertificates(final List<Certificate> newCertificates) throws CertificateException {
-        if (sameCertificates(newCertificates)) {
-            throw new CertificateException("Existing certificates are the same as new one");
-        }
-        if (!hasValidExpiryDates(newCertificates)) {
-            throw new CertificateException("New certificates should not expire before the current ones");
-        }
+        validateDates(newCertificates);
         validateSubjectDns(newCertificates);
         validateIssuerDns(newCertificates);
         validateSans(newCertificates);

diff --git a/src/main/java/org/opensearch/security/ssl/SslSettingsManager.java b/src/main/java/org/opensearch/security/ssl/SslSettingsManager.java
@@ -197,6 +197,7 @@ private void validateHttpSettings(final Settings httpSettings) {
         if (!httpSettings.getAsBoolean(ENABLED, SECURITY_SSL_HTTP_ENABLED_DEFAULT)) return;
 
         final var clientAuth = ClientAuth.valueOf(httpSettings.get(CLIENT_AUTH_MODE, ClientAuth.OPTIONAL.name()).toUpperCase(Locale.ROOT));
+
         if (hasPemStoreSettings(httpSettings)) {
             if (!httpSettings.hasValue(PEM_CERT_FILEPATH) || !httpSettings.hasValue(PEM_KEY_FILEPATH)) {
                 throw new OpenSearchException(

diff --git a/src/main/java/org/opensearch/security/ssl/config/SslParameters.java b/src/main/java/org/opensearch/security/ssl/config/SslParameters.java
@@ -184,10 +184,10 @@ public SslParameters load(final boolean http) {
                 ciphers(provider, sslConfigSettings)
             );
             if (sslParameters.allowedProtocols().isEmpty()) {
-                throw new OpenSearchSecurityException("Couldn't resolve SSL protocols for " + (http ? "HTTP" : "Transport") + " layer");
+                throw new OpenSearchSecurityException("No ssl protocols for " + (http ? "HTTP" : "Transport") + " layer");
             }
             if (sslParameters.allowedCiphers().isEmpty()) {
-                throw new OpenSearchSecurityException("Couldn't resolve SSL ciphers for " + (http ? "HTTP" : "Transport") + " layer");
+                throw new OpenSearchSecurityException("No valid cipher suites for " + (http ? "HTTP" : "Transport") + " layer");
             }
             return sslParameters;
         }

diff --git a/src/main/java/org/opensearch/security/ssl/rest/SecuritySSLInfoAction.java b/src/main/java/org/opensearch/security/ssl/rest/SecuritySSLInfoAction.java
@@ -17,9 +17,17 @@
 
 package org.opensearch.security.ssl.rest;
 
-import io.netty.handler.ssl.OpenSsl;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.security.cert.X509Certificate;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.core.rest.RestStatus;
@@ -38,13 +46,7 @@
 import org.opensearch.security.ssl.util.SSLRequestHelper;
 import org.opensearch.security.ssl.util.SSLRequestHelper.SSLInfo;
 
-import java.io.IOException;
-import java.nio.file.Path;
-import java.security.cert.X509Certificate;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.List;
-import java.util.stream.Collectors;
+import io.netty.handler.ssl.OpenSsl;
 
 import static org.opensearch.security.ssl.util.SSLConfigConstants.SSL_HTTP_PREFIX;
 import static org.opensearch.security.ssl.util.SSLConfigConstants.SSL_TRANSPORT_CLIENT_PREFIX;

diff --git a/src/test/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPluginTest.java b/src/test/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPluginTest.java
@@ -9,6 +9,7 @@
 package org.opensearch.security.ssl;
 
 import java.io.IOException;
+import java.nio.file.Path;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
@@ -26,6 +27,7 @@
 import org.opensearch.common.network.NetworkModule;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
+import org.opensearch.env.Environment;
 import org.opensearch.http.HttpServerTransport;
 import org.opensearch.http.netty4.ssl.SecureNetty4HttpServerTransport;
 import org.opensearch.plugins.SecureHttpTransportSettingsProvider;
@@ -55,16 +57,20 @@ public class OpenSearchSecuritySSLPluginTest extends AbstractSecurityUnitTest {
     private SecureTransportSettingsProvider secureTransportSettingsProvider;
     private ClusterSettings clusterSettings;
 
+    private Path esFolder;
+
     @Before
     public void setUp() {
+        esFolder = FileHelper.getAbsoluteFilePathFromClassPath("ssl/kirk-keystore.jks").getParent().getParent();
         settings = Settings.builder()
+            .put(Environment.PATH_HOME_SETTING.getKey(), esFolder)
             .put(
                 SSLConfigConstants.SECURITY_SSL_TRANSPORT_KEYSTORE_FILEPATH,
                 FileHelper.getAbsoluteFilePathFromClassPath("ssl/kirk-keystore.jks")
             )
             .put(
-                SSLConfigConstants.SECURITY_SSL_HTTP_PEMTRUSTEDCAS_FILEPATH,
-                FileHelper.getAbsoluteFilePathFromClassPath("ssl/root-ca.pem")
+                SSLConfigConstants.SECURITY_SSL_HTTP_TRUSTSTORE_FILEPATH,
+                FileHelper.getAbsoluteFilePathFromClassPath("ssl/node-0-keystore.jks")
             )
             .put(
                 SSLConfigConstants.SECURITY_SSL_TRANSPORT_TRUSTSTORE_FILEPATH,
@@ -116,7 +122,7 @@ public Optional<SSLEngine> buildSecureHttpServerEngine(Settings settings, HttpSe
 
     @Test
     public void testRegisterSecureHttpTransport() throws IOException {
-        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(settings, null, false)) {
+        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(settings, esFolder, false)) {
             final Map<String, Supplier<HttpServerTransport>> transports = plugin.getSecureHttpTransports(
                 settings,
                 MOCK_POOL,
@@ -140,7 +146,7 @@ public void testRegisterSecureHttpTransport() throws IOException {
 
     @Test
     public void testRegisterSecureTransport() throws IOException {
-        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(settings, null, false)) {
+        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(settings, esFolder, false)) {
             final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
                 settings,
                 MOCK_POOL,
@@ -165,7 +171,7 @@ public void testRegisterSecureTransportWithDeprecatedSecuirtyPluginSettings() th
             .put(SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION, false)
             .build();
 
-        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(deprecated, null, false)) {
+        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(deprecated, esFolder, false)) {
             final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
                 deprecated,
                 MOCK_POOL,
@@ -190,7 +196,7 @@ public void testRegisterSecureTransportWithNetworkModuleSettings() throws IOExce
             .put(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY, false)
             .build();
 
-        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(migrated, null, false)) {
+        try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(migrated, esFolder, false)) {
             final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
                 migrated,
                 MOCK_POOL,
@@ -229,7 +235,7 @@ public void testRegisterSecureTransportWithDuplicateSettings() throws IOExceptio
                 .put(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY, false)
                 .build();
 
-            try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(migrated, null, false)) {
+            try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(migrated, esFolder, false)) {
                 final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
                     migrated,
                     MOCK_POOL,

diff --git a/src/test/java/org/opensearch/security/ssl/SSLTest.java b/src/test/java/org/opensearch/security/ssl/SSLTest.java
@@ -568,7 +568,7 @@ public void testHttpsAndNodeSSLFailedCipher() throws Exception {
             Assert.fail();
         } catch (Exception e1) {
             Throwable e = ExceptionUtils.getRootCause(e1);
-            Assert.assertTrue(e.toString(), e.toString().contains("no valid cipher"));
+            Assert.assertTrue(e.toString(), e.toString().contains("No valid cipher"));
         }
     }