Obsfucate the LDAP system password in toString and debug.log

community/configuration/src/main/java/org/neo4j/configuration/ConfigOptions.java

@@ -61,7 +61,7 @@ public List<ConfigValue> asConfigValues( @Nonnull Map<String,String> 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() );
     }

community/configuration/src/main/java/org/neo4j/configuration/ConfigValue.java

@@ -34,21 +34,23 @@ public class ConfigValue
     private final Optional<Object> value;
     private final String valueDescription;
     private final boolean internal;
+    private final boolean isSecret;
     private final boolean dynamic;
     private final boolean deprecated;
     private final Optional<String> replacement;
 
     public ConfigValue( @Nonnull String name, @Nonnull Optional<String> description,
             @Nonnull Optional<String> documentedDefaultValue, @Nonnull Optional<Object> value,
             @Nonnull String valueDescription, boolean internal, boolean dynamic, boolean deprecated,
-            @Nonnull Optional<String> replacement )
+            @Nonnull Optional<String> replacement, boolean isSecret )
     {
         this.name = name;
         this.description = description;
         this.documentedDefaultValue = documentedDefaultValue;
         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;

community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java

@@ -77,6 +77,9 @@ default List<ConfigOptions> 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 );
                 }

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() );

community/graphdb-api/src/main/java/org/neo4j/graphdb/config/BaseSetting.java

@@ -31,6 +31,7 @@ public abstract class BaseSetting<T> implements Setting<T>
     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<String> documentedDefaultValue()
     {

community/graphdb-api/src/main/java/org/neo4j/graphdb/config/SettingGroup.java

@@ -62,6 +62,11 @@ public interface SettingGroup<T> 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.
      */

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> configOptions;
 
     private final Map<String,String> params = new CopyOnWriteHashMap<>(); // Read heavy workload
+    private final Set<String> secrets = new HashSet<>();
     private final Map<String, Collection<BiConsumer<String,String>>> updateListeners = new ConcurrentHashMap<>();
     private final ConfigurationMigrator migrator;
     private final List<ConfigurationValidator> 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<String,String> param : params.entrySet() )
             {
-                logger.log( "%s=%s", param.getKey(), param.getValue() );
+                logger.log( "%s=%s", param.getKey(), obsfucateIfSecret( param ) );
             }
         }
     }
 
+    private String obsfucateIfSecret( Map.Entry<String,String> param )
+    {
+        return secrets.contains( param.getKey() ) ? Secret.OBSFUCATED : param.getValue();
+    }
+
     /**
      * Migrates and validates all string values in the provided <code>settings</code> 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( ", ") );
     }
 }

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<String> documentedDefaultValue()
     {

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<String> documentedDefaultValue()
             {

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<String> documentedDefaultValue()
     {

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<String> oldSetting = setting( "some_setting", STRING, "Has no replacement" );
+
+        @Secret
+        public static final Setting<String> 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()
     {

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<String> 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`." )

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();