Skip to content

Commit

Permalink
Refactor DefaultKeyStore
Browse files Browse the repository at this point in the history 
Changes:

- Refactored DefaultKeyStore into specialized
  subclasses,
  each managing a distinct responsibility.

- Added missing tests for certificate loading,
  SSL parameter configuration,
  and related processes.

Signed-off-by: Andrey Pleskach <ples@aiven.io>

Switch to SslSettingsManager

Switched to using SslSettingsManager instead of DefaultKeyStore.

Signed-off-by: Andrey Pleskach <ples@aiven.io>
  • Loading branch information
@willyborankin
willyborankin committed Oct 2, 2024
1 parent 830b341 commit 4cb0e6a
Show file tree
Hide file tree
Showing 33 changed files with 3,886 additions and 124 deletions.
3 changes: 3 additions & 0 deletions build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,9 @@ dependencies {
testImplementation('org.awaitility:awaitility:4.2.2') {
exclude(group: 'org.hamcrest', module: 'hamcrest')
}
testImplementation "org.bouncycastle:bcpkix-jdk18on:${versions.bouncycastle}"
testImplementation "org.bouncycastle:bcutil-jdk18on:${versions.bouncycastle}"

// Only osx-x86_64, osx-aarch_64, linux-x86_64, linux-aarch_64, windows-x86_64 are available
if (osdetector.classifier in ["osx-x86_64", "osx-aarch_64", "linux-x86_64", "linux-aarch_64", "windows-x86_64"]) {
testImplementation "io.netty:netty-tcnative-classes:2.0.61.Final"
Expand Down
9 changes: 8 additions & 1 deletion checkstyle/checkstyle.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@
<module name="BeforeExecutionExclusionFileFilter">
<property name="fileNamePattern" value="src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java"/>
</module>
<module name="BeforeExecutionExclusionFileFilter">
<property name="fileNamePattern" value="src/test/java/org/opensearch/security/ssl/SslContextHandlerTest.java"/>
</module>
<module name="BeforeExecutionExclusionFileFilter">
<property name="fileNamePattern" value="src/test/java/org/opensearch/security/ssl/CertificatesRule.java"/>
</module>


<!-- https://checkstyle.org/config_filters.html#SuppressionFilter -->
<module name="SuppressionFilter">
Expand Down Expand Up @@ -221,7 +228,7 @@
<property name="severity" value="error"/>
</module>

<module name="RegexpSingleline">
<module name="RegexpSingleline">
<property name="format" value="extension"/>
<property name="ignoreCase" value="true"/>
<property name="message" value="Extension should only be used sparingly to keep implementations as generic as possible" />
Expand Down
10 changes: 5 additions & 5 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ public List<RestHandler> getRestHandlers(
evaluator,
threadPool,
Objects.requireNonNull(auditLog),
sks,
sslSettingsManager,
Objects.requireNonNull(userService),
sslCertReloadEnabled,
passwordHasher
Expand Down Expand Up @@ -1207,9 +1207,7 @@ public Collection<Object> createComponents(
components.add(userService);
components.add(passwordHasher);

if (!ExternalSecurityKeyStore.hasExternalSslContext(settings)) {
components.add(sks);
}
components.add(sslSettingsManager);
final var allowDefaultInit = settings.getAsBoolean(SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false);
final var useClusterState = useClusterStateToInitSecurityConfig(settings);
if (!SSLConfig.isSslOnlyMode() && !isDisabled(settings) && allowDefaultInit && useClusterState) {
Expand Down Expand Up @@ -2166,7 +2164,9 @@ public PluginSubject getPluginSubject(Plugin plugin) {

@Override
public Optional<SecureSettingsFactory> getSecureSettingFactory(Settings settings) {
return Optional.of(new OpenSearchSecureSettingsFactory(threadPool, sks, evaluateSslExceptionHandler(), securityRestHandler));
return Optional.of(
new OpenSearchSecureSettingsFactory(threadPool, sslSettingsManager, evaluateSslExceptionHandler(), securityRestHandler)
);
}

@SuppressWarnings("removal")
Expand Down
12 changes: 9 additions & 3 deletions src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import org.opensearch.security.configuration.ConfigurationRepository;
import org.opensearch.security.hasher.PasswordHasher;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.ssl.SecurityKeyStore;
import org.opensearch.security.ssl.SslSettingsManager;
import org.opensearch.security.ssl.transport.PrincipalExtractor;
import org.opensearch.security.user.UserService;
import org.opensearch.threadpool.ThreadPool;
Expand All @@ -46,7 +46,7 @@ public static Collection<RestHandler> getHandler(
final PrivilegesEvaluator evaluator,
final ThreadPool threadPool,
final AuditLog auditLog,
final SecurityKeyStore securityKeyStore,
final SslSettingsManager sslSettingsManager,
final UserService userService,
final boolean certificatesReloadEnabled,
final PasswordHasher passwordHasher
Expand Down Expand Up @@ -97,7 +97,13 @@ public static Collection<RestHandler> getHandler(
new MultiTenancyConfigApiAction(clusterService, threadPool, securityApiDependencies),
new RateLimitersApiAction(clusterService, threadPool, securityApiDependencies),
new ConfigUpgradeApiAction(clusterService, threadPool, securityApiDependencies),
new SecuritySSLCertsApiAction(clusterService, threadPool, securityKeyStore, certificatesReloadEnabled, securityApiDependencies),
new SecuritySSLCertsApiAction(
clusterService,
threadPool,
sslSettingsManager,
certificatesReloadEnabled,
securityApiDependencies
),
new CertificatesApiAction(clusterService, threadPool, securityApiDependencies)
);
}
Expand Down
89 changes: 46 additions & 43 deletions src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,10 @@
package org.opensearch.security.dlic.rest.api;

import java.io.IOException;
import java.security.cert.X509Certificate;
import java.util.Arrays;
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;
Expand All @@ -31,8 +30,10 @@
import org.opensearch.rest.RestRequest.Method;
import org.opensearch.security.dlic.rest.validation.ValidationResult;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.ssl.SecurityKeyStore;
import org.opensearch.security.ssl.util.SSLConfigConstants;
import org.opensearch.security.ssl.SslContextHandler;
import org.opensearch.security.ssl.SslSettingsManager;
import org.opensearch.security.ssl.config.CertType;
import org.opensearch.security.ssl.config.Certificate;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.threadpool.ThreadPool;

Expand Down Expand Up @@ -62,23 +63,20 @@ public class SecuritySSLCertsApiAction extends AbstractApiAction {
)
);

private final SecurityKeyStore securityKeyStore;
private final SslSettingsManager sslSettingsManager;

private final boolean certificatesReloadEnabled;

private final boolean httpsEnabled;

public SecuritySSLCertsApiAction(
final ClusterService clusterService,
final ThreadPool threadPool,
final SecurityKeyStore securityKeyStore,
final SslSettingsManager sslSettingsManager,
final boolean certificatesReloadEnabled,
final SecurityApiDependencies securityApiDependencies
) {
super(Endpoint.SSL, clusterService, threadPool, securityApiDependencies);
this.securityKeyStore = securityKeyStore;
this.sslSettingsManager = sslSettingsManager;
this.certificatesReloadEnabled = certificatesReloadEnabled;
this.httpsEnabled = securityApiDependencies.settings().getAsBoolean(SSLConfigConstants.SECURITY_SSL_HTTP_ENABLED, true);
this.requestHandlersBuilder.configureRequestHandlers(this::securitySSLCertsRequestHandlers);
}

Expand Down Expand Up @@ -108,10 +106,10 @@ private void securitySSLCertsRequestHandlers(RequestHandler.RequestHandlersBuild
.verifyAccessForAllMethods()
.override(
Method.GET,
(channel, request, client) -> withSecurityKeyStore().valid(keyStore -> loadCertificates(channel, keyStore))
(channel, request, client) -> withSecurityKeyStore().valid(ignore -> loadCertificates(channel))
.error((status, toXContent) -> response(channel, status, toXContent))
)
.override(Method.PUT, (channel, request, client) -> withSecurityKeyStore().valid(keyStore -> {
.override(Method.PUT, (channel, request, client) -> withSecurityKeyStore().valid(ignore -> {
if (!certificatesReloadEnabled) {
badRequest(
channel,
Expand All @@ -123,7 +121,7 @@ private void securitySSLCertsRequestHandlers(RequestHandler.RequestHandlersBuild
)
);
} else {
reloadCertificates(channel, request, keyStore);
reloadCertificates(channel, request);
}
}).error((status, toXContent) -> response(channel, status, toXContent)));
}
Expand All @@ -138,65 +136,70 @@ boolean accessHandler(final RestRequest request) {
}
}

ValidationResult<SecurityKeyStore> withSecurityKeyStore() {
if (securityKeyStore == null) {
ValidationResult<SslSettingsManager> withSecurityKeyStore() {
if (sslSettingsManager == null) {
return ValidationResult.error(RestStatus.OK, badRequestMessage("keystore is not initialized"));
}
return ValidationResult.success(securityKeyStore);
return ValidationResult.success(sslSettingsManager);
}

protected void loadCertificates(final RestChannel channel, final SecurityKeyStore keyStore) throws IOException {
protected void loadCertificates(final RestChannel channel) throws IOException {
ok(
channel,
(builder, params) -> builder.startObject()
.field("http_certificates_list", httpsEnabled ? generateCertDetailList(keyStore.getHttpCerts()) : null)
.field("transport_certificates_list", generateCertDetailList(keyStore.getTransportCerts()))
.field(
"http_certificates_list",
generateCertDetailList(
sslSettingsManager.sslContextHandler(CertType.HTTP).map(SslContextHandler::keyMaterialCertificates).orElse(null)
)
)
.field(
"transport_certificates_list",
generateCertDetailList(
sslSettingsManager.sslContextHandler(CertType.TRANSPORT)
.map(SslContextHandler::keyMaterialCertificates)
.orElse(null)
)
)
.endObject()
);
}

private List<Map<String, String>> generateCertDetailList(final X509Certificate[] certs) {
private List<Map<String, String>> generateCertDetailList(final Stream<Certificate> certs) {
if (certs == null) {
return null;
}
return Arrays.stream(certs).map(cert -> {
final String issuerDn = cert != null && cert.getIssuerX500Principal() != null ? cert.getIssuerX500Principal().getName() : "";
final String subjectDn = cert != null && cert.getSubjectX500Principal() != null ? cert.getSubjectX500Principal().getName() : "";

final String san = securityKeyStore.getSubjectAlternativeNames(cert);

final String notBefore = cert != null && cert.getNotBefore() != null ? cert.getNotBefore().toInstant().toString() : "";
final String notAfter = cert != null && cert.getNotAfter() != null ? cert.getNotAfter().toInstant().toString() : "";
return ImmutableMap.of(
return certs.map(
c -> ImmutableMap.of(
"issuer_dn",
issuerDn,
c.issuer(),
"subject_dn",
subjectDn,
c.subject(),
"san",
san,
c.subjectAlternativeNames(),
"not_before",
notBefore,
c.notBefore(),
"not_after",
notAfter
);
}).collect(Collectors.toList());
c.notAfter()
)
).collect(Collectors.toList());
}

protected void reloadCertificates(final RestChannel channel, final RestRequest request, final SecurityKeyStore keyStore)
throws IOException {
protected void reloadCertificates(final RestChannel channel, final RestRequest request) throws IOException {
final String certType = request.param("certType").toLowerCase().trim();
try {
switch (certType) {
case "http":
if (!httpsEnabled) {
if (sslSettingsManager.sslConfiguration(CertType.HTTP).isPresent()) {
sslSettingsManager.reloadSslContext(CertType.HTTP);
ok(channel, (builder, params) -> builder.startObject().field("message", "updated http certs").endObject());
} else {
badRequest(channel, "SSL for HTTP is disabled");
return;
}
keyStore.initHttpSSLConfig();
ok(channel, (builder, params) -> builder.startObject().field("message", "updated http certs").endObject());
break;
case "transport":
keyStore.initTransportSSLConfig();
sslSettingsManager.reloadSslContext(CertType.TRANSPORT);
sslSettingsManager.reloadSslContext(CertType.TRANSPORT_CLIENT);
ok(channel, (builder, params) -> builder.startObject().field("message", "updated transport certs").endObject());
break;
default:
Expand Down
50 changes: 22 additions & 28 deletions .../java/org/opensearch/security/dlic/rest/api/ssl/TransportCertificatesInfoNodesAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,22 @@
package org.opensearch.security.dlic.rest.api.ssl;

import java.io.IOException;
import java.security.cert.X509Certificate;
import java.util.List;
import java.util.Map;

import com.google.common.collect.ImmutableList;
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;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.inject.Inject;
import org.opensearch.common.settings.Settings;
import org.opensearch.core.common.io.stream.StreamInput;
import org.opensearch.core.common.io.stream.StreamOutput;
import org.opensearch.security.ssl.DefaultSecurityKeyStore;
import org.opensearch.security.ssl.util.SSLConfigConstants;
import org.opensearch.security.ssl.SslContextHandler;
import org.opensearch.security.ssl.SslSettingsManager;
import org.opensearch.security.ssl.config.CertType;
import org.opensearch.security.ssl.config.Certificate;
import org.opensearch.threadpool.ThreadPool;
import org.opensearch.transport.TransportRequest;
import org.opensearch.transport.TransportService;
Expand All @@ -38,18 +38,15 @@ public class TransportCertificatesInfoNodesAction extends TransportNodesAction<
TransportCertificatesInfoNodesAction.NodeRequest,
CertificatesNodesResponse.CertificatesNodeResponse> {

private final DefaultSecurityKeyStore securityKeyStore;

private final boolean httpsEnabled;
private final SslSettingsManager sslSettingsManager;

@Inject
public TransportCertificatesInfoNodesAction(
final Settings settings,
final ThreadPool threadPool,
final ClusterService clusterService,
final TransportService transportService,
final ActionFilters actionFilters,
final DefaultSecurityKeyStore securityKeyStore
final SslSettingsManager sslSettingsManager
) {
super(
CertificatesActionType.NAME,
Expand All @@ -62,8 +59,7 @@ public TransportCertificatesInfoNodesAction(
ThreadPool.Names.GENERIC,
CertificatesNodesResponse.CertificatesNodeResponse.class
);
this.httpsEnabled = settings.getAsBoolean(SSLConfigConstants.SECURITY_SSL_HTTP_ENABLED, true);
this.securityKeyStore = securityKeyStore;
this.sslSettingsManager = sslSettingsManager;
}

@Override
Expand All @@ -89,12 +85,6 @@ protected CertificatesNodesResponse.CertificatesNodeResponse newNodeResponse(fin
protected CertificatesNodesResponse.CertificatesNodeResponse nodeOperation(final NodeRequest request) {
final var sslCertRequest = request.sslCertsInfoNodesRequest;

if (securityKeyStore == null) {
return new CertificatesNodesResponse.CertificatesNodeResponse(
clusterService.localNode(),
new IllegalStateException("keystore is not initialized")
);
}
try {
return new CertificatesNodesResponse.CertificatesNodeResponse(
clusterService.localNode(),
Expand All @@ -109,23 +99,27 @@ protected CertificatesInfo loadCertificates(final CertificateType certificateTyp
var httpCertificates = List.<CertificateInfo>of();
var transportsCertificates = List.<CertificateInfo>of();
if (CertificateType.isHttp(certificateType)) {
httpCertificates = httpsEnabled ? certificatesDetails(securityKeyStore.getHttpCerts()) : List.of();
httpCertificates = sslSettingsManager.sslContextHandler(CertType.HTTP)
.map(SslContextHandler::keyMaterialCertificates)
.map(this::certificatesDetails)
.orElse(List.of());
}
if (CertificateType.isTransport(certificateType)) {
transportsCertificates = certificatesDetails(securityKeyStore.getTransportCerts());
transportsCertificates = sslSettingsManager.sslContextHandler(CertType.TRANSPORT)
.map(SslContextHandler::keyMaterialCertificates)
.map(this::certificatesDetails)
.orElse(List.of());
}
return new CertificatesInfo(Map.of(CertificateType.HTTP, httpCertificates, CertificateType.TRANSPORT, transportsCertificates));
}

private List<CertificateInfo> certificatesDetails(final X509Certificate[] certs) {
if (certs == null) {
private List<CertificateInfo> certificatesDetails(final Stream<Certificate> certificateStream) {
if (certificateStream == null) {
return null;
}
final var certificates = ImmutableList.<CertificateInfo>builder();
for (final var c : certs) {
certificates.add(CertificateInfo.from(c, securityKeyStore.getSubjectAlternativeNames(c)));
}
return certificates.build();
return certificateStream.map(
c -> new CertificateInfo(c.subject(), c.subjectAlternativeNames(), c.issuer(), c.notAfter(), c.notBefore())
).collect(Collectors.toList());
}

public static class NodeRequest extends TransportRequest {
Expand Down
Loading

0 comments on commit 4cb0e6a

Please sign in to comment.