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 f7c206628812b..f5347061eba3c 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.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 index 1a60ad71acd9c..3309f39c36c75 100644 --- 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 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 index 11c0b5f5ce9a7..b6a75a598209a 100644 --- a/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java +++ b/community/configuration/src/main/java/org/neo4j/configuration/LoadableConfig.java @@ -78,7 +78,7 @@ default List 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 new file mode 100644 index 0000000000000..ee3a696fc43c2 --- /dev/null +++ b/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 . + */ +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 = "##########"; +} 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 fc678b197ec66..91bab3c7993f2 100644 --- 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() 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() { 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 035965e15338b..6c97d7c091131 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 @@ -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 index 9147f542f5943..4de825007c897 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 @@ -65,7 +65,10 @@ public interface SettingGroup 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 index 8ef9ed1ed8a84..225fab57492b1 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 @@ -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 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 index 6f723f3879313..01c8c0d448fdd 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 @@ -231,7 +231,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 index 1288d3b1bbd2b..fcfe27b2c3e9b 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 @@ -1188,9 +1188,9 @@ public boolean internal() } @Override - public boolean isSecret() + public boolean secret() { - return newSetting.isSecret(); + return newSetting.secret(); } @Override 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 9905565106d9d..7bc156731c6f5 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 @@ -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 index 2a5c820081ade..6679a3239ad00 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 @@ -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 boolSetting = setting( "bool_setting", BOOLEAN, Settings.TRUE ); + + @Dynamic + @Secret + public static final Setting 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 ) ); + } }