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.
neo4j · Apr 4, 2018 · 047f7fb · 047f7fb
1 parent 382113f
commit 047f7fb
Show file tree

Hide file tree

Showing 12 changed files with 85 additions and 14 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
@@ -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() );
     }

diff --git a/community/configuration/src/main/java/org/neo4j/configuration/ConfigValue.java b/community/configuration/src/main/java/org/neo4j/configuration/ConfigValue.java
@@ -77,7 +77,7 @@ public Optional<?> value()
     @Nonnull
     public Optional<String> valueAsString()
     {
-        return value.map( ConfigValue::valueToString );
+        return this.isSecret() ? Optional.of( Secret.OBSFUCATED ) : value.map( ConfigValue::valueToString );
     }
 
     @Override

diff --git a/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java b/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java
@@ -77,7 +77,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 );

diff --git a/community/configuration/src/main/java/org/neo4j/configuration/Secret.java b/community/configuration/src/main/java/org/neo4j/configuration/Secret.java
@@ -26,6 +26,7 @@
 
 /**
  * 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} )

diff --git a/community/configuration/src/test/java/org/neo4j/configuration/ConfigValueTest.java b/community/configuration/src/test/java/org/neo4j/configuration/ConfigValueTest.java
@@ -42,6 +42,7 @@ public void handlesEmptyValue() throws Exception
         assertFalse( value.deprecated() );
         assertEquals( Optional.empty(), value.replacement() );
         assertFalse( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -51,6 +52,7 @@ public void handlesInternal() throws Exception
                 "description", true, false, false, Optional.empty(), false );
 
         assertTrue( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -64,6 +66,7 @@ public void handlesNonEmptyValue() throws Exception
         assertFalse( value.deprecated() );
         assertEquals( Optional.empty(), value.replacement() );
         assertFalse( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -77,6 +80,7 @@ public void handlesDeprecationAndReplacement() throws Exception
         assertTrue( value.deprecated() );
         assertEquals( "new_name", value.replacement().get() );
         assertFalse( value.internal() );
+        assertFalse( value.isSecret() );
     }
 
     @Test
@@ -90,9 +94,24 @@ public void handlesValueDescription() throws Exception
         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() throws Exception
     {

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
@@ -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;
     }

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
@@ -63,7 +63,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.

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
@@ -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() ) );
@@ -645,9 +645,11 @@ public void updateDynamicSetting( String setting, String update )
                 }
                 newValue = update;
             }
+            String oldValueForLog = obsfucateIfSecret( setting, oldValue );
+            String newValueForLog = obsfucateIfSecret( setting, newValue );
             log.info( "Setting changed: '%s' changed from '%s' to '%s'",
-                    setting, oldValueIsDefault ? "default (" + oldValue + ")" : oldValue,
-                    newValueIsDefault ? "default (" + newValue + ")" : newValue );
+                    setting, oldValueIsDefault ? "default (" + oldValueForLog + ")" : oldValueForLog,
+                    newValueIsDefault ? "default (" + newValueForLog + ")" : newValueForLog );
             updateListeners.getOrDefault( setting, emptyList() ).forEach( l -> l.accept( oldValue, newValue ) );
         }
     }
@@ -760,7 +762,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;
     }
 
     /**

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
@@ -229,7 +229,7 @@ public boolean internal()
     }
 
     @Override
-    public boolean isSecret()
+    public boolean secret()
     {
         return false;
     }

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
@@ -1228,9 +1228,9 @@ public boolean internal()
             }
 
             @Override
-            public boolean isSecret()
+            public boolean secret()
             {
-                return newSetting.isSecret();
+                return newSetting.secret();
             }
 
             @Override

diff --git a/...ity/kernel/src/main/java/org/neo4j/kernel/configuration/ssl/SslPolicyConfigValidator.java b/...ity/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;
     }

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
@@ -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() throws Excepti
         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'";
+        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" );
+        config.updateDynamicSetting( settingName, "secret2" );
+        config.updateDynamicSetting( settingName, "" );
+
+        // Then we should see obsfucated log messages
+        InOrder order = inOrder( log );
+        order.verify( log ).info( changedMessage, settingName, "default (" + Secret.OBSFUCATED + ")", Secret.OBSFUCATED );
+        order.verify( log ).info( changedMessage, settingName, Secret.OBSFUCATED, Secret.OBSFUCATED );
+        order.verify( log ).info( changedMessage, settingName, Secret.OBSFUCATED, "default (" + Secret.OBSFUCATED + ")" );
+        verifyNoMoreInteractions( log );
+
+        // And see 3 calls to the update listener
+        assertThat( counter.get(), is( 3 ) );
+    }
 }