Review feedback

Author: gjmwoods

driver/src/main/java/org/neo4j/driver/internal/security/SecurityPlanImpl.java

@@ -63,7 +63,7 @@ public static SecurityPlan forSystemCASignedCertificates( boolean requiresHostna
         return new SecurityPlanImpl( true, sslContext, requiresHostnameVerification, requiresRevocationChecking );
     }
 
-    public static SSLContext configureSSLContext( File customCertFile, boolean requiresRevocationChecking )
+    private static SSLContext configureSSLContext( File customCertFile, boolean requiresRevocationChecking )
             throws GeneralSecurityException, IOException
     {
         KeyStore trustedKeyStore = KeyStore.getInstance( KeyStore.getDefaultType() );
@@ -85,18 +85,16 @@ public static SSLContext configureSSLContext( File customCertFile, boolean requi
         // sets checking of stapled ocsp response
         pkixBuilderParameters.setRevocationEnabled( requiresRevocationChecking );
 
-        // enables status_request exentension in client hello
+        // enables status_request extension in client hello
         if ( requiresRevocationChecking )
         {
             System.setProperty( "jdk.tls.client.enableStatusRequestExtension", "true" );
         }
 
-        // Create TrustManager from TrustedKeyStore
-        TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance( TrustManagerFactory.getDefaultAlgorithm() );
-        trustManagerFactory.init( new CertPathTrustManagerParameters( pkixBuilderParameters ) );
-
         SSLContext sslContext = SSLContext.getInstance( "TLS" );
 
+        TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance( TrustManagerFactory.getDefaultAlgorithm() );
+        trustManagerFactory.init( new CertPathTrustManagerParameters( pkixBuilderParameters ) );
         sslContext.init( new KeyManager[0], trustManagerFactory.getTrustManagers(), null );
 
         return sslContext;

driver/src/main/java/org/neo4j/driver/internal/util/CertificateTool.java

@@ -36,7 +36,7 @@
 /**
  * A tool used to save, load certs, etc.
  */
-public class CertificateTool
+public final class CertificateTool
 {
     private static final String BEGIN_CERT = "-----BEGIN CERTIFICATE-----";
     private static final String END_CERT = "-----END CERTIFICATE-----";
@@ -170,6 +170,10 @@ public static String X509CertToString( String cert )
         String cert64CharPerLine = cert.replaceAll( "(.{64})", "$1\n" );
         return BEGIN_CERT + "\n" + cert64CharPerLine + "\n"+ END_CERT + "\n";
     }
+
+    private CertificateTool()
+    {
+    }
 }
 
 

driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java

@@ -23,13 +23,11 @@
 import org.junit.jupiter.params.provider.MethodSource;
 
 import java.util.stream.Stream;
-import javax.net.ssl.SSLContext;
 
 import org.neo4j.driver.Config;
 import org.neo4j.driver.exceptions.ClientException;
 import org.neo4j.driver.internal.security.SecurityPlan;
 
-import static org.junit.Assert.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -75,11 +73,9 @@ void testSystemCertCompatibleConfiguration( String scheme ) throws Exception
 
         SecurityPlan securityPlan = securitySettings.createSecurityPlan( scheme );
 
-        SSLContext defaultContext = SSLContext.getDefault();
-
         assertTrue( securityPlan.requiresEncryption() );
         assertTrue( securityPlan.requiresHostnameVerification() );
-        assertEquals( defaultContext, securityPlan.sslContext() );
+        assertFalse( securityPlan.requiresRevocationChecking() );
     }
 
     @ParameterizedTest
@@ -140,7 +136,7 @@ void testThrowsOnUserCustomizedTrustConfigurationAndEncryption( String scheme )
         assertTrue( ex.getMessage().contains( String.format( "Scheme %s is not configurable with manual encryption and trust settings", scheme ) ));
     }
 
-    @ParameterizedTest()
+    @ParameterizedTest
     @MethodSource( "unencryptedSchemes" )
     void testNoEncryption( String scheme )
     {
@@ -151,7 +147,7 @@ void testNoEncryption( String scheme )
         assertFalse( securityPlan.requiresEncryption() );
     }
 
-    @ParameterizedTest()
+    @ParameterizedTest
     @MethodSource( "unencryptedSchemes" )
     void testConfiguredEncryption( String scheme )
     {
@@ -163,7 +159,7 @@ void testConfiguredEncryption( String scheme )
         assertTrue( securityPlan.requiresEncryption() );
     }
 
-    @ParameterizedTest()
+    @ParameterizedTest
     @MethodSource( "unencryptedSchemes" )
     void testConfiguredAllCertificates( String scheme)
     {
@@ -177,7 +173,7 @@ void testConfiguredAllCertificates( String scheme)
         assertTrue( securityPlan.requiresEncryption() );
     }
 
-    @ParameterizedTest()
+    @ParameterizedTest
     @MethodSource( "unencryptedSchemes" )
     void testConfigureRevocationChecking( String scheme )
     {
@@ -191,7 +187,7 @@ void testConfigureRevocationChecking( String scheme )
         assertTrue( securityPlan.requiresRevocationChecking() );
     }
 
-    @ParameterizedTest()
+    @ParameterizedTest
     @MethodSource( "unencryptedSchemes" )
     void testRevocationCheckingDisabledByDefault( String scheme )
     {