From aa333452e1e25953e01316c06fa2929e89c90689 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Thu, 9 Dec 2021 16:39:37 +0100 Subject: [PATCH] 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. A couple of tests needed to be adapted due to the fact that the previous test did not actually change the default value. --- .../driver/internal/SecuritySettings.java | 16 ++- .../driver/internal/SecuritySettingsTest.java | 112 +++++++++++++++++- 2 files changed, 124 insertions(+), 4 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..dce4b45d95 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 @@ -98,7 +106,7 @@ void testSelfSignedCertConfigDisablesHostnameVerification( String scheme ) throw void testThrowsOnUserCustomizedEncryption( String scheme ) { SecuritySettings securitySettings = new SecuritySettings.SecuritySettingsBuilder() - .withoutEncryption() + .withEncryption() .build(); ClientException ex = @@ -113,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 = @@ -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() ); + } + } }