From 8b083e7e4ef14efd3b5f497f8e9ec12fae1db5f2 Mon Sep 17 00:00:00 2001 From: Roberto Cortez Date: Thu, 16 Apr 2020 11:04:17 +0100 Subject: [PATCH] Fixes #269. Add LoggingInterceptor so we can see how to implement it without exposing secret keys. --- .../LoggingConfigSourceInterceptor.java | 23 +++++++++++ .../java/io/smallrye/config/SecretKeys.java | 31 +++----------- .../io/smallrye/config/SmallRyeConfig.java | 29 +++++++++---- .../config/inject/ConfigProducerUtil.java | 2 +- .../ConfigSourceLoggingInterceptorTest.java | 41 ------------------- .../LoggingConfigSourceInterceptorTest.java | 33 +++++++++++++++ .../io/smallrye/config/SecretKeysTest.java | 5 +-- .../config/inject/ConfigInjectionTest.java | 12 +++++- .../ConfigInjectionTestConfigFactory.java | 2 +- .../test/resources/config-values.properties | 2 +- 10 files changed, 97 insertions(+), 83 deletions(-) create mode 100644 implementation/src/main/java/io/smallrye/config/LoggingConfigSourceInterceptor.java delete mode 100644 implementation/src/test/java/io/smallrye/config/ConfigSourceLoggingInterceptorTest.java create mode 100644 implementation/src/test/java/io/smallrye/config/LoggingConfigSourceInterceptorTest.java diff --git a/implementation/src/main/java/io/smallrye/config/LoggingConfigSourceInterceptor.java b/implementation/src/main/java/io/smallrye/config/LoggingConfigSourceInterceptor.java new file mode 100644 index 000000000..b6e7f4478 --- /dev/null +++ b/implementation/src/main/java/io/smallrye/config/LoggingConfigSourceInterceptor.java @@ -0,0 +1,23 @@ +package io.smallrye.config; + +import org.jboss.logging.Logger; + +public class LoggingConfigSourceInterceptor implements ConfigSourceInterceptor { + private static final Logger LOG = Logger.getLogger("io.smallrye.config"); + + @Override + public ConfigValue getValue(final ConfigSourceInterceptorContext context, final String name) { + final ConfigValue configValue = context.proceed(name); + + if (configValue != null) { + final String value = configValue.getValue(); + final String configLocation = configValue.getConfigSourceName() + ":" + configValue.getLineNumber(); + + LOG.infov("The config {0} was loaded from {1} with the value {2}", name, configLocation, value); + } else { + LOG.infov("The config {0} was not found", name); + } + + return configValue; + } +} diff --git a/implementation/src/main/java/io/smallrye/config/SecretKeys.java b/implementation/src/main/java/io/smallrye/config/SecretKeys.java index f0063aa99..51f844d02 100644 --- a/implementation/src/main/java/io/smallrye/config/SecretKeys.java +++ b/implementation/src/main/java/io/smallrye/config/SecretKeys.java @@ -1,37 +1,16 @@ package io.smallrye.config; import java.io.Serializable; -import java.util.Set; -import java.util.function.Function; import java.util.function.Supplier; -public class SecretKeys implements Serializable { +class SecretKeys implements Serializable { private static final ThreadLocal LOCKED = ThreadLocal.withInitial(() -> Boolean.TRUE); - private final Function secrets; - - public SecretKeys() { - this.secrets = (Function & Serializable) s -> false; - } - - public SecretKeys(final Function secrets) { - this.secrets = secrets != null ? (Function & Serializable) secrets - : (Function & Serializable) s -> false; - } - - public SecretKeys(final Set secrets) { - this.secrets = (Function & Serializable) secrets::contains; - } - - public boolean isSecret(String propertyName) { - return secrets.apply(propertyName); - } - - public boolean isSecretAccessible(String propertyName) { - return !isSecret(propertyName) || !LOCKED.get(); + static boolean isLocked() { + return LOCKED.get(); } - public void accessSecrets(Runnable runnable) { + static void accessSecrets(Runnable runnable) { LOCKED.set(false); try { runnable.run(); @@ -40,7 +19,7 @@ public void accessSecrets(Runnable runnable) { } } - public String accessSecret(Supplier supplier) { + static String accessSecret(Supplier supplier) { LOCKED.set(false); try { return supplier.get(); diff --git a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java index 25e505a74..21b46b87a 100644 --- a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java +++ b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java @@ -38,6 +38,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.function.IntFunction; +import java.util.function.Supplier; import java.util.function.UnaryOperator; import java.util.stream.Collectors; @@ -69,7 +70,7 @@ public int compare(ConfigSource o1, ConfigSource o2) { private final Map> converters; private final Map>> optionalConverters = new ConcurrentHashMap<>(); private final ConfigSourceInterceptorContext interceptorChain; - private final SecretKeys secretKeys; + private final Set secretKeys; SmallRyeConfig(SmallRyeConfigBuilder builder) { this.configSourcesRef = buildConfigSources(builder); @@ -83,7 +84,7 @@ protected SmallRyeConfig(List configSources, Map(Collections.unmodifiableList(configSources)); this.converters = new ConcurrentHashMap<>(Converters.ALL_CONVERTERS); this.converters.putAll(converters); - this.secretKeys = new SecretKeys(); + this.secretKeys = Collections.emptySet(); this.interceptorChain = buildInterceptorChain(new SmallRyeConfigBuilder()); } @@ -128,8 +129,8 @@ private Map> buildConverters(final SmallRyeConfigBuilder buil return converters; } - private SecretKeys buildSecretKeys(final SmallRyeConfigBuilder builder) { - return new SecretKeys(builder.getSecretKeys()); + private Set buildSecretKeys(final SmallRyeConfigBuilder builder) { + return Collections.unmodifiableSet(builder.getSecretKeys()); } private ConfigSourceInterceptorContext buildInterceptorChain(final SmallRyeConfigBuilder builder) { @@ -172,7 +173,7 @@ private ConfigSourceInterceptorContext buildInterceptorChain(final SmallRyeConfi // Security interceptor to prevent access to secret keys current = new SmallRyeConfigSourceInterceptorContext((ConfigSourceInterceptor) (context, name) -> { - if (!secretKeys.isSecretAccessible(name)) { + if (SecretKeys.isLocked() && isSecret(name)) { throw new SecurityException("Not allowed to access secret key " + name); } return context.proceed(name); @@ -264,7 +265,7 @@ public Iterable getPropertyNames() { Set names = new HashSet<>(); for (ConfigSource configSource : getConfigSources()) { names.addAll( - configSource.getPropertyNames().stream().filter(secretKeys::isSecretAccessible) + configSource.getPropertyNames().stream().filter(this::isSecretAccessible) .collect(Collectors.toSet())); } return names; @@ -275,8 +276,20 @@ public Iterable getConfigSources() { return configSourcesRef.get(); } - public SecretKeys getSecretKeys() { - return secretKeys; + public void accessSecrets(Runnable runnable) { + SecretKeys.accessSecrets(runnable); + } + + public String accessSecret(Supplier supplier) { + return SecretKeys.accessSecret(supplier); + } + + private boolean isSecret(String name) { + return secretKeys.contains(name); + } + + private boolean isSecretAccessible(String name) { + return !isSecret(name) || !SecretKeys.isLocked(); } /** diff --git a/implementation/src/main/java/io/smallrye/config/inject/ConfigProducerUtil.java b/implementation/src/main/java/io/smallrye/config/inject/ConfigProducerUtil.java index d79fb2846..32d19b5fa 100644 --- a/implementation/src/main/java/io/smallrye/config/inject/ConfigProducerUtil.java +++ b/implementation/src/main/java/io/smallrye/config/inject/ConfigProducerUtil.java @@ -60,7 +60,7 @@ public static T getValue(InjectionPoint injectionPoint, Config config) { } private static String getRawValue(SmallRyeConfig config, String name) { - return config.getSecretKeys().accessSecret(() -> config.getRawValue(name)); + return config.accessSecret(() -> config.getRawValue(name)); } private static NoSuchElementException propertyNotFound(final String name) { diff --git a/implementation/src/test/java/io/smallrye/config/ConfigSourceLoggingInterceptorTest.java b/implementation/src/test/java/io/smallrye/config/ConfigSourceLoggingInterceptorTest.java deleted file mode 100644 index 657cbe770..000000000 --- a/implementation/src/test/java/io/smallrye/config/ConfigSourceLoggingInterceptorTest.java +++ /dev/null @@ -1,41 +0,0 @@ -package io.smallrye.config; - -import org.eclipse.microprofile.config.Config; -import org.jboss.logging.Logger; -import org.junit.Assert; -import org.junit.Test; - -public class ConfigSourceLoggingInterceptorTest { - @Test - public void interceptor() { - SmallRyeConfig config = (SmallRyeConfig) buildConfig("my.prop", "1234"); - - final String value = config.getValue("my.prop", String.class); - Assert.assertEquals("1234", value); - } - - private static Config buildConfig(String... keyValues) { - return new SmallRyeConfigBuilder() - .addDefaultSources() - .withSources(KeyValuesConfigSource.config(keyValues)) - .withInterceptors(new LoggingConfigSourceInterceptor()) - .build(); - } - - private static class LoggingConfigSourceInterceptor implements ConfigSourceInterceptor { - private static final Logger LOG = Logger.getLogger("io.smallrye.config"); - - @Override - public ConfigValue getValue(final ConfigSourceInterceptorContext context, final String name) { - final ConfigValue configValue = context.proceed(name); - - final String key = configValue.getName(); - final String value = configValue.getValue(); - final String configSource = configValue.getConfigSourceName(); - - LOG.infov("The key {0} was loaded from {1} with the value {2}", key, configSource, value); - - return configValue; - } - } -} diff --git a/implementation/src/test/java/io/smallrye/config/LoggingConfigSourceInterceptorTest.java b/implementation/src/test/java/io/smallrye/config/LoggingConfigSourceInterceptorTest.java new file mode 100644 index 000000000..b815a57c6 --- /dev/null +++ b/implementation/src/test/java/io/smallrye/config/LoggingConfigSourceInterceptorTest.java @@ -0,0 +1,33 @@ +package io.smallrye.config; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; + +import java.util.NoSuchElementException; + +import org.eclipse.microprofile.config.Config; +import org.junit.Test; + +public class LoggingConfigSourceInterceptorTest { + @Test + public void interceptor() throws Exception { + SmallRyeConfig config = (SmallRyeConfig) buildConfig(); + + //assertEquals("abc", config.getValue("my.prop", String.class)); + //assertThrows(SecurityException.class, () -> config.getValue("secret", String.class)); + //assertThrows(NoSuchElementException.class, () -> config.getValue("not.found", String.class)); + + // This should not log the secret: + config.accessSecret(() -> config.getRawValue("secret")); + } + + private static Config buildConfig() throws Exception { + return new SmallRyeConfigBuilder() + .addDefaultSources() + .withSources(new ConfigValuePropertiesConfigSource( + LoggingConfigSourceInterceptorTest.class.getResource("/config-values.properties"))) + .withInterceptors(new LoggingConfigSourceInterceptor()) + .withSecretKeys("secret") + .build(); + } +} diff --git a/implementation/src/test/java/io/smallrye/config/SecretKeysTest.java b/implementation/src/test/java/io/smallrye/config/SecretKeysTest.java index ba34b8af5..80c65eabc 100644 --- a/implementation/src/test/java/io/smallrye/config/SecretKeysTest.java +++ b/implementation/src/test/java/io/smallrye/config/SecretKeysTest.java @@ -24,8 +24,7 @@ public void unlock() { final Config config = buildConfig("secret", "12345678", "not.secret", "value"); final SmallRyeConfig smallRyeConfig = (SmallRyeConfig) config; - smallRyeConfig.getSecretKeys().accessSecrets( - () -> assertEquals("12345678", config.getValue("secret", String.class))); + smallRyeConfig.accessSecrets(() -> assertEquals("12345678", config.getValue("secret", String.class))); assertThrows("Not allowed to access secret key secret", SecurityException.class, () -> config.getValue("secret", String.class)); @@ -42,7 +41,7 @@ public void allPropertyNames() { final Config config = buildConfig("secret", "12345678", "not.secret", "value"); final SmallRyeConfig smallRyeConfig = (SmallRyeConfig) config; - smallRyeConfig.getSecretKeys().accessSecrets( + smallRyeConfig.accessSecrets( () -> assertTrue(stream(smallRyeConfig.getPropertyNames().spliterator(), false) .anyMatch(s -> s.equals("secret")))); } diff --git a/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTest.java b/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTest.java index 09a085f67..3072fa23e 100644 --- a/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTest.java +++ b/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTest.java @@ -50,6 +50,7 @@ public void afterClass() { @Test public void inject() { assertEquals("value", configBean.getConfig()); + assertEquals("value", configBean.getExpansion()); assertEquals("12345678", configBean.getSecret()); } @@ -59,14 +60,21 @@ public static class ConfigBean { @ConfigProperty(name = "config") private String config; @Inject + @ConfigProperty(name = "expansion") + private String expansion; + @Inject @ConfigProperty(name = "secret") private String secret; - public String getConfig() { + String getConfig() { return config; } - public String getSecret() { + String getExpansion() { + return expansion; + } + + String getSecret() { return secret; } } diff --git a/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTestConfigFactory.java b/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTestConfigFactory.java index b89f01d3e..bb0d9df4b 100644 --- a/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTestConfigFactory.java +++ b/implementation/src/test/java/io/smallrye/config/inject/ConfigInjectionTestConfigFactory.java @@ -12,7 +12,7 @@ public SmallRyeConfig getConfigFor( return configProviderResolver.getBuilder().forClassLoader(classLoader) .addDefaultSources() .addDefaultInterceptors() - .withSources(KeyValuesConfigSource.config("config", "value", "secret", "12345678")) + .withSources(KeyValuesConfigSource.config("config", "value", "expansion", "${config}", "secret", "12345678")) .withSecretKeys("secret") .build(); } diff --git a/implementation/src/test/resources/config-values.properties b/implementation/src/test/resources/config-values.properties index f22753f97..8e3c16c07 100644 --- a/implementation/src/test/resources/config-values.properties +++ b/implementation/src/test/resources/config-values.properties @@ -7,7 +7,7 @@ my.prop=abc - +secret=secret