From 382113f28df20f791e030427a732b232694065d3 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 ++ .../java/org/neo4j/configuration/Secret.java | 35 +++++++++++++++++++ .../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 ++++++++++++ 14 files changed, 157 insertions(+), 16 deletions(-) create mode 100644 community/configuration/src/main/java/org/neo4j/configuration/Secret.java 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 f23677e4430c7..52419bdc73e70 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 8f12215652818..c23dec47c2992 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 1b0cb432affc8..e24b3d6d58183 100644 --- a/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java +++ b/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java @@ -76,6 +76,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/main/java/org/neo4j/configuration/Secret.java b/community/configuration/src/main/java/org/neo4j/configuration/Secret.java new file mode 100644 index 0000000000000..74e609acd9ff8 --- /dev/null +++ b/community/configuration/src/main/java/org/neo4j/configuration/Secret.java @@ -0,0 +1,35 @@ +/* + * Copyright (c) 2002-2018 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.configuration; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Used to define a configuration setting as secret, which means we should not show the value in logs. + */ +@Retention( RetentionPolicy.RUNTIME ) +@Target( {ElementType.TYPE, ElementType.FIELD} ) +public @interface Secret +{ + String OBSFUCATED = "##########"; +} 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 971af18416684..4b24210d85401 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() throws Exception { 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() throws Exception public void handlesInternal() throws Exception { 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() throws Exception public void handlesNonEmptyValue() throws Exception { 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() throws Exception public void handlesDeprecationAndReplacement() throws Exception { 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() throws Exception public void handlesValueDescription() throws Exception { 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 63a37fca6d036..6d201c94cd90f 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 9ed2c8c9b0380..5018acfc8faf5 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 @@ -60,6 +60,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 11c69e9bf869b..12a861502dc8f 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 ); @@ -741,11 +753,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. * @@ -942,7 +959,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 6e24c8a1cd5d8..fdbe402655599 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 @@ -228,6 +228,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 62cdde1b97a9e..b40c163c032c8 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 @@ -1227,6 +1227,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 d3be466580b5c..d332c73bf9ca2 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 2dccb236e0083..3e3c784008687 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; @@ -101,6 +102,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 @@ -249,16 +253,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() throws Exception 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 03221ece77a71..529bc1c7224bd 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 @@ -27,6 +27,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; @@ -225,6 +226,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 510b97844e755..8ce8de13f2470 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 @@ -56,21 +56,30 @@ import org.neo4j.bolt.v1.transport.integration.TransportTestUtil; import org.neo4j.bolt.v1.transport.socket.client.TransportConnection; +import org.neo4j.configuration.Secret; import org.neo4j.graphdb.config.Setting; import org.neo4j.io.fs.FileSystemAbstraction; import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.enterprise.api.security.EnterpriseSecurityContext; +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; @@ -1190,6 +1199,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();