Skip to content

Commit

Permalink
Address code review comments
Browse files Browse the repository at this point in the history
Signed-off-by: Andriy Redko <[email protected]>
  • Loading branch information
reta committed Mar 26, 2024
1 parent 268431c commit f2e2209
Show file tree
Hide file tree
Showing 2 changed files with 257 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,13 @@ public List<Setting<?>> getSettings() {
Property.Filtered
)
);
settings.add(SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION);
settings.add(SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME);
if (!settings.stream().anyMatch(s -> s.getKey().equalsIgnoreCase(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY))) {
settings.add(SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION);
}
if (!settings.stream()
.anyMatch(s -> s.getKey().equalsIgnoreCase(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME_KEY))) {
settings.add(SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME);
}
settings.add(
Setting.simpleString(SSLConfigConstants.SECURITY_SSL_TRANSPORT_KEYSTORE_FILEPATH, Property.NodeScope, Property.Filtered)
);
Expand Down Expand Up @@ -686,7 +691,7 @@ protected Settings migrateSettings(Settings settings) {
+ NetworkModule.TRANSPORT_SSL_DUAL_MODE_ENABLED_KEY
+ ", "
+ SecuritySettings.SSL_DUAL_MODE_SETTING.getKey()
+ "(deprecated)] could be specified but not both"
+ " (deprecated)] could be specified but not both"
);
}
}
Expand All @@ -700,10 +705,10 @@ protected Settings migrateSettings(Settings settings) {
if (SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME.exists(settings)) {
throw new OpenSearchException(
"Only one of the settings ["
+ NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME
+ NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME_KEY
+ ", "
+ SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME.getKey()
+ "(deprecated)] could be specified but not both"
+ " (deprecated)] could be specified but not both"
);
}
}
Expand All @@ -720,7 +725,7 @@ protected Settings migrateSettings(Settings settings) {
+ NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY
+ ", "
+ SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION.getKey()
+ "(deprecated)] could be specified but not both"
+ " (deprecated)] could be specified but not both"
);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,246 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.security.ssl;

import java.io.IOException;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Supplier;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLException;

import org.junit.Before;
import org.junit.Test;

import org.opensearch.OpenSearchException;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.network.NetworkModule;
import org.opensearch.common.settings.ClusterSettings;
import org.opensearch.common.settings.Settings;
import org.opensearch.http.HttpServerTransport;
import org.opensearch.plugins.SecureTransportSettingsProvider;
import org.opensearch.security.ssl.util.SSLConfigConstants;
import org.opensearch.security.support.SecuritySettings;
import org.opensearch.security.test.AbstractSecurityUnitTest;
import org.opensearch.security.test.helper.file.FileHelper;
import org.opensearch.telemetry.tracing.noop.NoopTracer;
import org.opensearch.transport.TcpTransport;
import org.opensearch.transport.Transport;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.collection.IsMapContaining.hasKey;
import static org.junit.Assert.assertThrows;

public class OpenSearchSecuritySSLPluginTest extends AbstractSecurityUnitTest {
private Settings settings;
private SecureTransportSettingsProvider secureTransportSettingsProvider;
private ClusterSettings clusterSettings;

@Before
public void setUp() {
settings = Settings.builder()
.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")
)
.put(
SSLConfigConstants.SECURITY_SSL_TRANSPORT_TRUSTSTORE_FILEPATH,
FileHelper.getAbsoluteFilePathFromClassPath("ssl/truststore.jks")
)
.put(
SSLConfigConstants.SECURITY_SSL_TRANSPORT_KEYSTORE_FILEPATH,
FileHelper.getAbsoluteFilePathFromClassPath("ssl/kirk-keystore.jks")
)
.put(
SSLConfigConstants.SECURITY_SSL_HTTP_KEYSTORE_FILEPATH,
FileHelper.getAbsoluteFilePathFromClassPath("ssl/node-0-keystore.jks")
)
.put(SSLConfigConstants.SECURITY_SSL_HTTP_ENABLED, true)
.put(OpenSearchSecuritySSLPlugin.CLIENT_TYPE, "node")
.build();

secureTransportSettingsProvider = new SecureTransportSettingsProvider() {
@Override
public Optional<ServerExceptionHandler> buildHttpServerExceptionHandler(Settings settings, HttpServerTransport transport) {
return Optional.empty();
}

@Override
public Optional<ServerExceptionHandler> buildServerTransportExceptionHandler(Settings settings, TcpTransport transport) {
return Optional.empty();
}

@Override
public Optional<SSLEngine> buildSecureHttpServerEngine(Settings settings, HttpServerTransport transport) throws SSLException {
return Optional.empty();
}

@Override
public Optional<SSLEngine> buildSecureServerTransportEngine(Settings settings, TcpTransport transport) throws SSLException {
return Optional.empty();
}

@Override
public Optional<SSLEngine> buildSecureClientTransportEngine(Settings settings, String hostname, int port) throws SSLException {
return Optional.empty();
}
};

clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
}

@Test
public void testRegisterSecureHttpTransport() throws IOException {
try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(settings, null, false)) {
final Map<String, Supplier<HttpServerTransport>> transports = plugin.getSecureHttpTransports(
settings,
MOCK_POOL,
null,
null,
null,
null,
null,
null,
clusterSettings,
secureTransportSettingsProvider,
NoopTracer.INSTANCE
);
assertThat(transports, hasKey("org.opensearch.security.ssl.http.netty.SecuritySSLNettyHttpServerTransport"));
}
}

@Test
public void testRegisterSecureTransport() throws IOException {
try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(settings, null, false)) {
final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
settings,
MOCK_POOL,
null,
null,
null,
null,
secureTransportSettingsProvider,
NoopTracer.INSTANCE
);
assertThat(transports, hasKey("org.opensearch.security.ssl.http.netty.SecuritySSLNettyTransport"));
}
}

@Test
public void testRegisterSecureTransportWithDeprecatedSecuirtyPluginSettings() throws IOException {
final Settings deprecated = Settings.builder()
.put(settings)
.put(SecuritySettings.SSL_DUAL_MODE_SETTING.getKey(), true)
.put(SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME, false)
.put(SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION, false)
.build();

try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(deprecated, null, false)) {
final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
deprecated,
MOCK_POOL,
null,
null,
null,
null,
secureTransportSettingsProvider,
NoopTracer.INSTANCE
);
assertThat(transports, hasKey("org.opensearch.security.ssl.http.netty.SecuritySSLNettyTransport"));
assertThat(transports.get("org.opensearch.security.ssl.http.netty.SecuritySSLNettyTransport").get(), not(nullValue()));
}
}

@Test
public void testRegisterSecureTransportWithNetworkModuleSettings() throws IOException {
final Settings migrated = Settings.builder()
.put(settings)
.put(NetworkModule.TRANSPORT_SSL_DUAL_MODE_ENABLED_KEY, true)
.put(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME_KEY, false)
.put(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY, false)
.build();

try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(migrated, null, false)) {
final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
migrated,
MOCK_POOL,
null,
null,
null,
null,
secureTransportSettingsProvider,
NoopTracer.INSTANCE
);
assertThat(transports, hasKey("org.opensearch.security.ssl.http.netty.SecuritySSLNettyTransport"));
assertThat(transports.get("org.opensearch.security.ssl.http.netty.SecuritySSLNettyTransport").get(), not(nullValue()));
}
}

@Test
public void testRegisterSecureTransportWithDuplicateSettings() throws IOException {
final Collection<Tuple<String, String>> duplicates = List.of(
Tuple.tuple(SecuritySettings.SSL_DUAL_MODE_SETTING.getKey(), NetworkModule.TRANSPORT_SSL_DUAL_MODE_ENABLED_KEY),
Tuple.tuple(
SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME,
NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME_KEY
),
Tuple.tuple(
SSLConfigConstants.SECURITY_SSL_TRANSPORT_ENFORCE_HOSTNAME_VERIFICATION,
NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY
)
);

for (final Tuple<String, String> duplicate : duplicates) {
final Settings migrated = Settings.builder()
.put(settings)
.put(duplicate.v1(), true)
.put(NetworkModule.TRANSPORT_SSL_DUAL_MODE_ENABLED_KEY, true)
.put(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_RESOLVE_HOST_NAME_KEY, false)
.put(NetworkModule.TRANSPORT_SSL_ENFORCE_HOSTNAME_VERIFICATION_KEY, false)
.build();

try (OpenSearchSecuritySSLPlugin plugin = new OpenSearchSecuritySSLPlugin(migrated, null, false)) {
final Map<String, Supplier<Transport>> transports = plugin.getSecureTransports(
migrated,
MOCK_POOL,
null,
null,
null,
null,
secureTransportSettingsProvider,
NoopTracer.INSTANCE
);
assertThat(transports, hasKey("org.opensearch.security.ssl.http.netty.SecuritySSLNettyTransport"));
final OpenSearchException ex = assertThrows(
OpenSearchException.class,
transports.get("org.opensearch.security.ssl.http.netty.SecuritySSLNettyTransport")::get
);
assertThat(
ex.getMessage(),
containsString(
"Only one of the settings ["
+ duplicate.v2()
+ ", "
+ duplicate.v1()
+ " (deprecated)] could be specified but not both"
)
);
}
}
}
}

0 comments on commit f2e2209

Please sign in to comment.