From f036c7088b6ed4b9a0cb36b7fe114b70a75fafdf Mon Sep 17 00:00:00 2001 From: injectives <11927660+injectives@users.noreply.github.com> Date: Tue, 15 Feb 2022 15:31:23 +0200 Subject: [PATCH] Add support for multiple certificate files (#1153) * Add support for multiple certificate files This update brings support for specifying multiple certificate files via `TrustStrategy#trustCustomCertificateSignedBy(File...)`. Also, it deprecates `TrustStrategy#certFiles()` that is superseded by `TrustStrategy#certFiles()`. In addition, it adds support for the following Testkit feature flags: - `Feature:API:SSLConfig` - `Detail:DefaultSecurityConfigValueEquality` * Updated certFiles management --- driver/clirr-ignored-differences.xml | 7 +++ .../main/java/org/neo4j/driver/Config.java | 43 ++++++++++++----- .../driver/internal/SecuritySettings.java | 5 +- .../internal/security/SecurityPlanImpl.java | 16 ++++--- .../driver/internal/util/CertificateTool.java | 43 +++++++++-------- .../java/org/neo4j/driver/ConfigTest.java | 4 +- .../driver/util/CertificateUtilTest.java | 3 +- .../messages/requests/GetFeatures.java | 4 +- .../backend/messages/requests/NewDriver.java | 48 +++++++++++++++++-- .../backend/messages/requests/StartTest.java | 5 +- testkit/Dockerfile | 1 + 11 files changed, 128 insertions(+), 51 deletions(-) diff --git a/driver/clirr-ignored-differences.xml b/driver/clirr-ignored-differences.xml index eb8afa58f3..d20a91b513 100644 --- a/driver/clirr-ignored-differences.xml +++ b/driver/clirr-ignored-differences.xml @@ -55,4 +55,11 @@ java.lang.Iterable values() + + org/neo4j/driver/Config$TrustStrategy + 7005 + org.neo4j.driver.Config$TrustStrategy trustCustomCertificateSignedBy(java.io.File) + org.neo4j.driver.Config$TrustStrategy trustCustomCertificateSignedBy(java.io.File[]) + + diff --git a/driver/src/main/java/org/neo4j/driver/Config.java b/driver/src/main/java/org/neo4j/driver/Config.java index 4a5398225e..7032ad5336 100644 --- a/driver/src/main/java/org/neo4j/driver/Config.java +++ b/driver/src/main/java/org/neo4j/driver/Config.java @@ -21,6 +21,10 @@ import java.io.File; import java.io.Serializable; import java.net.InetAddress; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.Objects; import java.util.concurrent.TimeUnit; import java.util.logging.Level; @@ -469,7 +473,7 @@ public ConfigBuilder withoutEncryption() /** * Specify how to determine the authenticity of an encryption certificate provided by the Neo4j instance we are connecting to. This defaults to {@link - * TrustStrategy#trustSystemCertificates()}. See {@link TrustStrategy#trustCustomCertificateSignedBy(File)} for using certificate signatures instead to + * TrustStrategy#trustSystemCertificates()}. See {@link TrustStrategy#trustCustomCertificateSignedBy(File...)} for using certificate signatures instead to * verify trust. *

* This is an important setting to understand, because unless we know that the remote server we have an encrypted connection to is really Neo4j, there @@ -798,19 +802,20 @@ public enum Strategy } private final Strategy strategy; - private final File certFile; + private final List certFiles; private boolean hostnameVerificationEnabled = true; private RevocationStrategy revocationStrategy = RevocationStrategy.NO_CHECKS; private TrustStrategy( Strategy strategy ) { - this( strategy, null ); + this( strategy, Collections.emptyList() ); } - private TrustStrategy( Strategy strategy, File certFile ) + private TrustStrategy( Strategy strategy, List certFiles ) { + Objects.requireNonNull( certFiles, "certFiles can't be null" ); this.strategy = strategy; - this.certFile = certFile; + this.certFiles = Collections.unmodifiableList( new ArrayList<>( certFiles ) ); } /** @@ -827,10 +832,22 @@ public Strategy strategy() * Return the configured certificate file. * * @return configured certificate or {@code null} if trust strategy does not require a certificate. + * @deprecated superseded by {@link TrustStrategy#certFiles()} */ + @Deprecated public File certFile() { - return certFile; + return certFiles.isEmpty() ? null : certFiles.get( 0 ); + } + + /** + * Return the configured certificate files. + * + * @return configured certificate files or empty list if trust strategy does not require certificates. + */ + public List certFiles() + { + return certFiles; } /** @@ -866,18 +883,18 @@ public TrustStrategy withoutHostnameVerification() } /** - * Only encrypted connections to Neo4j instances with certificates signed by a trusted certificate will be accepted. - * The file specified should contain one or more trusted X.509 certificates. + * Only encrypted connections to Neo4j instances with certificates signed by a trusted certificate will be accepted. The file(s) specified should + * contain one or more trusted X.509 certificates. *

- * The certificate(s) in the file must be encoded using PEM encoding, meaning the certificates in the file should be encoded using Base64, - * and each certificate is bounded at the beginning by "-----BEGIN CERTIFICATE-----", and bounded at the end by "-----END CERTIFICATE-----". + * The certificate(s) in the file(s) must be encoded using PEM encoding, meaning the certificates in the file(s) should be encoded using Base64, and + * each certificate is bounded at the beginning by "-----BEGIN CERTIFICATE-----", and bounded at the end by "-----END CERTIFICATE-----". * - * @param certFile the trusted certificate file + * @param certFiles the trusted certificate files * @return an authentication config */ - public static TrustStrategy trustCustomCertificateSignedBy( File certFile ) + public static TrustStrategy trustCustomCertificateSignedBy( File... certFiles ) { - return new TrustStrategy( Strategy.TRUST_CUSTOM_CA_SIGNED_CERTIFICATES, certFile ); + return new TrustStrategy( Strategy.TRUST_CUSTOM_CA_SIGNED_CERTIFICATES, Arrays.asList( certFiles ) ); } /** diff --git a/driver/src/main/java/org/neo4j/driver/internal/SecuritySettings.java b/driver/src/main/java/org/neo4j/driver/internal/SecuritySettings.java index 0202f29a3a..5c61efc5a6 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/SecuritySettings.java +++ b/driver/src/main/java/org/neo4j/driver/internal/SecuritySettings.java @@ -21,7 +21,6 @@ import java.io.IOException; import java.io.Serializable; import java.security.GeneralSecurityException; -import java.util.Objects; import org.neo4j.driver.Config; import org.neo4j.driver.exceptions.ClientException; @@ -73,7 +72,7 @@ private boolean hasEqualTrustStrategy( SecuritySettings other ) } return t1.isHostnameVerificationEnabled() == t2.isHostnameVerificationEnabled() && t1.strategy() == t2.strategy() && - Objects.equals( t1.certFile(), t2.certFile() ) && t1.revocationStrategy() == t2.revocationStrategy(); + t1.certFiles().equals( t2.certFiles() ) && t1.revocationStrategy() == t2.revocationStrategy(); } public SecurityPlan createSecurityPlan( String uriScheme ) @@ -131,7 +130,7 @@ private static SecurityPlan createSecurityPlanImpl( boolean encrypted, Config.Tr switch ( trustStrategy.strategy() ) { case TRUST_CUSTOM_CA_SIGNED_CERTIFICATES: - return SecurityPlanImpl.forCustomCASignedCertificates( trustStrategy.certFile(), hostnameVerificationEnabled, revocationStrategy ); + return SecurityPlanImpl.forCustomCASignedCertificates( trustStrategy.certFiles(), hostnameVerificationEnabled, revocationStrategy ); case TRUST_SYSTEM_CA_SIGNED_CERTIFICATES: return SecurityPlanImpl.forSystemCASignedCertificates( hostnameVerificationEnabled, revocationStrategy ); case TRUST_ALL_CERTIFICATES: diff --git a/driver/src/main/java/org/neo4j/driver/internal/security/SecurityPlanImpl.java b/driver/src/main/java/org/neo4j/driver/internal/security/SecurityPlanImpl.java index 227ff4ef51..b394d7d2f5 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/security/SecurityPlanImpl.java +++ b/driver/src/main/java/org/neo4j/driver/internal/security/SecurityPlanImpl.java @@ -27,6 +27,8 @@ import java.security.cert.PKIXBuilderParameters; import java.security.cert.X509CertSelector; import java.security.cert.X509Certificate; +import java.util.Collections; +import java.util.List; import javax.net.ssl.CertPathTrustManagerParameters; import javax.net.ssl.KeyManager; import javax.net.ssl.SSLContext; @@ -53,31 +55,31 @@ public static SecurityPlan forAllCertificates( boolean requiresHostnameVerificat return new SecurityPlanImpl( true, sslContext, requiresHostnameVerification, revocationStrategy ); } - public static SecurityPlan forCustomCASignedCertificates( File certFile, boolean requiresHostnameVerification, + public static SecurityPlan forCustomCASignedCertificates( List certFiles, boolean requiresHostnameVerification, RevocationStrategy revocationStrategy ) throws GeneralSecurityException, IOException { - SSLContext sslContext = configureSSLContext( certFile, revocationStrategy ); + SSLContext sslContext = configureSSLContext( certFiles, revocationStrategy ); return new SecurityPlanImpl( true, sslContext, requiresHostnameVerification, revocationStrategy ); } public static SecurityPlan forSystemCASignedCertificates( boolean requiresHostnameVerification, RevocationStrategy revocationStrategy ) throws GeneralSecurityException, IOException { - SSLContext sslContext = configureSSLContext( null, revocationStrategy ); + SSLContext sslContext = configureSSLContext( Collections.emptyList(), revocationStrategy ); return new SecurityPlanImpl( true, sslContext, requiresHostnameVerification, revocationStrategy ); } - private static SSLContext configureSSLContext( File customCertFile, RevocationStrategy revocationStrategy ) + private static SSLContext configureSSLContext( List customCertFiles, RevocationStrategy revocationStrategy ) throws GeneralSecurityException, IOException { KeyStore trustedKeyStore = KeyStore.getInstance( KeyStore.getDefaultType() ); trustedKeyStore.load( null, null ); - if ( customCertFile != null ) + if ( !customCertFiles.isEmpty() ) { - // A certificate file is specified so we will load the certificates in the file - loadX509Cert( customCertFile, trustedKeyStore ); + // Certificate files are specified, so we will load the certificates in the file + loadX509Cert( customCertFiles, trustedKeyStore ); } else { diff --git a/driver/src/main/java/org/neo4j/driver/internal/util/CertificateTool.java b/driver/src/main/java/org/neo4j/driver/internal/util/CertificateTool.java index bb459b4825..9077dc8b78 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/util/CertificateTool.java +++ b/driver/src/main/java/org/neo4j/driver/internal/util/CertificateTool.java @@ -32,6 +32,7 @@ import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; import java.util.Base64; +import java.util.List; /** * A tool used to save, load certs, etc. @@ -106,35 +107,39 @@ public static void saveX509Cert( Certificate[] certs, File certFile ) throws Gen /** * Load the certificates written in X.509 format in a file to a key store. * - * @param certFile + * @param certFiles * @param keyStore * @throws GeneralSecurityException * @throws IOException */ - public static void loadX509Cert( File certFile, KeyStore keyStore ) throws GeneralSecurityException, IOException + public static void loadX509Cert( List certFiles, KeyStore keyStore ) throws GeneralSecurityException, IOException { - try ( BufferedInputStream inputStream = new BufferedInputStream( new FileInputStream( certFile ) ) ) + int certCount = 0; // The files might contain multiple certs + for ( File certFile : certFiles ) { - CertificateFactory certFactory = CertificateFactory.getInstance( "X.509" ); - - int certCount = 0; // The file might contain multiple certs - while ( inputStream.available() > 0 ) + try ( BufferedInputStream inputStream = new BufferedInputStream( new FileInputStream( certFile ) ) ) { - try - { - Certificate cert = certFactory.generateCertificate( inputStream ); - certCount++; - loadX509Cert( cert, "neo4j.javadriver.trustedcert." + certCount, keyStore ); - } - catch ( CertificateException e ) + CertificateFactory certFactory = CertificateFactory.getInstance( "X.509" ); + + while ( inputStream.available() > 0 ) { - if ( e.getCause() != null && e.getCause().getMessage().equals( "Empty input" ) ) + try + { + Certificate cert = certFactory.generateCertificate( inputStream ); + certCount++; + loadX509Cert( cert, "neo4j.javadriver.trustedcert." + certCount, keyStore ); + } + catch ( CertificateException e ) { - // This happens if there is whitespace at the end of the certificate - we load one cert, and then try and load a - // second cert, at which point we fail - return; + if ( e.getCause() != null && e.getCause().getMessage().equals( "Empty input" ) ) + { + // This happens if there is whitespace at the end of the certificate - we load one cert, and then try and load a + // second cert, at which point we fail + return; + } + throw new IOException( "Failed to load certificate from `" + certFile.getAbsolutePath() + "`: " + certCount + " : " + e.getMessage(), + e ); } - throw new IOException( "Failed to load certificate from `" + certFile.getAbsolutePath() + "`: " + certCount + " : " + e.getMessage(), e ); } } } diff --git a/driver/src/test/java/org/neo4j/driver/ConfigTest.java b/driver/src/test/java/org/neo4j/driver/ConfigTest.java index 98592ad0cb..a68f5da3b6 100644 --- a/driver/src/test/java/org/neo4j/driver/ConfigTest.java +++ b/driver/src/test/java/org/neo4j/driver/ConfigTest.java @@ -80,7 +80,7 @@ void shouldChangeToTrustedCert() // Then assertEquals( authConfig.strategy(), Config.TrustStrategy.Strategy.TRUST_CUSTOM_CA_SIGNED_CERTIFICATES ); - assertEquals( trustedCert.getAbsolutePath(), authConfig.certFile().getAbsolutePath() ); + assertEquals( trustedCert.getAbsolutePath(), authConfig.certFiles().get( 0 ).getAbsolutePath() ); } @Test @@ -401,7 +401,7 @@ void shouldSerialize() throws Exception assertEquals( config.eventLoopThreads(), verify.eventLoopThreads() ); assertEquals( config.encrypted(), verify.encrypted() ); assertEquals( config.trustStrategy().strategy(), verify.trustStrategy().strategy() ); - assertEquals( config.trustStrategy().certFile(), verify.trustStrategy().certFile() ); + assertEquals( config.trustStrategy().certFiles(), verify.trustStrategy().certFiles() ); assertEquals( config.trustStrategy().isHostnameVerificationEnabled(), verify.trustStrategy().isHostnameVerificationEnabled() ); assertEquals( config.trustStrategy().revocationStrategy(), verify.trustStrategy().revocationStrategy() ); assertEquals( config.userAgent(), verify.userAgent() ); diff --git a/driver/src/test/java/org/neo4j/driver/util/CertificateUtilTest.java b/driver/src/test/java/org/neo4j/driver/util/CertificateUtilTest.java index b23a3da1cc..5b50334be1 100644 --- a/driver/src/test/java/org/neo4j/driver/util/CertificateUtilTest.java +++ b/driver/src/test/java/org/neo4j/driver/util/CertificateUtilTest.java @@ -24,6 +24,7 @@ import java.security.KeyStore; import java.security.cert.Certificate; import java.security.cert.X509Certificate; +import java.util.Collections; import java.util.Enumeration; import org.neo4j.driver.internal.util.CertificateTool; @@ -52,7 +53,7 @@ void shouldLoadMultipleCertsIntoKeyStore() throws Throwable keyStore.load( null, null ); // When - CertificateTool.loadX509Cert( certFile, keyStore ); + CertificateTool.loadX509Cert( Collections.singletonList( certFile ), keyStore ); // Then Enumeration aliases = keyStore.aliases(); diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java index 5bddcafddd..9db58a8e83 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java @@ -62,7 +62,9 @@ public class GetFeatures implements TestkitRequest "Temporary:FullSummary", "Temporary:ResultKeys", "Temporary:TransactionClose", - "Optimization:EagerTransactionBegin" + "Optimization:EagerTransactionBegin", + "Feature:API:SSLConfig", + "Detail:DefaultSecurityConfigValueEquality" ) ); private static final Set SYNC_FEATURES = new HashSet<>( Arrays.asList( diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java index d0fec6e805..3257c04709 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java @@ -31,10 +31,14 @@ import neo4j.org.testkit.backend.messages.responses.TestkitResponse; import reactor.core.publisher.Mono; +import java.io.File; import java.net.InetAddress; import java.net.URI; import java.net.UnknownHostException; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.LinkedHashSet; +import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; @@ -117,7 +121,8 @@ public TestkitResponse process( TestkitState testkitState ) Config config = configBuilder.build(); try { - driver = driver( URI.create( data.uri ), authToken, config, retrySettings, domainNameResolver, testkitState, id ); + driver = driver( URI.create( data.uri ), authToken, config, retrySettings, domainNameResolver, configureSecuritySettingsBuilder(), testkitState, + id ); } catch ( RuntimeException e ) { @@ -223,11 +228,9 @@ private CompletionStage dispatchTestkitCallback( TestkitS } private org.neo4j.driver.Driver driver( URI uri, AuthToken authToken, Config config, RetrySettings retrySettings, DomainNameResolver domainNameResolver, - TestkitState testkitState, - String driverId ) + SecuritySettings.SecuritySettingsBuilder securitySettingsBuilder, TestkitState testkitState, String driverId ) { RoutingSettings routingSettings = RoutingSettings.DEFAULT; - SecuritySettings.SecuritySettingsBuilder securitySettingsBuilder = new SecuritySettings.SecuritySettingsBuilder(); SecuritySettings securitySettings = securitySettingsBuilder.build(); SecurityPlan securityPlan = securitySettings.createSecurityPlan( uri.getScheme() ); return new DriverFactoryWithDomainNameResolver( domainNameResolver, testkitState, driverId ) @@ -248,6 +251,41 @@ private Optional handleExceptionAsErrorResponse( TestkitState t return response; } + private SecuritySettings.SecuritySettingsBuilder configureSecuritySettingsBuilder() + { + SecuritySettings.SecuritySettingsBuilder securitySettingsBuilder = new SecuritySettings.SecuritySettingsBuilder(); + if ( data.encrypted ) + { + securitySettingsBuilder.withEncryption(); + } + else + { + securitySettingsBuilder.withoutEncryption(); + } + + if ( data.trustedCertificates != null ) + { + if ( !data.trustedCertificates.isEmpty() ) + { + File[] certs = data.trustedCertificates.stream() + .map( cert -> "/usr/local/share/custom-ca-certificates/" + cert ) + .map( Paths::get ) + .map( Path::toFile ) + .toArray( File[]::new ); + securitySettingsBuilder.withTrustStrategy( Config.TrustStrategy.trustCustomCertificateSignedBy( certs ) ); + } + else + { + securitySettingsBuilder.withTrustStrategy( Config.TrustStrategy.trustAllCertificates() ); + } + } + else + { + securitySettingsBuilder.withTrustStrategy( Config.TrustStrategy.trustSystemCertificates() ); + } + return securitySettingsBuilder; + } + @Setter @Getter public static class NewDriverBody @@ -263,6 +301,8 @@ public static class NewDriverBody private Long livenessCheckTimeoutMs; private Integer maxConnectionPoolSize; private Long connectionAcquisitionTimeoutMs; + private boolean encrypted; + private List trustedCertificates; } @RequiredArgsConstructor diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java index 01c29f0b4d..5946ba196a 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/StartTest.java @@ -53,13 +53,16 @@ public class StartTest implements TestkitRequest COMMON_SKIP_PATTERN_TO_REASON.put( "^.*\\.test_partial_summary_contains_system_updates$", "Does not contain updates because value is zero" ); COMMON_SKIP_PATTERN_TO_REASON.put( "^.*\\.test_partial_summary_contains_updates$", "Does not contain updates because value is zero" ); COMMON_SKIP_PATTERN_TO_REASON.put( "^.*\\.test_supports_multi_db$", "Database is None" ); + String skipMessage = "This test expects hostname verification to be turned off when all certificates are trusted"; + COMMON_SKIP_PATTERN_TO_REASON.put( "^.*\\.TestTrustAllCertsConfig\\.test_trusted_ca_wrong_hostname$", skipMessage ); + COMMON_SKIP_PATTERN_TO_REASON.put( "^.*\\.TestTrustAllCertsConfig\\.test_untrusted_ca_wrong_hostname$", skipMessage ); ASYNC_SKIP_PATTERN_TO_REASON.putAll( COMMON_SKIP_PATTERN_TO_REASON ); ASYNC_SKIP_PATTERN_TO_REASON.put( "^.*\\.test_should_reject_server_using_verify_connectivity_bolt_3x0$", "Does not error as expected" ); REACTIVE_SKIP_PATTERN_TO_REASON.putAll( COMMON_SKIP_PATTERN_TO_REASON ); // Current limitations (require further investigation or bug fixing) - String skipMessage = "Does not report RUN FAILURE"; + skipMessage = "Does not report RUN FAILURE"; REACTIVE_SKIP_PATTERN_TO_REASON.put( "^.*\\.Routing[^.]+\\.test_should_write_successfully_on_leader_switch_using_tx_function$", skipMessage ); REACTIVE_SKIP_PATTERN_TO_REASON.put( "^.*\\.TestDisconnects\\.test_disconnect_after_hello$", skipMessage ); REACTIVE_SKIP_PATTERN_TO_REASON.put( "^.*\\.TestDisconnects\\.test_disconnect_session_on_run$", skipMessage ); diff --git a/testkit/Dockerfile b/testkit/Dockerfile index 8d2de55dc8..4fbcb50e5a 100644 --- a/testkit/Dockerfile +++ b/testkit/Dockerfile @@ -13,4 +13,5 @@ ENV PATH=$JAVA_HOME/bin:$PATH # Assumes Linux Debian based image. # JAVA_HOME needed by update-ca-certificates hook to update Java with changed system CAs. COPY CAs/* /usr/local/share/ca-certificates/ +COPY CustomCAs/* /usr/local/share/custom-ca-certificates/ RUN update-ca-certificates