Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only check validity of certs in the chain of the node certificates #4979

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions src/main/java/org/opensearch/security/ssl/SslConfiguration.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
import java.security.PrivilegedExceptionAction;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.TrustManagerFactory;
import javax.security.auth.x500.X500Principal;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -74,7 +76,10 @@
}

public TrustManagerFactory trustStoreFactory() {
return trustStoreConfiguration.createTrustManagerFactory(sslParameters.shouldValidateNewCertDNs());
return trustStoreConfiguration.createTrustManagerFactory(
sslParameters.shouldValidateNewCertDNs(),
keyStoreConfiguration.getIssuerDns()

Check warning on line 81 in src/main/java/org/opensearch/security/ssl/SslConfiguration.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/ssl/SslConfiguration.java#L79-L81

Added lines #L79 - L81 were not covered by tests
);
}

public SslParameters sslParameters() {
Expand All @@ -84,10 +89,10 @@
@SuppressWarnings("removal")
SslContext buildServerSslContext(final boolean validateCertificates) {
try {
return AccessController.doPrivileged(
(PrivilegedExceptionAction<SslContext>) () -> SslContextBuilder.forServer(
keyStoreConfiguration.createKeyManagerFactory(validateCertificates)
)
return AccessController.doPrivileged((PrivilegedExceptionAction<SslContext>) () -> {
KeyManagerFactory kmFactory = keyStoreConfiguration.createKeyManagerFactory(validateCertificates);
Set<X500Principal> issuerDns = keyStoreConfiguration.getIssuerDns();
return SslContextBuilder.forServer(kmFactory)
.sslProvider(sslParameters.provider())
.clientAuth(sslParameters.clientAuth())
.protocols(sslParameters.allowedProtocols().toArray(new String[0]))
Expand All @@ -112,9 +117,9 @@
ApplicationProtocolNames.HTTP_1_1
)
)
.trustManager(trustStoreConfiguration.createTrustManagerFactory(validateCertificates))
.build()
);
.trustManager(trustStoreConfiguration.createTrustManagerFactory(validateCertificates, issuerDns))
.build();
});
} catch (PrivilegedActionException e) {
throw new OpenSearchException("Failed to build server SSL context", e);
}
Expand All @@ -123,19 +128,21 @@
@SuppressWarnings("removal")
SslContext buildClientSslContext(final boolean validateCertificates) {
try {
return AccessController.doPrivileged(
(PrivilegedExceptionAction<SslContext>) () -> SslContextBuilder.forClient()
return AccessController.doPrivileged((PrivilegedExceptionAction<SslContext>) () -> {
KeyManagerFactory kmFactory = keyStoreConfiguration.createKeyManagerFactory(validateCertificates);
Set<X500Principal> issuerDns = keyStoreConfiguration.getIssuerDns();
return SslContextBuilder.forClient()
.sslProvider(sslParameters.provider())
.protocols(sslParameters.allowedProtocols())
.ciphers(sslParameters.allowedCiphers())
.applicationProtocolConfig(ApplicationProtocolConfig.DISABLED)
.sessionCacheSize(0)
.sessionTimeout(0)
.sslProvider(sslParameters.provider())
.keyManager(keyStoreConfiguration.createKeyManagerFactory(validateCertificates))
.trustManager(trustStoreConfiguration.createTrustManagerFactory(validateCertificates))
.build()
);
.keyManager(kmFactory)
.trustManager(trustStoreConfiguration.createTrustManagerFactory(validateCertificates, issuerDns))
.build();
});
} catch (PrivilegedActionException e) {
throw new OpenSearchException("Failed to build client SSL context", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import javax.net.ssl.KeyManagerFactory;
import javax.security.auth.x500.X500Principal;

import com.google.common.collect.ImmutableList;

Expand All @@ -41,6 +44,15 @@ default KeyManagerFactory createKeyManagerFactory(boolean validateCertificates)
return buildKeyManagerFactory(keyStore.v1(), keyStore.v2());
}

default Set<X500Principal> getIssuerDns() {
Set<X500Principal> issuerDns = new HashSet<>();
final List<Certificate> certificates = loadCertificates();
for (Certificate certificate : certificates) {
issuerDns.add(certificate.x509Certificate().getIssuerX500Principal());
}
return issuerDns;
}

default KeyManagerFactory buildKeyManagerFactory(final KeyStore keyStore, final char[] password) {
try {
final var keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,16 @@
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.security.spec.InvalidKeySpecException;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import javax.crypto.NoSuchPaddingException;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLSessionContext;
import javax.security.auth.x500.X500Principal;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.OpenSearchException;

Expand All @@ -37,6 +43,8 @@

final class KeyStoreUtils {

private final static Logger log = LogManager.getLogger(KeyStoreUtils.class);

private final static class SecuritySslContext extends SslContext {

private SecuritySslContext() {}
Expand Down Expand Up @@ -141,15 +149,10 @@
final var a = aliases.nextElement();
if (keyStore.isCertificateEntry(a)) {
final var c = (X509Certificate) keyStore.getCertificate(a);
if (c == null) {
throw new CertificateException("Alias " + a + " does not contain a certificate entry");
}
c.checkValidity();
} else if (keyStore.isKeyEntry(a)) {
final var cc = keyStore.getCertificateChain(a);
if (cc == null) {
throw new CertificateException("Alias " + a + " does not contain a certificate chain");
}
}
final var cc = keyStore.getCertificateChain(a);
if (cc != null) {
for (final var c : cc) {
((X509Certificate) c).checkValidity();
}
Expand All @@ -162,6 +165,48 @@
}
}

// If dnsToCheck is present, this method will only validate the validate for certificates that match the dns in this list or
// up the chain
public static void validateKeyStoreCertificates(final KeyStore keyStore, Set<X500Principal> dnsToCheck) {
try {
final var aliases = keyStore.aliases();
while (aliases.hasMoreElements()) {
final var a = aliases.nextElement();
if (keyStore.isCertificateEntry(a)) {
final var c = (X509Certificate) keyStore.getCertificate(a);
if (dnsToCheck.contains(c.getSubjectX500Principal())) {
c.checkValidity();
final var cc = keyStore.getCertificateChain(a);
if (cc != null) {
for (final var c1 : cc) {
((X509Certificate) c1).checkValidity();

Check warning on line 182 in src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java#L182

Added line #L182 was not covered by tests
}
}
} else {
log.info("Skipping validation for " + c.getSubjectX500Principal().getName());
}
} else {
if (keyStore.isCertificateEntry(a)) {
final var c = (X509Certificate) keyStore.getCertificate(a);
c.checkValidity();

Check warning on line 191 in src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java#L190-L191

Added lines #L190 - L191 were not covered by tests
}
final var cc = keyStore.getCertificateChain(a);

Check warning on line 193 in src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java#L193

Added line #L193 was not covered by tests
if (cc != null) {
if (Arrays.stream(cc).anyMatch(c -> dnsToCheck.contains(((X509Certificate) c).getSubjectX500Principal()))) {
for (final var c : cc) {
((X509Certificate) c).checkValidity();

Check warning on line 197 in src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java#L197

Added line #L197 was not covered by tests
}
}
}
}
}
} catch (KeyStoreException e) {
throw new OpenSearchException("Couldn't load keys store", e);
} catch (CertificateException e) {
throw new OpenSearchException("Invalid certificates", e);

Check warning on line 206 in src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java#L203-L206

Added lines #L203 - L206 were not covered by tests
}
}

public static KeyStore loadKeyStore(final Path path, final String type, final char[] password) {
try {
final var keyStore = KeyStore.getInstance(type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.net.ssl.TrustManagerFactory;
import javax.security.auth.x500.X500Principal;

import com.google.common.collect.ImmutableList;

Expand All @@ -46,7 +48,7 @@ public KeyStore createTrustStore() {
}

@Override
public TrustManagerFactory createTrustManagerFactory(boolean validateCertificates) {
public TrustManagerFactory createTrustManagerFactory(boolean validateCertificates, Set<X500Principal> issuerDns) {
return null;
}
};
Expand All @@ -55,10 +57,10 @@ public TrustManagerFactory createTrustManagerFactory(boolean validateCertificate

List<Certificate> loadCertificates();

default TrustManagerFactory createTrustManagerFactory(boolean validateCertificates) {
default TrustManagerFactory createTrustManagerFactory(boolean validateCertificates, Set<X500Principal> issuerDns) {
final var trustStore = createTrustStore();
if (validateCertificates) {
KeyStoreUtils.validateKeyStoreCertificates(trustStore);
KeyStoreUtils.validateKeyStoreCertificates(trustStore, issuerDns);
}
return buildTrustManagerFactory(trustStore);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

import java.nio.file.Path;
import java.util.List;
import java.util.Set;

import com.carrotsearch.randomizedtesting.RandomizedTest;
import org.junit.ClassRule;
Expand Down Expand Up @@ -49,7 +50,7 @@ void assertTrustStoreConfiguration(
assertThat("Truststore configuration created", nonNull(trustStoreConfiguration));
assertThat(trustStoreConfiguration.file(), is(expectedFile));
assertThat(trustStoreConfiguration.loadCertificates(), containsInAnyOrder(expectedCertificates));
assertThat(trustStoreConfiguration.createTrustManagerFactory(true), is(notNullValue()));
assertThat(trustStoreConfiguration.createTrustManagerFactory(true, Set.of()), is(notNullValue()));
}

void assertKeyStoreConfiguration(
Expand Down
Loading