From 762665f0047f1f439db36250be018d31b0fcfc28 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Thu, 9 Dec 2021 16:39:37 +0100 Subject: [PATCH 1/2] fix: Check for equality of value instead of equality of instance. isCustomized returned the wrong value for all instances that have been serialized and read back as they obviously does not point to the default instance. The solution is to check for equality of the values, not for equality of the instances. This is a minor change in behaviour: A setting that is created with the builder matching the exact same defaults is now considered to be not customized. This solution has been preferred over a custom readResolve returning the DEFAULT when things match as it would otherwise be able to create an instance that identifies itself as customized but changed to not customized when read back, basically the oppositte of what is fixed here: An instances created as not customized must never be read back as customized. --- .../driver/internal/SecuritySettings.java | 16 ++- .../driver/internal/SecuritySettingsTest.java | 108 +++++++++++++++++- 2 files changed, 122 insertions(+), 2 deletions(-) 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 4a6245b2f8..0202f29a3a 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/SecuritySettings.java +++ b/driver/src/main/java/org/neo4j/driver/internal/SecuritySettings.java @@ -21,6 +21,7 @@ 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; @@ -59,7 +60,20 @@ public Config.TrustStrategy trustStrategy() private boolean isCustomized() { - return this != DEFAULT; + return !(DEFAULT.encrypted() == this.encrypted() && DEFAULT.hasEqualTrustStrategy( this )); + } + + private boolean hasEqualTrustStrategy( SecuritySettings other ) + { + Config.TrustStrategy t1 = this.trustStrategy; + Config.TrustStrategy t2 = other.trustStrategy; + if ( t1 == t2 ) + { + return true; + } + + return t1.isHostnameVerificationEnabled() == t2.isHostnameVerificationEnabled() && t1.strategy() == t2.strategy() && + Objects.equals( t1.certFile(), t2.certFile() ) && t1.revocationStrategy() == t2.revocationStrategy(); } public SecurityPlan createSecurityPlan( String uriScheme ) diff --git a/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java b/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java index ec946d2f2d..17b9f46730 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java @@ -18,21 +18,29 @@ */ package org.neo4j.driver.internal; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.platform.commons.support.ReflectionSupport; +import java.io.File; +import java.io.IOException; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.util.stream.Stream; import org.neo4j.driver.Config; import org.neo4j.driver.exceptions.ClientException; import org.neo4j.driver.internal.security.SecurityPlan; +import org.neo4j.driver.util.TestUtil; import static org.junit.jupiter.api.Assertions.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; -import static org.neo4j.driver.internal.RevocationStrategy.STRICT; import static org.neo4j.driver.internal.RevocationStrategy.NO_CHECKS; +import static org.neo4j.driver.internal.RevocationStrategy.STRICT; import static org.neo4j.driver.internal.RevocationStrategy.VERIFY_IF_PRESENT; class SecuritySettingsTest @@ -218,4 +226,102 @@ void testRevocationCheckingDisabledByDefault( String scheme ) assertEquals( NO_CHECKS, securityPlan.revocationStrategy() ); } + @Nested + class SerializationTests + { + Method isCustomized = ReflectionSupport.findMethod( SecuritySettings.class, "isCustomized" ).orElseThrow( + () -> new RuntimeException( "This test requires isCustomized to be present." ) ); + + boolean isCustomized( SecuritySettings securitySettings ) + { + isCustomized.setAccessible( true ); + try + { + return (boolean) isCustomized.invoke( securitySettings ); + } + catch ( IllegalAccessException | InvocationTargetException e ) + { + throw new RuntimeException( e ); + } + } + + @Test + void defaultSettingsShouldNotBeCustomizedWhenReadBack() throws IOException, ClassNotFoundException + { + SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder().build(); + + assertFalse( isCustomized( securitySettings ) ); + + SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class ); + + assertFalse( isCustomized( verify ) ); + } + + @Test + void defaultsShouldBeCheckCorrect() throws IOException, ClassNotFoundException + { + SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder().withoutEncryption().withTrustStrategy( + Config.TrustStrategy.trustSystemCertificates() ).build(); + + // The settings are still equivalent to the defaults, even if the builder has been used. It is not customized. + assertFalse( isCustomized( securitySettings ) ); + + SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class ); + + assertFalse( isCustomized( verify ) ); + } + + @Test + void shouldReadBackChangedEncryption() throws IOException, ClassNotFoundException + { + SecuritySettings securitySettings = + new SecuritySettings.SecuritySettingsBuilder().withEncryption().withTrustStrategy( Config.TrustStrategy.trustSystemCertificates() ).build(); + + assertTrue( isCustomized( securitySettings ) ); + assertTrue( securitySettings.encrypted() ); + + SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class ); + + assertTrue( isCustomized( verify ) ); + assertTrue( securitySettings.encrypted() ); + } + + @Test + void shouldReadBackChangedStrategey() throws IOException, ClassNotFoundException + { + SecuritySettings securitySettings = + new SecuritySettings.SecuritySettingsBuilder().withoutEncryption().withTrustStrategy( Config.TrustStrategy.trustAllCertificates() ).build(); + + // The settings are still equivalent to the defaults, even if the builder has been used. It is not customized. + assertTrue( isCustomized( securitySettings ) ); + assertFalse( securitySettings.encrypted() ); + assertEquals( Config.TrustStrategy.trustAllCertificates().strategy(), securitySettings.trustStrategy().strategy() ); + + SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class ); + + assertTrue( isCustomized( verify ) ); + assertFalse( securitySettings.encrypted() ); + assertEquals( Config.TrustStrategy.trustAllCertificates().strategy(), securitySettings.trustStrategy().strategy() ); + } + + @Test + void shouldReadBackChangedCertFile() throws IOException, ClassNotFoundException + { + SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder().withoutEncryption().withTrustStrategy( + Config.TrustStrategy.trustCustomCertificateSignedBy( new File( "some.cert" ) ) ).build(); + + // The settings are still equivalent to the defaults, even if the builder has been used. It is not customized. + assertTrue( isCustomized( securitySettings ) ); + assertFalse( securitySettings.encrypted() ); + assertEquals( Config.TrustStrategy.trustCustomCertificateSignedBy( new File( "some.cert" ) ).strategy(), + securitySettings.trustStrategy().strategy() ); + + SecuritySettings verify = TestUtil.serializeAndReadBack( securitySettings, SecuritySettings.class ); + + assertTrue( isCustomized( verify ) ); + assertFalse( securitySettings.encrypted() ); + assertEquals( Config.TrustStrategy.trustCustomCertificateSignedBy( new File( "some.cert" ) ).strategy(), + securitySettings.trustStrategy().strategy() ); + } + } } From 0cd6f003eaf5e2d18d30db4bb137f5f785764db5 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Thu, 9 Dec 2021 23:49:06 +0100 Subject: [PATCH 2/2] Adapt test. These tests needed to be adapted due to the fact that the previous test did not actually change the default value. --- .../java/org/neo4j/driver/internal/SecuritySettingsTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java b/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java index 17b9f46730..dce4b45d95 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/SecuritySettingsTest.java @@ -106,7 +106,7 @@ void testSelfSignedCertConfigDisablesHostnameVerification( String scheme ) throw void testThrowsOnUserCustomizedEncryption( String scheme ) { SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder() - .withoutEncryption() + .withEncryption() .build(); ClientException ex = @@ -121,7 +121,7 @@ void testThrowsOnUserCustomizedEncryption( String scheme ) void testThrowsOnUserCustomizedTrustConfiguration( String scheme ) { SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder() - .withTrustStrategy( Config.TrustStrategy.trustSystemCertificates() ) + .withTrustStrategy( Config.TrustStrategy.trustAllCertificates() ) .build(); ClientException ex =