From 7ff537d1c910280cfcea34b6cccebce4a9d52288 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 27 Mar 2018 17:35:28 +0200 Subject: [PATCH] Obsfucate the LDAP system password in toString and debug.log --- .../neo4j/configuration/ConfigOptions.java | 2 +- .../org/neo4j/configuration/ConfigValue.java | 9 +++++- .../neo4j/configuration/LoadableConfig.java | 3 ++ .../neo4j/configuration/ConfigValueTest.java | 13 +++----- .../org/neo4j/graphdb/config/BaseSetting.java | 12 +++++++ .../neo4j/graphdb/config/SettingGroup.java | 5 +++ .../neo4j/kernel/configuration/Config.java | 21 +++++++++++-- .../configuration/ConnectorValidator.java | 6 ++++ .../neo4j/kernel/configuration/Settings.java | 6 ++++ .../ssl/SslPolicyConfigValidator.java | 6 ++++ .../kernel/configuration/ConfigTest.java | 31 ++++++++++++++++--- .../configuration/SecuritySettings.java | 2 ++ .../auth/integration/bolt/LdapAuthIT.java | 22 +++++++++++++ 13 files changed, 122 insertions(+), 16 deletions(-) diff --git a/community/configuration/src/main/java/org/neo4j/configuration/ConfigOptions.java b/community/configuration/src/main/java/org/neo4j/configuration/ConfigOptions.java index e939ff22ced34..f7c206628812b 100644 --- a/community/configuration/src/main/java/org/neo4j/configuration/ConfigOptions.java +++ b/community/configuration/src/main/java/org/neo4j/configuration/ConfigOptions.java @@ -61,7 +61,7 @@ public List asConfigValues( @Nonnull Map validConfig setting.documentedDefaultValue(), Optional.ofNullable( val.getValue() ), setting.valueDescription(), setting.internal(), setting.dynamic(), - setting.deprecated(), setting.replacement() ); + setting.deprecated(), setting.replacement(), setting.isSecret() ); } ) .collect( Collectors.toList() ); } diff --git a/community/configuration/src/main/java/org/neo4j/configuration/ConfigValue.java b/community/configuration/src/main/java/org/neo4j/configuration/ConfigValue.java index 533e9af519450..1a60ad71acd9c 100644 --- a/community/configuration/src/main/java/org/neo4j/configuration/ConfigValue.java +++ b/community/configuration/src/main/java/org/neo4j/configuration/ConfigValue.java @@ -34,6 +34,7 @@ public class ConfigValue private final Optional value; private final String valueDescription; private final boolean internal; + private final boolean isSecret; private final boolean dynamic; private final boolean deprecated; private final Optional replacement; @@ -41,7 +42,7 @@ public class ConfigValue public ConfigValue( @Nonnull String name, @Nonnull Optional description, @Nonnull Optional documentedDefaultValue, @Nonnull Optional value, @Nonnull String valueDescription, boolean internal, boolean dynamic, boolean deprecated, - @Nonnull Optional replacement ) + @Nonnull Optional replacement, boolean isSecret ) { this.name = name; this.description = description; @@ -49,6 +50,7 @@ public ConfigValue( @Nonnull String name, @Nonnull Optional description, this.value = value; this.valueDescription = valueDescription; this.internal = internal; + this.isSecret = isSecret; this.dynamic = dynamic; this.deprecated = deprecated; this.replacement = replacement; @@ -100,6 +102,11 @@ public boolean internal() return internal; } + public boolean isSecret() + { + return isSecret; + } + public boolean dynamic() { return dynamic; diff --git a/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java b/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java index 20a7c41e61f3d..11c0b5f5ce9a7 100644 --- a/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java +++ b/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java @@ -77,6 +77,9 @@ default List getConfigOptions() final Internal internalAnnotation = f.getAnnotation( Internal.class ); setting.setInternal( internalAnnotation != null ); + final Secret secretAnnotation = f.getAnnotation( Secret.class ); + setting.makeSecret( secretAnnotation != null ); + final Dynamic dynamicAnnotation = f.getAnnotation( Dynamic.class ); setting.setDynamic( dynamicAnnotation != null ); } diff --git a/community/configuration/src/test/java/org/neo4j/configuration/ConfigValueTest.java b/community/configuration/src/test/java/org/neo4j/configuration/ConfigValueTest.java index 5958d8b3dec29..fc678b197ec66 100644 --- a/community/configuration/src/test/java/org/neo4j/configuration/ConfigValueTest.java +++ b/community/configuration/src/test/java/org/neo4j/configuration/ConfigValueTest.java @@ -35,7 +35,7 @@ public class ConfigValueTest public void handlesEmptyValue() { ConfigValue value = new ConfigValue( "name", Optional.empty(), Optional.empty(), Optional.empty(), - "description", false, false, false, Optional.empty() ); + "description", false, false, false, Optional.empty(), false ); assertEquals( Optional.empty(), value.value() ); assertEquals( "null", value.toString() ); @@ -48,8 +48,7 @@ public void handlesEmptyValue() public void handlesInternal() { ConfigValue value = new ConfigValue( "name", Optional.empty(), Optional.empty(), Optional.empty(), - "description", true, false, false, - Optional.empty() ); + "description", true, false, false, Optional.empty(), false ); assertTrue( value.internal() ); } @@ -58,7 +57,7 @@ public void handlesInternal() public void handlesNonEmptyValue() { ConfigValue value = new ConfigValue( "name", Optional.empty(), Optional.empty(), Optional.of( 1 ), - "description", false, false, false, Optional.empty() ); + "description", false, false, false, Optional.empty(), false ); assertEquals( Optional.of( 1 ), value.value() ); assertEquals( "1", value.toString() ); @@ -71,8 +70,7 @@ public void handlesNonEmptyValue() public void handlesDeprecationAndReplacement() { ConfigValue value = new ConfigValue( "old_name", Optional.empty(), Optional.empty(), Optional.of( 1 ), - "description", false, false, true, - Optional.of( "new_name" ) ); + "description", false, false, true, Optional.of( "new_name" ), false ); assertEquals( Optional.of( 1 ), value.value() ); assertEquals( "1", value.toString() ); @@ -85,8 +83,7 @@ public void handlesDeprecationAndReplacement() public void handlesValueDescription() { ConfigValue value = new ConfigValue( "old_name", Optional.empty(), Optional.empty(), Optional.of( 1 ), - "a simple integer", false, false, true, - Optional.of( "new_name" ) ); + "a simple integer", false, false, true, Optional.of( "new_name" ), false ); assertEquals( Optional.of( 1 ), value.value() ); assertEquals( "1", value.toString() ); diff --git a/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/BaseSetting.java b/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/BaseSetting.java index b9deeaf2e3be7..035965e15338b 100644 --- a/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/BaseSetting.java +++ b/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/BaseSetting.java @@ -31,6 +31,7 @@ public abstract class BaseSetting implements Setting private boolean deprecated; private String replacement; private boolean internal; + private boolean isSecret; private boolean dynamic; private String documentedDefaultValue; private String description; @@ -68,6 +69,17 @@ public void setInternal( final boolean val ) this.internal = val; } + @Override + public boolean isSecret() + { + return this.isSecret; + } + + public void makeSecret( final boolean val ) + { + this.isSecret = val; + } + @Override public Optional documentedDefaultValue() { diff --git a/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/SettingGroup.java b/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/SettingGroup.java index e7c329104b64b..9147f542f5943 100644 --- a/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/SettingGroup.java +++ b/community/graphdb-api/src/main/java/org/neo4j/graphdb/config/SettingGroup.java @@ -62,6 +62,11 @@ public interface SettingGroup extends SettingValidator */ boolean internal(); + /** + * @return {@code true} if secret setting (should be hidden), false otherwise. + */ + boolean isSecret(); + /** * @return the documented default value if it needs special documentation, empty if default value is good as is. */ diff --git a/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java b/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java index 9f226474de42d..8ef9ed1ed8a84 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/configuration/Config.java @@ -26,6 +26,7 @@ import java.util.Collection; import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -44,10 +45,12 @@ import org.neo4j.configuration.ConfigOptions; import org.neo4j.configuration.ConfigValue; import org.neo4j.configuration.LoadableConfig; +import org.neo4j.configuration.Secret; import org.neo4j.graphdb.config.BaseSetting; import org.neo4j.graphdb.config.Configuration; import org.neo4j.graphdb.config.InvalidSettingException; import org.neo4j.graphdb.config.Setting; +import org.neo4j.graphdb.config.SettingGroup; import org.neo4j.graphdb.config.SettingValidator; import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.helpers.collection.MapUtil; @@ -82,6 +85,7 @@ public class Config implements DiagnosticsProvider, Configuration private final List configOptions; private final Map params = new CopyOnWriteHashMap<>(); // Read heavy workload + private final Set secrets = new HashSet<>(); private final Map>> updateListeners = new ConcurrentHashMap<>(); private final ConfigurationMigrator migrator; private final List validators = new ArrayList<>(); @@ -390,6 +394,14 @@ private Config( File configFile, .map( BaseSetting.class::cast ) .forEach( setting -> settingsMap.put( setting.name(), setting ) ); + // Find secret settings + configOptions.stream() + .map( ConfigOptions::settingGroup ) + .filter( SettingGroup::isSecret ) + .filter( BaseSetting.class::isInstance ) + .map( BaseSetting.class::cast ) + .forEach( setting -> secrets.add( setting.name() ) ); + validators.addAll( additionalValidators ); migrator = new AnnotationBasedConfigurationMigrator( settingsClasses ); this.overriddenDefaults.putAll( overriddenDefaults ); @@ -742,11 +754,16 @@ public void dump( DiagnosticsPhase phase, Logger logger ) logger.log( "Neo4j Kernel properties:" ); for ( Map.Entry param : params.entrySet() ) { - logger.log( "%s=%s", param.getKey(), param.getValue() ); + logger.log( "%s=%s", param.getKey(), obsfucateIfSecret( param ) ); } } } + private String obsfucateIfSecret( Map.Entry param ) + { + return secrets.contains( param.getKey() ) ? Secret.OBSFUCATED : param.getValue(); + } + /** * Migrates and validates all string values in the provided settings map. * @@ -943,7 +960,7 @@ public String toString() { return params.entrySet().stream() .sorted( Comparator.comparing( Map.Entry::getKey ) ) - .map( entry -> entry.getKey() + "=" + entry.getValue() ) + .map( entry -> entry.getKey() + "=" + obsfucateIfSecret( entry ) ) .collect( Collectors.joining( ", ") ); } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/configuration/ConnectorValidator.java b/community/kernel/src/main/java/org/neo4j/kernel/configuration/ConnectorValidator.java index 183181118fb0d..6f723f3879313 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/configuration/ConnectorValidator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/configuration/ConnectorValidator.java @@ -230,6 +230,12 @@ public boolean internal() return false; } + @Override + public boolean isSecret() + { + return false; + } + @Override public Optional documentedDefaultValue() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/configuration/Settings.java b/community/kernel/src/main/java/org/neo4j/kernel/configuration/Settings.java index 27ac16700c92d..1288d3b1bbd2b 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/configuration/Settings.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/configuration/Settings.java @@ -1187,6 +1187,12 @@ public boolean internal() return newSetting.internal(); } + @Override + public boolean isSecret() + { + return newSetting.isSecret(); + } + @Override public Optional documentedDefaultValue() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/configuration/ssl/SslPolicyConfigValidator.java b/community/kernel/src/main/java/org/neo4j/kernel/configuration/ssl/SslPolicyConfigValidator.java index 83247c71e7d5d..9905565106d9d 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/configuration/ssl/SslPolicyConfigValidator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/configuration/ssl/SslPolicyConfigValidator.java @@ -157,6 +157,12 @@ public boolean internal() return false; } + @Override + public boolean isSecret() + { + return false; + } + @Override public Optional documentedDefaultValue() { diff --git a/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java b/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java index c98012d0bc9c6..2a5c820081ade 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java @@ -39,6 +39,7 @@ import org.neo4j.configuration.Internal; import org.neo4j.configuration.LoadableConfig; import org.neo4j.configuration.ReplacedBy; +import org.neo4j.configuration.Secret; import org.neo4j.graphdb.config.InvalidSettingException; import org.neo4j.graphdb.config.Setting; import org.neo4j.graphdb.factory.GraphDatabaseSettings; @@ -103,6 +104,9 @@ public static class MySettingsWithDefaults implements LoadableConfig @Deprecated public static final Setting oldSetting = setting( "some_setting", STRING, "Has no replacement" ); + + @Secret + public static final Setting password = setting( "password", STRING, "This text should not appear in logs or toString" ); } private static class HelloHasToBeNeo4jConfigurationValidator implements ConfigurationValidator @@ -250,16 +254,35 @@ public void shouldSetInternalParameter() { // Given Config config = Config.builder() - .withSetting( MySettingsWithDefaults.secretSetting, "false" ) - .withSetting( MySettingsWithDefaults.hello, "ABC" ) - .withConfigClasses( Arrays.asList( mySettingsWithDefaults, myMigratingSettings ) ) - .build(); + .withSetting( MySettingsWithDefaults.secretSetting, "false" ) + .withSetting( MySettingsWithDefaults.hello, "ABC" ) + .withConfigClasses( Arrays.asList( mySettingsWithDefaults, myMigratingSettings ) ) + .build(); // Then assertTrue( config.getConfigValues().get( MySettingsWithDefaults.secretSetting.name() ).internal() ); assertFalse( config.getConfigValues().get( MySettingsWithDefaults.hello.name() ).internal() ); } + @Test + public void shouldSetSecretParameter() + { + // Given + Config config = Config.builder() + .withSetting( MySettingsWithDefaults.password, "this should not be visible" ) + .withSetting( MySettingsWithDefaults.hello, "ABC" ) + .withConfigClasses( Arrays.asList( mySettingsWithDefaults, myMigratingSettings ) ) + .build(); + + // Then + assertTrue( config.getConfigValues().get( MySettingsWithDefaults.password.name() ).isSecret() ); + assertFalse( config.getConfigValues().get( MySettingsWithDefaults.hello.name() ).isSecret() ); + String configText = config.toString(); + assertTrue( configText.contains( Secret.OBSFUCATED ) ); + assertFalse( configText.contains( "this should not be visible" ) ); + assertFalse( configText.contains( config.get( MySettingsWithDefaults.password ) ) ); + } + @Test public void shouldSetDocumentedDefaultValue() { diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/configuration/SecuritySettings.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/configuration/SecuritySettings.java index 3f0501c8ea027..448ab36ba4218 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/configuration/SecuritySettings.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/configuration/SecuritySettings.java @@ -30,6 +30,7 @@ import org.neo4j.configuration.Description; import org.neo4j.configuration.Internal; import org.neo4j.configuration.LoadableConfig; +import org.neo4j.configuration.Secret; import org.neo4j.graphdb.config.Setting; import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.logging.Level; @@ -228,6 +229,7 @@ public class SecuritySettings implements LoadableConfig public static final Setting ldap_authorization_system_username = setting( "dbms.security.ldap.authorization.system_username", STRING, NO_DEFAULT ); + @Secret @Description( "An LDAP system account password to use for authorization searches when " + "`dbms.security.ldap.authorization.use_system_account` is `true`." ) diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/integration/bolt/LdapAuthIT.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/integration/bolt/LdapAuthIT.java index d78596a0aba93..d3d3cfc363fe4 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/integration/bolt/LdapAuthIT.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/integration/bolt/LdapAuthIT.java @@ -58,22 +58,31 @@ import javax.naming.ldap.LdapContext; import org.neo4j.bolt.v1.transport.socket.client.TransportConnection; +import org.neo4j.configuration.Secret; import org.neo4j.graphdb.config.Setting; import org.neo4j.internal.kernel.api.security.AuthSubject; import org.neo4j.io.fs.FileSystemAbstraction; import org.neo4j.kernel.api.exceptions.InvalidArgumentsException; import org.neo4j.kernel.api.exceptions.Status; +import org.neo4j.kernel.configuration.Config; import org.neo4j.kernel.impl.factory.GraphDatabaseFacade; import org.neo4j.kernel.impl.proc.Procedures; +import org.neo4j.kernel.info.DiagnosticsPhase; import org.neo4j.kernel.internal.GraphDatabaseAPI; +import org.neo4j.logging.Logger; import org.neo4j.server.security.enterprise.auth.EnterpriseAuthAndUserManager; import org.neo4j.server.security.enterprise.auth.ProcedureInteractionTestBase; import org.neo4j.server.security.enterprise.auth.plugin.api.PredefinedRoles; import org.neo4j.server.security.enterprise.configuration.SecuritySettings; import org.neo4j.test.DoubleLatch; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.neo4j.bolt.v1.messaging.message.InitMessage.init; import static org.neo4j.bolt.v1.messaging.message.PullAllMessage.pullAll; import static org.neo4j.bolt.v1.messaging.message.RunMessage.run; @@ -1252,6 +1261,19 @@ public void shouldClearAuthorizationCache() throws Throwable } } + @Test + public void shouldNotSeeSystemPassword() + { + Config config = ((GraphDatabaseAPI) server.graphDatabaseService()).getDependencyResolver().resolveDependency( Config.class ); + String expected = "dbms.security.ldap.authorization.system_password=" + Secret.OBSFUCATED; + assertThat( "Should see obsfucated password in config.toString", config.toString(), containsString( expected ) ); + String password = config.get( SecuritySettings.ldap_authorization_system_password ); + assertThat( "Normal access should not be obsfucated", password, not( containsString( Secret.OBSFUCATED ) ) ); + Logger log = mock( Logger.class ); + config.dump( DiagnosticsPhase.EXPLICIT, log ); + verify( log, atLeastOnce() ).log( "%s=%s", "dbms.security.ldap.authorization.system_password", Secret.OBSFUCATED ); + } + private void clearAuthCacheFromDifferentConnection() throws Exception { TransportConnection adminClient = cf.newInstance();