Obsfucate secret configs in more places

* the logger passed to Configs, and called on updates
* the results from dbms.listConfigs

Even though these are admin actions, so is reading the debug.log, so
we obsfucate these cases for the same reason.

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.isSecret() );
+                            setting.deprecated(), setting.replacement(), setting.secret() );
                 } )
                 .collect( Collectors.toList() );
     }

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

@@ -77,7 +77,7 @@ public Optional<Object> value()
     @Nonnull
     public Optional<String> valueAsString()
     {
-        return value.map( ConfigValue::valueToString );
+        return this.isSecret() ? Optional.of( Secret.OBSFUCATED ) : value.map( ConfigValue::valueToString );
     }
 
     @Override

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

@@ -78,7 +78,7 @@ default List<ConfigOptions> getConfigOptions()
                     setting.setInternal( internalAnnotation != null );
 
                     final Secret secretAnnotation = f.getAnnotation( Secret.class );
-                    setting.makeSecret( secretAnnotation != null );
+                    setting.setSecret( secretAnnotation != null );
 
                     final Dynamic dynamicAnnotation = f.getAnnotation( Dynamic.class );
                     setting.setDynamic( dynamicAnnotation != null );

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

@@ -0,0 +1,36 @@
+/*
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+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.
+ * It is supported in configs set using LoadableConfig, but not for group settings, like connectors and SSL policies.
+ */
+@Retention( RetentionPolicy.RUNTIME )
+@Target( {ElementType.TYPE, ElementType.FIELD} )
+public @interface Secret
+{
+    String OBSFUCATED = "##########";
+}

community/configuration/src/test/java/org/neo4j/configuration/ConfigValueTest.java

@@ -42,6 +42,7 @@ public void handlesEmptyValue()
         assertFalse( value.deprecated() );
         assertEquals( Optional.empty(), value.replacement() );
         assertFalse( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -51,6 +52,7 @@ public void handlesInternal()
                 "description", true, false, false, Optional.empty(), false );
 
         assertTrue( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -64,6 +66,7 @@ public void handlesNonEmptyValue()
         assertFalse( value.deprecated() );
         assertEquals( Optional.empty(), value.replacement() );
         assertFalse( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -77,6 +80,7 @@ public void handlesDeprecationAndReplacement()
         assertTrue( value.deprecated() );
         assertEquals( "new_name", value.replacement().get() );
         assertFalse( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -90,9 +94,24 @@ public void handlesValueDescription()
         assertTrue( value.deprecated() );
         assertEquals( "new_name", value.replacement().get() );
         assertFalse( value.internal() );
+        assertFalse( value.isSecret() );
         assertEquals( "a simple integer", value.valueDescription() );
     }
 
+    @Test
+    public void handlesSecretValue() throws Exception
+    {
+        ConfigValue value = new ConfigValue( "name", Optional.empty(), Optional.empty(), Optional.of( "secret" ),
+                "description", false, false, false, Optional.empty(), true );
+
+        assertEquals( Optional.of( "secret" ), value.value() );
+        assertEquals( Secret.OBSFUCATED, value.toString() );
+        assertFalse( value.deprecated() );
+        assertEquals( Optional.empty(), value.replacement() );
+        assertFalse( value.internal() );
+        assertTrue( value.isSecret() );
+    }
+
     @Test
     public void durationValueIsRepresentedWithUnit()
     {

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

@@ -70,12 +70,12 @@ public void setInternal( final boolean val )
     }
 
     @Override
-    public boolean isSecret()
+    public boolean secret()
     {
         return this.isSecret;
     }
 
-    public void makeSecret( final boolean val )
+    public void setSecret( final boolean val )
     {
         this.isSecret = val;
     }

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

@@ -65,7 +65,10 @@ public interface SettingGroup<T> extends SettingValidator
     /**
      * @return {@code true} if secret setting (should be hidden), false otherwise.
      */
-    boolean isSecret();
+    default boolean secret()
+    {
+        return false;
+    }
 
     /**
      * @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

@@ -397,7 +397,7 @@ private Config( File configFile,
         // Find secret settings
         configOptions.stream()
                 .map( ConfigOptions::settingGroup )
-                .filter( SettingGroup::isSecret )
+                .filter( SettingGroup::secret )
                 .filter( BaseSetting.class::isInstance )
                 .map( BaseSetting.class::cast )
                 .forEach( setting -> secrets.add( setting.name() ) );
@@ -646,9 +646,12 @@ public void updateDynamicSetting( String setting, String update, String origin )
                 }
                 newValue = update;
             }
+            //TODO: check if this is correct
+            String oldValueForLog = obsfucateIfSecret( setting, oldValue );
+            String newValueForLog = obsfucateIfSecret( setting, newValue );
             log.info( "Setting changed: '%s' changed from '%s' to '%s' via '%s'",
-                    setting, oldValueIsDefault ? "default (" + oldValue + ")" : oldValue,
-                    newValueIsDefault ? "default (" + newValue + ")" : newValue, origin );
+                    setting, oldValueIsDefault ? "default (" + oldValueForLog + ")" : oldValueForLog,
+                    newValueIsDefault ? "default (" + newValueForLog + ")" : newValueForLog, origin );
             updateListeners.getOrDefault( setting, emptyList() ).forEach( l -> l.accept( oldValue, newValue ) );
         }
     }
@@ -761,7 +764,12 @@ public void dump( DiagnosticsPhase phase, Logger logger )
 
     private String obsfucateIfSecret( Map.Entry<String,String> param )
     {
-        return secrets.contains( param.getKey() ) ? Secret.OBSFUCATED : param.getValue();
+        return obsfucateIfSecret( param.getKey(), param.getValue() );
+    }
+
+    private String obsfucateIfSecret( String key, String value )
+    {
+        return secrets.contains( key ) ? Secret.OBSFUCATED : value;
     }
 
     /**

community/kernel/src/main/java/org/neo4j/kernel/configuration/ConnectorValidator.java

@@ -231,7 +231,7 @@ public boolean internal()
     }
 
     @Override
-    public boolean isSecret()
+    public boolean secret()
     {
         return false;
     }

community/kernel/src/main/java/org/neo4j/kernel/configuration/Settings.java

@@ -1188,9 +1188,9 @@ public boolean internal()
             }
 
             @Override
-            public boolean isSecret()
+            public boolean secret()
             {
-                return newSetting.isSecret();
+                return newSetting.secret();
             }
 
             @Override

community/kernel/src/main/java/org/neo4j/kernel/configuration/ssl/SslPolicyConfigValidator.java

@@ -158,7 +158,7 @@ public boolean internal()
     }
 
     @Override
-    public boolean isSecret()
+    public boolean secret()
     {
         return false;
     }

community/kernel/src/test/java/org/neo4j/kernel/configuration/ConfigTest.java

@@ -19,6 +19,7 @@
  */
 package org.neo4j.kernel.configuration;
 
+import org.hamcrest.CoreMatchers;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -49,6 +50,7 @@
 import static java.util.Collections.singletonList;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
@@ -390,6 +392,10 @@ public static class MyDynamicSettings implements LoadableConfig
     {
         @Dynamic
         public static final Setting<Boolean> boolSetting = setting( "bool_setting", BOOLEAN, Settings.TRUE );
+
+        @Dynamic
+        @Secret
+        public static final Setting<String> secretSetting = setting( "password", STRING, "secret" );
     }
 
     @Test
@@ -470,4 +476,39 @@ public void updateDynamicShouldLogExceptionsFromUpdateListeners()
         verify( log ).error( "Failure when notifying listeners after dynamic setting change; " +
                              "new setting might not have taken effect: Boo", exception );
     }
+
+    @Test
+    public void updateDynamicShouldWorkWithSecret() throws Exception
+    {
+        // Given a secret dynamic setting with a registered update listener
+        String settingName = MyDynamicSettings.secretSetting.name();
+        String changedMessage = "Setting changed: '%s' changed from '%s' to '%s' via '%s'";
+        Config config = Config.builder().withConfigClasses( singletonList( new MyDynamicSettings() ) ).build();
+
+        Log log = mock( Log.class );
+        config.setLogger( log );
+
+        AtomicInteger counter = new AtomicInteger( 0 );
+        config.registerDynamicUpdateListener( MyDynamicSettings.secretSetting, ( previous, update ) ->
+        {
+            counter.getAndIncrement();
+            assertThat( "Update listener should not see obsfucated secret", previous, not( CoreMatchers.equalTo( Secret.OBSFUCATED ) ) );
+            assertThat( "Update listener should not see obsfucated secret", update, not( CoreMatchers.equalTo( Secret.OBSFUCATED ) ) );
+        } );
+
+        // When changing secret settings three times
+        config.updateDynamicSetting( settingName, "another", ORIGIN );
+        config.updateDynamicSetting( settingName, "secret2", ORIGIN );
+        config.updateDynamicSetting( settingName, "", ORIGIN );
+
+        // Then we should see obsfucated log messages
+        InOrder order = inOrder( log );
+        order.verify( log ).info( changedMessage, settingName, "default (" + Secret.OBSFUCATED + ")", Secret.OBSFUCATED, ORIGIN );
+        order.verify( log ).info( changedMessage, settingName, Secret.OBSFUCATED, Secret.OBSFUCATED, ORIGIN );
+        order.verify( log ).info( changedMessage, settingName, Secret.OBSFUCATED, "default (" + Secret.OBSFUCATED + ")", ORIGIN );
+        verifyNoMoreInteractions( log );
+
+        // And see 3 calls to the update listener
+        assertThat( counter.get(), is( 3 ) );
+    }
 }