Fixes smallrye#269. Add LoggingInterceptor so we can see how to imple…

…ment it without exposing secret keys.
radcortez · Apr 17, 2020 · 8b083e7 · 8b083e7
1 parent cd22f7d
commit 8b083e7
Show file tree

Hide file tree

Showing 10 changed files with 97 additions and 83 deletions.
diff --git a/implementation/src/main/java/io/smallrye/config/LoggingConfigSourceInterceptor.java 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
@@ -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<Boolean> LOCKED = ThreadLocal.withInitial(() -> Boolean.TRUE);
 
-    private final Function<String, Boolean> secrets;
-
-    public SecretKeys() {
-        this.secrets = (Function<String, Boolean> & Serializable) s -> false;
-    }
-
-    public SecretKeys(final Function<String, Boolean> secrets) {
-        this.secrets = secrets != null ? (Function<String, Boolean> & Serializable) secrets
-                : (Function<String, Boolean> & Serializable) s -> false;
-    }
-
-    public SecretKeys(final Set<String> secrets) {
-        this.secrets = (Function<String, Boolean> & 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<String> supplier) {
+    static String accessSecret(Supplier<String> 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
@@ -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<Type, Converter<?>> converters;
     private final Map<Type, Converter<Optional<?>>> optionalConverters = new ConcurrentHashMap<>();
     private final ConfigSourceInterceptorContext interceptorChain;
-    private final SecretKeys secretKeys;
+    private final Set<String> secretKeys;
 
     SmallRyeConfig(SmallRyeConfigBuilder builder) {
         this.configSourcesRef = buildConfigSources(builder);
@@ -83,7 +84,7 @@ protected SmallRyeConfig(List<ConfigSource> configSources, Map<Type, Converter<?
         this.configSourcesRef = new AtomicReference<>(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<Type, Converter<?>> buildConverters(final SmallRyeConfigBuilder buil
         return converters;
     }
 
-    private SecretKeys buildSecretKeys(final SmallRyeConfigBuilder builder) {
-        return new SecretKeys(builder.getSecretKeys());
+    private Set<String> 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<String> getPropertyNames() {
         Set<String> 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<ConfigSource> getConfigSources() {
         return configSourcesRef.get();
     }
 
-    public SecretKeys getSecretKeys() {
-        return secretKeys;
+    public void accessSecrets(Runnable runnable) {
+        SecretKeys.accessSecrets(runnable);
+    }
+
+    public String accessSecret(Supplier<String> 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
@@ -60,7 +60,7 @@ public static <T> 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
diff --git a/implementation/src/test/java/io/smallrye/config/LoggingConfigSourceInterceptorTest.java 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
@@ -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
@@ -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
@@ -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
@@ -7,7 +7,7 @@ my.prop=abc
 
 
 
-
+secret=secret
Original file line number	Diff line number	Diff line change
Expand Up		@@ -7,7 +7,7 @@ my.prop=abc




		secret=secret



Expand Down