From 7cc225bbf755a1240e1303cee63ec526d9b7e297 Mon Sep 17 00:00:00 2001 From: Andrey Pleskach Date: Tue, 22 Oct 2024 21:20:05 +0200 Subject: [PATCH] Add validation of authority certificates Added validation of authority certificates prior to reloading the SSL context. Signed-off-by: Andrey Pleskach --- .../security/ssl/SslContextHandler.java | 100 ++++++++++++------ .../security/ssl/CertificatesRule.java | 21 ++-- .../security/ssl/SslContextHandlerTest.java | 35 +++++- 3 files changed, 112 insertions(+), 44 deletions(-) diff --git a/src/main/java/org/opensearch/security/ssl/SslContextHandler.java b/src/main/java/org/opensearch/security/ssl/SslContextHandler.java index 9fda1641af..97cd0e7147 100644 --- a/src/main/java/org/opensearch/security/ssl/SslContextHandler.java +++ b/src/main/java/org/opensearch/security/ssl/SslContextHandler.java @@ -26,6 +26,8 @@ import io.netty.handler.ssl.SslContext; +import static java.util.function.Predicate.not; + public class SslContextHandler { private SslContext sslContext; @@ -68,40 +70,69 @@ Stream keyMaterialCertificates(final List certificates return certificates.stream().filter(Certificate::hasKey); } + Stream authorityCertificates() { + return authorityCertificates(loadedCertificates); + } + + Stream authorityCertificates(final List certificates) { + return certificates.stream().filter(not(Certificate::hasKey)); + } + void reloadSslContext() throws CertificateException { final var newCertificates = sslConfiguration.certificates(); - if (sameCertificates(newCertificates)) { - return; + boolean hasChanges = false; + + final var loadedAuthorityCertificates = authorityCertificates().collect(Collectors.toList()); + final var loadedKeyMaterialCertificates = keyMaterialCertificates().collect(Collectors.toList()); + final var newAuthorityCertificates = authorityCertificates(newCertificates).collect(Collectors.toList()); + final var newKeyMaterialCertificates = keyMaterialCertificates(newCertificates).collect(Collectors.toList()); + + if (notSameCertificates(loadedAuthorityCertificates, newAuthorityCertificates)) { + hasChanges = true; + validateDates(newAuthorityCertificates); } - validateNewCertificates(newCertificates); - invalidateSessions(); - if (sslContext.isClient()) { - sslContext = sslConfiguration.buildClientSslContext(false); - } else { - sslContext = sslConfiguration.buildServerSslContext(false); + + if (notSameCertificates(loadedKeyMaterialCertificates, newKeyMaterialCertificates)) { + hasChanges = true; + validateNewKeyMaterialCertificates(loadedKeyMaterialCertificates, newKeyMaterialCertificates); + } + if (hasChanges) { + invalidateSessions(); + if (sslContext.isClient()) { + sslContext = sslConfiguration.buildClientSslContext(false); + } else { + sslContext = sslConfiguration.buildServerSslContext(false); + } + loadedCertificates.clear(); + loadedCertificates.addAll(newCertificates); } - loadedCertificates.clear(); - loadedCertificates.addAll(newCertificates); } - private boolean sameCertificates(final List newCertificates) { - final Set currentCertSignatureSet = keyMaterialCertificates().map(Certificate::x509Certificate) + private boolean notSameCertificates(final List loadedCertificates, final List newCertificates) { + final Set currentCertSignatureSet = loadedCertificates.stream() + .map(Certificate::x509Certificate) .map(X509Certificate::getSignature) .map(s -> new String(s, StandardCharsets.UTF_8)) .collect(Collectors.toSet()); - final Set newCertSignatureSet = keyMaterialCertificates(newCertificates).map(Certificate::x509Certificate) + final Set newCertSignatureSet = newCertificates.stream() + .map(Certificate::x509Certificate) .map(X509Certificate::getSignature) .map(s -> new String(s, StandardCharsets.UTF_8)) .collect(Collectors.toSet()); - return currentCertSignatureSet.equals(newCertSignatureSet); + return !currentCertSignatureSet.equals(newCertSignatureSet); } - private void validateSubjectDns(final List newCertificates) throws CertificateException { - final List currentSubjectDNs = keyMaterialCertificates().map(Certificate::subject).sorted().collect(Collectors.toList()); - final List newSubjectDNs = keyMaterialCertificates(newCertificates).map(Certificate::subject) - .sorted() - .collect(Collectors.toList()); + private void validateDates(final List newCertificates) throws CertificateException { + for (final var certificate : newCertificates) { + certificate.x509Certificate().checkValidity(); + } + } + + private void validateSubjectDns(final List loadedCertificates, final List newCertificates) + throws CertificateException { + final List currentSubjectDNs = loadedCertificates.stream().map(Certificate::subject).sorted().collect(Collectors.toList()); + final List newSubjectDNs = newCertificates.stream().map(Certificate::subject).sorted().collect(Collectors.toList()); if (!currentSubjectDNs.equals(newSubjectDNs)) { throw new CertificateException( "New certificates do not have valid Subject DNs. Current Subject DNs " @@ -112,11 +143,10 @@ private void validateSubjectDns(final List newCertificates) throws } } - private void validateIssuerDns(final List newCertificates) throws CertificateException { - final List currentIssuerDNs = keyMaterialCertificates().map(Certificate::issuer).sorted().collect(Collectors.toList()); - final List newIssuerDNs = keyMaterialCertificates(newCertificates).map(Certificate::issuer) - .sorted() - .collect(Collectors.toList()); + private void validateIssuerDns(final List loadedCertificates, final List newCertificates) + throws CertificateException { + final List currentIssuerDNs = loadedCertificates.stream().map(Certificate::issuer).sorted().collect(Collectors.toList()); + final List newIssuerDNs = newCertificates.stream().map(Certificate::issuer).sorted().collect(Collectors.toList()); if (!currentIssuerDNs.equals(newIssuerDNs)) { throw new CertificateException( "New certificates do not have valid Issuer DNs. Current Issuer DNs: " @@ -127,11 +157,14 @@ private void validateIssuerDns(final List newCertificates) throws C } } - private void validateSans(final List newCertificates) throws CertificateException { - final List currentSans = keyMaterialCertificates().map(Certificate::subjectAlternativeNames) + private void validateSans(final List loadedCertificates, final List newCertificates) + throws CertificateException { + final List currentSans = loadedCertificates.stream() + .map(Certificate::subjectAlternativeNames) .sorted() .collect(Collectors.toList()); - final List newSans = keyMaterialCertificates(newCertificates).map(Certificate::subjectAlternativeNames) + final List newSans = newCertificates.stream() + .map(Certificate::subjectAlternativeNames) .sorted() .collect(Collectors.toList()); if (!currentSans.equals(newSans)) { @@ -141,13 +174,12 @@ private void validateSans(final List newCertificates) throws Certif } } - private void validateNewCertificates(final List newCertificates) throws CertificateException { - for (final var certificate : newCertificates) { - certificate.x509Certificate().checkValidity(); - } - validateSubjectDns(newCertificates); - validateIssuerDns(newCertificates); - validateSans(newCertificates); + private void validateNewKeyMaterialCertificates(final List loadedCertificates, final List newCertificates) + throws CertificateException { + validateDates(newCertificates); + validateSubjectDns(loadedCertificates, newCertificates); + validateIssuerDns(loadedCertificates, newCertificates); + validateSans(loadedCertificates, newCertificates); } private void invalidateSessions() { diff --git a/src/test/java/org/opensearch/security/ssl/CertificatesRule.java b/src/test/java/org/opensearch/security/ssl/CertificatesRule.java index a27de233dc..b5a397d7c8 100644 --- a/src/test/java/org/opensearch/security/ssl/CertificatesRule.java +++ b/src/test/java/org/opensearch/security/ssl/CertificatesRule.java @@ -123,12 +123,21 @@ public KeyPair generateKeyPair() throws NoSuchAlgorithmException { public X509CertificateHolder generateCaCertificate(final KeyPair parentKeyPair) throws IOException, NoSuchAlgorithmException, OperatorCreationException { - return generateCaCertificate(parentKeyPair, generateSerialNumber()); + final var startAndEndDate = generateStartAndEndDate(); + return generateCaCertificate(parentKeyPair, generateSerialNumber(), startAndEndDate.v1(), startAndEndDate.v2()); } - public X509CertificateHolder generateCaCertificate(final KeyPair parentKeyPair, final BigInteger serialNumber) throws IOException, - NoSuchAlgorithmException, OperatorCreationException { - final var startAndEndDate = generateStartAndEndDate(); + public X509CertificateHolder generateCaCertificate(final KeyPair parentKeyPair, final Instant startDate, final Instant endDate) + throws IOException, NoSuchAlgorithmException, OperatorCreationException { + return generateCaCertificate(parentKeyPair, generateSerialNumber(), startDate, endDate); + } + + public X509CertificateHolder generateCaCertificate( + final KeyPair parentKeyPair, + final BigInteger serialNumber, + final Instant startDate, + final Instant endDate + ) throws IOException, NoSuchAlgorithmException, OperatorCreationException { // CS-SUPPRESS-SINGLE: RegexpSingleline Extension should only be used sparingly to keep implementations as generic as possible return createCertificateBuilder( DEFAULT_SUBJECT_NAME, @@ -136,8 +145,8 @@ public X509CertificateHolder generateCaCertificate(final KeyPair parentKeyPair, parentKeyPair.getPublic(), parentKeyPair.getPublic(), serialNumber, - startAndEndDate.v1(), - startAndEndDate.v2() + startDate, + endDate ).addExtension(Extension.basicConstraints, true, new BasicConstraints(true)) .addExtension(Extension.keyUsage, true, new KeyUsage(KeyUsage.digitalSignature | KeyUsage.keyCertSign | KeyUsage.cRLSign)) .build(new JcaContentSignerBuilder("SHA256withRSA").setProvider(BOUNCY_CASTLE_PROVIDER).build(parentKeyPair.getPrivate())); diff --git a/src/test/java/org/opensearch/security/ssl/SslContextHandlerTest.java b/src/test/java/org/opensearch/security/ssl/SslContextHandlerTest.java index 4dea300754..916b7b09a7 100644 --- a/src/test/java/org/opensearch/security/ssl/SslContextHandlerTest.java +++ b/src/test/java/org/opensearch/security/ssl/SslContextHandlerTest.java @@ -84,7 +84,34 @@ public void doesNothingIfCertificatesAreSame() throws Exception { } @Test - public void failsIfCertificatesHasInvalidDates() throws Exception { + public void failsIfAuthorityCertificateHasInvalidDates() throws Exception { + final var sslContextHandler = sslContextHandler(); + final var keyPair = certificatesRule.generateKeyPair(); + + final var caCertificate = certificatesRule.caCertificateHolder(); + + var newCaCertificate = certificatesRule.generateCaCertificate( + keyPair, + caCertificate.getNotAfter().toInstant(), + caCertificate.getNotAfter().toInstant().minus(10, ChronoUnit.DAYS) + ); + + writeCertificates(newCaCertificate, certificatesRule.accessCertificateHolder(), certificatesRule.accessCertificatePrivateKey()); + + assertThrows(CertificateException.class, sslContextHandler::reloadSslContext); + + newCaCertificate = certificatesRule.generateCaCertificate( + keyPair, + caCertificate.getNotBefore().toInstant().plus(10, ChronoUnit.DAYS), + caCertificate.getNotAfter().toInstant().plus(20, ChronoUnit.DAYS) + ); + writeCertificates(newCaCertificate, certificatesRule.accessCertificateHolder(), certificatesRule.accessCertificatePrivateKey()); + + assertThrows(CertificateException.class, sslContextHandler::reloadSslContext); + } + + @Test + public void failsIfKeyMaterialCertificateHasInvalidDates() throws Exception { final var sslContextHandler = sslContextHandler(); final var accessCertificate = certificatesRule.x509AccessCertificate(); @@ -111,7 +138,7 @@ public void failsIfCertificatesHasInvalidDates() throws Exception { } @Test - public void filesIfHasNotValidSubjectDNs() throws Exception { + public void failsIfKeyMaterialCertificateHasNotValidSubjectDNs() throws Exception { final var sslContextHandler = sslContextHandler(); final var keyPair = certificatesRule.generateKeyPair(); @@ -137,7 +164,7 @@ public void filesIfHasNotValidSubjectDNs() throws Exception { } @Test - public void filesIfHasNotValidIssuerDNs() throws Exception { + public void failsIfKeyMaterialCertificateHasNotValidIssuerDNs() throws Exception { final var sslContextHandler = sslContextHandler(); final var keyPair = certificatesRule.generateKeyPair(); @@ -163,7 +190,7 @@ public void filesIfHasNotValidIssuerDNs() throws Exception { } @Test - public void filesIfHasNotValidSans() throws Exception { + public void failsIfKeyMaterialCertificateHasNotValidSans() throws Exception { final var sslContextHandler = sslContextHandler(); final var keyPair = certificatesRule.generateKeyPair();