[SECURITY-1497]

jenkinsci · Aug 6, 2019 · 322ef83 · 322ef83
1 parent c2060e9
commit 322ef83
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 5 deletions.
diff --git a/plugin/src/main/java/io/jenkins/plugins/casc/Attribute.java b/plugin/src/main/java/io/jenkins/plugins/casc/Attribute.java
@@ -1,6 +1,5 @@
 package io.jenkins.plugins.casc;
 
-import com.google.common.annotations.VisibleForTesting;
 import hudson.util.Secret;
 import io.jenkins.plugins.casc.model.CNode;
 import io.jenkins.plugins.casc.model.Scalar;
@@ -27,6 +26,8 @@
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang.StringUtils;
 import org.kohsuke.accmod.AccessRestriction;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.export.Exported;
 
 import static io.jenkins.plugins.casc.ConfigurationAsCode.printThrowable;
@@ -329,8 +330,8 @@ private static Field locatePrivateFieldInHierarchy(Class<?> clazz, @Nonnull Stri
      * @return {@code true} if the attribute is secret
      *         {@code false} if not or if there is no conclusive answer.
      */
-    @VisibleForTesting
-    /*package*/ static boolean calculateIfSecret(@CheckForNull Class<?> targetClass, @Nonnull String fieldName) {
+    @Restricted(NoExternalUse.class)
+    public static boolean calculateIfSecret(@CheckForNull Class<?> targetClass, @Nonnull String fieldName) {
         if (targetClass == Secret.class) { // Class is final, so the check is safe
             LOGGER.log(Level.FINER, "Attribute {0}#{1} is secret, because it has a Secret type",
                     new Object[] {targetClass.getName(), fieldName});

diff --git a/plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfigurator.java b/plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfigurator.java
@@ -160,8 +160,10 @@ private T tryConstructor(Constructor<T> constructor, Mapping config, Configurati
                         final Configurator configurator = context.lookupOrFail(k);
                         args[i] = configurator.configure(value, context);
                     }
-                    LOGGER.log(Level.FINE, "Setting {0}. {1} = {2}",
-                        new Object[] {target, names[i], t == Secret.class ? "****" : value});
+                    if (LOGGER.isLoggable(Level.FINE)) {
+                        LOGGER.log(Level.FINE, "Setting {0}. {1} = {2}",
+                                new Object[]{target, names[i], t == Secret.class || Attribute.calculateIfSecret(target, names[i]) ? "****" : value});
+                    }
                 } else if (t.isPrimitive()) {
                     args[i] = defaultValue(t);
                 }

diff --git a/...n/src/test/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfiguratorTest.java b/...n/src/test/java/io/jenkins/plugins/casc/impl/configurators/DataBoundConfiguratorTest.java
@@ -1,5 +1,6 @@
 package io.jenkins.plugins.casc.impl.configurators;
 
+import hudson.util.Secret;
 import io.jenkins.plugins.casc.ConfigurationAsCode;
 import io.jenkins.plugins.casc.ConfigurationContext;
 import io.jenkins.plugins.casc.Configurator;
@@ -15,15 +16,21 @@
 import io.jenkins.plugins.casc.model.Sequence;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.ParametersAreNonnullByDefault;
 import javax.annotation.PostConstruct;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.LoggerRule;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
 
+import static io.jenkins.plugins.casc.misc.Util.assertNotInLog;
+import static io.jenkins.plugins.casc.misc.Util.assertLogContains;
 import static io.jenkins.plugins.casc.misc.Util.getJenkinsRoot;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertEquals;
@@ -41,6 +48,14 @@ public class DataBoundConfiguratorTest {
     @Rule
     public JenkinsRule j = new JenkinsRule();
 
+    @Rule
+    public LoggerRule logging = new LoggerRule();
+
+    @Before
+    public void tearUp() {
+        logging.record(Logger.getLogger(DataBoundConfigurator.class.getName()), Level.FINEST).capture(2048);
+    }
+
     @Test
     public void configure_databound() throws Exception {
         Mapping config = new Mapping();
@@ -208,6 +223,27 @@ public void shouldThrowConfiguratorException() {
         }
     }
 
+    @Test
+    public void shouldNotLogSecrets() throws Exception {
+        Mapping config = new Mapping();
+        config.put("secret", "mySecretValue");
+        ConfiguratorRegistry registry = ConfiguratorRegistry.get();
+        registry.lookupOrFail(SecretHolder.class).configure(config, new ConfigurationContext(registry));
+        assertLogContains(logging, "secret");
+        assertNotInLog(logging, "mySecretValue");
+    }
+
+    @Test
+    @Issue("SECURITY-1497")
+    public void shouldNotLogSecretsForUndefinedConstructors() throws Exception {
+        Mapping config = new Mapping();
+        config.put("secret", "mySecretValue");
+        ConfiguratorRegistry registry = ConfiguratorRegistry.get();
+        registry.lookupOrFail(SecretHolderWithString.class).configure(config, new ConfigurationContext(registry));
+        assertLogContains(logging, "secret");
+        assertNotInLog(logging, "mySecretValue");
+    }
+
     public static class Foo {
 
         final String foo;
@@ -287,4 +323,23 @@ public Set<String> getStrings() {
         }
     }
 
+    public static class SecretHolder {
+
+        Secret secret;
+
+        @DataBoundConstructor
+        public SecretHolder(Secret secret) {
+            this.secret = secret;
+        }
+    }
+
+    public static class SecretHolderWithString {
+
+        Secret secret;
+
+        @DataBoundConstructor
+        public SecretHolderWithString(String secret) {
+            this.secret = Secret.fromString(secret);
+        }
+    }
 }