Merge pull request #132 from jglick/show-username-JENKINS-44860-alt

[JENKINS-44860] Honor secret setting for username
jenkinsci · Jun 2, 2021 · eb36486 · eb36486
2 parents 0f8ff9f + cb242b0
commit eb36486
Show file tree

Hide file tree

Showing 11 changed files with 141 additions and 41 deletions.
diff --git a/pom.xml b/pom.xml
@@ -65,6 +65,7 @@
     <dependency>
       <groupId>org.jenkins-ci.plugins</groupId>
       <artifactId>credentials</artifactId>
+      <version>2.5</version>
     </dependency>
     <dependency>
       <groupId>org.jenkins-ci.plugins</groupId>

diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/Binding.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/Binding.java
@@ -151,7 +151,7 @@ private static class UnbinderWrapper implements Unbinder {
         return new MultiEnvironment(Collections.singletonMap(variable, single.value), single.unbinder);
     }
 
-    @Override public final Set<String> variables() {
+    @Override public final Set<String> variables(Run<?, ?> build) {
         return Collections.singleton(variable);
     }
 

diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/MultiBinding.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/MultiBinding.java
@@ -30,11 +30,11 @@
 import hudson.ExtensionPoint;
 import hudson.FilePath;
 import hudson.Launcher;
+import hudson.Util;
 import hudson.model.AbstractDescribableImpl;
 import hudson.model.Descriptor;
 import hudson.model.Run;
 import hudson.model.TaskListener;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.Serializable;
 import java.util.Collections;
@@ -45,6 +45,7 @@
 import javax.annotation.Nullable;
 
 import jenkins.model.Jenkins;
+import org.apache.commons.collections.CollectionUtils;
 import org.jenkinsci.plugins.credentialsbinding.impl.CredentialNotFoundException;
 import org.kohsuke.stapler.DataBoundConstructor;
 
@@ -72,20 +73,57 @@ public final String getCredentialsId() {
     /** Result of {@link #bind}. */
     public static final class MultiEnvironment implements Serializable {
 
-        private final Map<String,String> values;
+        private static final long serialVersionUID = 1;
+
+        @Deprecated
+        private transient Map<String,String> values;
+        private Map<String,String> secretValues;
+        private Map<String,String> publicValues;
         private final Unbinder unbinder;
 
-        public MultiEnvironment(Map<String,String> values) {
-            this(values, new NullUnbinder());
+        public MultiEnvironment(Map<String,String> secretValues) {
+            this(secretValues, Collections.<String, String>emptyMap());
         }
 
-        public MultiEnvironment(Map<String,String> values, Unbinder unbinder) {
-            this.values = new LinkedHashMap<>(values);
+        public MultiEnvironment(Map<String,String> secretValues, Map<String,String> publicValues) {
+            this(secretValues, publicValues, new NullUnbinder());
+        }
+
+        public MultiEnvironment(Map<String,String> secretValues, Unbinder unbinder) {
+            this(secretValues, Collections.<String, String>emptyMap(), unbinder);
+        }
+
+        public MultiEnvironment(Map<String,String> secretValues, Map<String,String> publicValues, Unbinder unbinder) {
+            this.values = null;
+            this.secretValues = new LinkedHashMap<>(secretValues);
+            this.publicValues = new LinkedHashMap<>(publicValues);
+            if (!CollectionUtils.intersection(secretValues.keySet(), publicValues.keySet()).isEmpty()) {
+                throw new IllegalArgumentException("Cannot use the same key in both secretValues and publicValues");
+            }
             this.unbinder = unbinder;
         }
 
+        // To avoid de-serialization issues with newly added field (secretValues, publicValues)
+        private Object readResolve() {
+            if (values != null) {
+                secretValues = values;
+                publicValues = Collections.emptyMap();
+                values = null;
+            }
+            return this;
+        }
+
+        @Deprecated
         public Map<String,String> getValues() {
-            return Collections.unmodifiableMap(values);
+            return Collections.unmodifiableMap(secretValues);
+        }
+
+        public Map<String,String> getSecretValues() {
+            return Collections.unmodifiableMap(secretValues);
+        }
+
+        public Map<String,String> getPublicValues() {
+            return Collections.unmodifiableMap(publicValues);
         }
 
         public Unbinder getUnbinder() {
@@ -131,16 +169,29 @@ public abstract MultiEnvironment bind(@Nonnull Run<?,?> build,
                                           @Nullable Launcher launcher,
                                           @Nonnull TaskListener listener) throws IOException, InterruptedException;
 
-    /** Defines keys expected to be set in {@link MultiEnvironment#getValues}, particularly any that might be sensitive. */
-    public abstract Set<String> variables();
+    /**
+     * @deprecated override {@link #variables(Run)}
+     */
+    public Set<String> variables() {
+        return Collections.emptySet();
+    }
+
+    /** Defines keys expected to be set in {@link MultiEnvironment#getSecretValues()}, particularly any that might be sensitive. */
+    public /*abstract*/ Set<String> variables(@Nonnull Run<?,?> build) throws CredentialNotFoundException {
+        if (Util.isOverridden(MultiBinding.class, getClass(), "variables")) {
+            return variables();
+        } else {
+            throw new AbstractMethodError("Implement variables");
+        }
+    }
 
     /**
      * Looks up the actual credentials.
      * @param build the build.
      * @return the credentials
-     * @throws FileNotFoundException if the credentials could not be found (for convenience, rather than returning null)
+     * @throws CredentialNotFoundException if the credentials could not be found (for convenience, rather than returning null)
      */
-    protected final @Nonnull C getCredentials(@Nonnull Run<?,?> build) throws IOException {
+    protected final @Nonnull C getCredentials(@Nonnull Run<?,?> build) throws CredentialNotFoundException {
         IdCredentials cred = CredentialsProvider.findCredentialById(credentialsId, IdCredentials.class, build);
         if (cred==null)
             throw new CredentialNotFoundException("Could not find credentials entry with ID '" + credentialsId + "'");

diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java
@@ -45,6 +45,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -125,6 +126,8 @@ private void doStart() throws Exception {
             FilePath workspace = getContext().get(FilePath.class);
             Launcher launcher = getContext().get(Launcher.class);
 
+            Collection<String> secrets = new HashSet<>();
+            Collection<String> secretKeys = new LinkedHashSet<>();
             Map<String,String> overrides = new LinkedHashMap<>();
             List<MultiBinding.Unbinder> unbinders = new ArrayList<>();
             for (MultiBinding<?> binding : step.bindings) {
@@ -134,17 +137,21 @@ private void doStart() throws Exception {
                 }
                 MultiBinding.MultiEnvironment environment = binding.bind(run, workspace, launcher, listener);
                 unbinders.add(environment.getUnbinder());
-                overrides.putAll(environment.getValues());
+                Map<String, String> secretValues = environment.getSecretValues();
+                secrets.addAll(secretValues.values());
+                secretKeys.addAll(secretValues.keySet());
+                overrides.putAll(secretValues);
+                overrides.putAll(environment.getPublicValues());
             }
             if (!overrides.isEmpty()) {
                 boolean unix = launcher != null ? launcher.isUnix() : true;
-                listener.getLogger().println("Masking supported pattern matches of " + overrides.keySet().stream().map(
+                listener.getLogger().println("Masking supported pattern matches of " + secretKeys.stream().map(
                     v -> unix ? "$" + v : "%" + v + "%"
                 ).collect(Collectors.joining(" or ")));
             }
             getContext().newBodyInvoker().
                     withContext(EnvironmentExpander.merge(getContext().get(EnvironmentExpander.class), new Overrider(overrides))).
-                    withContext(BodyInvoker.mergeConsoleLogFilters(getContext().get(ConsoleLogFilter.class), new Filter(overrides.values(), run.getCharset().name()))).
+                    withContext(BodyInvoker.mergeConsoleLogFilters(getContext().get(ConsoleLogFilter.class), new Filter(secrets, run.getCharset().name()))).
                     withCallback(new Callback2(unbinders)).
                     start();
         }

diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/CertificateMultiBinding.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/CertificateMultiBinding.java
@@ -105,7 +105,7 @@ public org.jenkinsci.plugins.credentialsbinding.MultiBinding.MultiEnvironment bi
 	}
 
 	@Override
-	public Set<String> variables() {
+	public Set<String> variables(Run<?, ?> build) {
 		Set<String> set = new HashSet<>();
 		set.add(keystoreVariable);
 		if (aliasVariable != null && !aliasVariable.isEmpty()) {

diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SSHUserPrivateKeyBinding.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SSHUserPrivateKeyBinding.java
@@ -76,10 +76,11 @@ public String getPassphraseVariable() {
         return SSHUserPrivateKey.class;
     }
 
-    @Override public Set<String> variables() {
+    @Override public Set<String> variables(Run<?, ?> build) throws CredentialNotFoundException {
+        SSHUserPrivateKey sshKey = getCredentials(build);
         Set<String> set = new HashSet<>();
         set.add(keyFileVariable);
-        if (usernameVariable != null) {
+        if (usernameVariable != null && sshKey.isUsernameSecret()) {
             set.add(usernameVariable);
         }
         if (passphraseVariable != null) {
@@ -101,21 +102,22 @@ public String getPassphraseVariable() {
         keyFile.write(contents.toString(), "UTF-8");
         keyFile.chmod(0400);
 
-        Map<String, String> map = new LinkedHashMap<>();
-        map.put(keyFileVariable, keyFile.getRemote());
+        Map<String, String> secretValues = new LinkedHashMap<>();
+        Map<String, String> publicValues = new LinkedHashMap<>();
+        secretValues.put(keyFileVariable, keyFile.getRemote());
         if (passphraseVariable != null) {
             Secret passphrase = sshKey.getPassphrase();
             if (passphrase != null) {
-                map.put(passphraseVariable, passphrase.getPlainText());
+                secretValues.put(passphraseVariable, passphrase.getPlainText());
             } else {
-                map.put(passphraseVariable, "");
+                secretValues.put(passphraseVariable, "");
             }
         }
         if (usernameVariable != null) {
-            map.put(usernameVariable, sshKey.getUsername());
+            (sshKey.isUsernameSecret() ? secretValues : publicValues).put(usernameVariable, sshKey.getUsername());
         }
 
-        return new MultiEnvironment(map, keyDir.getUnbinder());
+        return new MultiEnvironment(secretValues, publicValues, keyDir.getUnbinder());
     }
 
     @Symbol("sshUserPrivateKey")

diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java
@@ -88,7 +88,7 @@ public OutputStream decorateLogger(AbstractBuild build, OutputStream logger) thr
         for (MultiBinding binding : bindings) {
             MultiBinding.MultiEnvironment e = binding.bind(build, build.getWorkspace(), launcher, listener);
             m.add(e);
-            secrets.addAll(e.getValues().values());
+            secrets.addAll(e.getSecretValues().values());
         }
 
         if (!secrets.isEmpty()) {
@@ -98,7 +98,10 @@ public OutputStream decorateLogger(AbstractBuild build, OutputStream logger) thr
         return new Environment() {
             @Override public void buildEnvVars(Map<String,String> env) {
                 for (MultiBinding.MultiEnvironment e : m) {
-                    for (Map.Entry<String,String> pair : e.getValues().entrySet()) {
+                    for (Map.Entry<String,String> pair : e.getSecretValues().entrySet()) {
+                        env.put(pair.getKey(), pair.getValue()./* SECURITY-698 */replace("$", "$$$$"));
+                    }
+                    for (Map.Entry<String,String> pair : e.getPublicValues().entrySet()) {
                         env.put(pair.getKey(), pair.getValue()./* SECURITY-698 */replace("$", "$$$$"));
                     }
                 }
@@ -114,7 +117,11 @@ public OutputStream decorateLogger(AbstractBuild build, OutputStream logger) thr
 
     @Override public void makeSensitiveBuildVariables(AbstractBuild build, Set<String> sensitiveVariables) {
         for (MultiBinding binding : bindings) {
-            sensitiveVariables.addAll(binding.variables());
+            try {
+                sensitiveVariables.addAll(binding.variables(build));
+            } catch (CredentialNotFoundException x) {
+                // ignore here (will throw an error later anyway)
+            }
         }
     }
 

diff --git a/...main/java/org/jenkinsci/plugins/credentialsbinding/impl/UsernamePasswordMultiBinding.java b/...main/java/org/jenkinsci/plugins/credentialsbinding/impl/UsernamePasswordMultiBinding.java
@@ -31,9 +31,8 @@
 import hudson.model.Run;
 import hudson.model.TaskListener;
 import java.io.IOException;
-import java.util.Arrays;
-import java.util.HashSet;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Set;
 import org.jenkinsci.Symbol;
@@ -73,14 +72,21 @@ public String getPasswordVariable() {
                                            @Nullable Launcher launcher,
                                            @Nonnull TaskListener listener) throws IOException, InterruptedException {
         StandardUsernamePasswordCredentials credentials = getCredentials(build);
-        Map<String,String> m = new LinkedHashMap<>();
-        m.put(usernameVariable, credentials.getUsername());
-        m.put(passwordVariable, credentials.getPassword().getPlainText());
-        return new MultiEnvironment(m);
+        Map<String, String> secretValues = new LinkedHashMap<>();
+        Map<String, String> publicValues = new LinkedHashMap<>();
+        (credentials.isUsernameSecret() ? secretValues : publicValues).put(usernameVariable, credentials.getUsername());
+        secretValues.put(passwordVariable, credentials.getPassword().getPlainText());
+        return new MultiEnvironment(secretValues, publicValues);
     }
 
-    @Override public Set<String> variables() {
-        return new HashSet<String>(Arrays.asList(usernameVariable, passwordVariable));
+    @Override public Set<String> variables(Run<?, ?> build) throws CredentialNotFoundException {
+        StandardUsernamePasswordCredentials credentials = getCredentials(build);
+        Set<String> vars = new LinkedHashSet<>();
+        if (credentials.isUsernameSecret()) {
+            vars.add(usernameVariable);
+        }
+        vars.add(passwordVariable);
+        return vars;
     }
 
     @Symbol("usernamePassword")

diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java
@@ -146,7 +146,7 @@ public static class Execution extends AbstractSynchronousStepExecution<Void> {
                         + "}", true));
                 WorkflowRun b = p.scheduleBuild2(0).waitForStart();
                 SemaphoreStep.waitForStart("basics/1", b);
-                story.j.assertLogContains(Functions.isWindows() ? "Masking supported pattern matches of %USERNAME% or %PASSWORD%" : "Masking supported pattern matches of $USERNAME or $PASSWORD", b);
+                story.j.assertLogContains(Functions.isWindows() ? "Masking supported pattern matches of %PASSWORD%" : "Masking supported pattern matches of $PASSWORD", b);
             }
         });
         story.addStep(new Statement() {

diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/SSHUserPrivateKeyTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/SSHUserPrivateKeyTest.java
@@ -30,6 +30,7 @@
 import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.FilePath;
 import hudson.Functions;
+import hudson.security.ACL;
 import hudson.util.Secret;
 import org.jenkinsci.plugins.credentialsbinding.MultiBinding;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
@@ -47,16 +48,20 @@
 import java.util.*;
 
 import static org.junit.Assert.*;
+import org.junit.ClassRule;
+import org.jvnet.hudson.test.BuildWatcher;
 
 public class SSHUserPrivateKeyTest {
 
     @Rule public RestartableJenkinsRule story = new RestartableJenkinsRule();
+    @ClassRule public static BuildWatcher bw = new BuildWatcher();
     @Rule public TemporaryFolder tmp = new TemporaryFolder();
 
     private static class DummyPrivateKey extends BaseCredentials implements SSHUserPrivateKey, Serializable {
 
         private final String id;
         private final String user;
+        boolean usernameSecret = true;
         private final Secret passphrase;
         private final String keyContent;
 
@@ -96,6 +101,11 @@ public String getUsername() {
             return user;
         }
 
+        @Override
+        public boolean isUsernameSecret() {
+            return usernameSecret;
+        }
+
         @NonNull
         @Override
         public String getDescription() {
@@ -138,7 +148,6 @@ public CredentialsScope getScope() {
                 if (Functions.isWindows()) {
                     script =
                         "    bat '''\n"
-                        + "      @echo off\n"
                         + "      echo %THEUSER%:%THEPASS% > out.txt\n"
                         + "      type \"%THEKEY%\" > key.txt"
                         + "    '''\n";
@@ -169,6 +178,7 @@ public CredentialsScope getScope() {
                 SemaphoreStep.success("basics/1", null);
                 story.j.waitForCompletion(b);
                 story.j.assertBuildStatusSuccess(b);
+                story.j.assertLogNotContains(username, b);
                 story.j.assertLogNotContains(passphrase, b);
                 FilePath out = story.j.jenkins.getWorkspaceFor(p).child("out.txt");
                 assertTrue(out.exists());
@@ -177,6 +187,12 @@ public CredentialsScope getScope() {
                 FilePath key = story.j.jenkins.getWorkspaceFor(p).child("key.txt");
                 assertTrue(key.exists());
                 assertEquals(keyContent, key.readToString().trim());
+
+                ((DummyPrivateKey) CredentialsProvider.lookupCredentials(SSHUserPrivateKey.class, story.j.jenkins, ACL.SYSTEM, Collections.emptyList()).get(0)).usernameSecret = false;
+                SemaphoreStep.success("basics/2", null);
+                b = story.j.buildAndAssertSuccess(p);
+                story.j.assertLogContains(username, b);
+                story.j.assertLogNotContains(passphrase, b);
             }
         });
     }
@@ -193,7 +209,6 @@ public CredentialsScope getScope() {
                 if (Functions.isWindows()) {
                     script =
                         "    bat '''\n"
-                        + "      @echo off\n"
                         + "      type \"%THEKEY%\" > key.txt"
                         + "    '''\n";
                 } else {