From 927eb267d361d3307a9cb082cd2c00533c84d24b Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Tue, 9 Apr 2019 14:22:02 -0500 Subject: [PATCH 01/36] [JENKINS-42950] Support more credential masking scenarios This adds a new extension point, PatternMaskerProvider, that can be used to implement different quoting and unquoting strategies used by shells that can mangle passwords and other secrets in their output. This is used to improve credential masking in more scenarios where the secret was not intended to be seen. Co-Authored-By: Wadeck Follonier --- pom.xml | 4 +- .../credentialsbinding/MultiBinding.java | 35 -- .../credentialsbinding/impl/BindingStep.java | 3 +- .../impl/SecretBuildWrapper.java | 3 +- .../AbstractShellPatternMaskerProvider.java | 62 ++++ .../masking/BashPatternMaskerProvider.java | 64 ++++ .../masking/BatchPatternMaskerProvider.java | 85 +++++ .../masking/PatternMaskerProvider.java | 77 +++++ .../impl/BindingStep/help.html | 9 +- .../impl/CredentialsMaskingBashTest.java | 118 +++++++ .../impl/CredentialsMaskingBatchTest.java | 300 ++++++++++++++++++ .../CredentialsMaskingPowerShellTest.java | 139 ++++++++ .../BashPatternMaskerProviderTest.java | 84 +++++ .../test/ExecutableExists.java | 59 ++++ 14 files changed, 996 insertions(+), 46 deletions(-) create mode 100644 src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java create mode 100644 src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java create mode 100644 src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java create mode 100644 src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java create mode 100644 src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java create mode 100644 src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBatchTest.java create mode 100644 src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java create mode 100644 src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java create mode 100644 src/test/java/org/jenkinsci/plugins/credentialsbinding/test/ExecutableExists.java diff --git a/pom.xml b/pom.xml index 15763a73..d79d5abe 100644 --- a/pom.xml +++ b/pom.xml @@ -95,13 +95,13 @@ org.jenkins-ci.plugins.workflow workflow-durable-task-step - 2.5 + 2.12 test org.jenkins-ci.plugins durable-task - 1.13 + 1.14 test diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/MultiBinding.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/MultiBinding.java index d2d90ba1..b4f1f6d7 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/MultiBinding.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/MultiBinding.java @@ -37,22 +37,15 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.Serializable; -import java.util.ArrayList; -import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Set; -import java.util.regex.Pattern; import javax.annotation.Nonnull; import javax.annotation.Nullable; import jenkins.model.Jenkins; import org.jenkinsci.plugins.credentialsbinding.impl.CredentialNotFoundException; -import org.kohsuke.accmod.Restricted; -import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.DataBoundConstructor; /** @@ -169,32 +162,4 @@ public abstract MultiEnvironment bind(@Nonnull Run build, return (BindingDescriptor) super.getDescriptor(); } - private static final Comparator stringLengthComparator = new Comparator() { - @Override - public int compare(String o1, String o2) { - return o2.length() - o1.length(); - } - }; - - /** - * Utility method for turning a collection of secret strings into a single {@link String} for pattern compilation. - * @param secrets A collection of secret strings - * @return A {@link String} generated from that collection. - */ - @Restricted(NoExternalUse.class) - public static String getPatternStringForSecrets(Collection secrets) { - StringBuilder b = new StringBuilder(); - List sortedByLength = new ArrayList(secrets); - Collections.sort(sortedByLength, stringLengthComparator); - - for (String secret : sortedByLength) { - if (!secret.isEmpty()) { - if (b.length() > 0) { - b.append('|'); - } - b.append(Pattern.quote(secret)); - } - } - return b.toString(); - } } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java index c3079e9b..d13533e9 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java @@ -54,6 +54,7 @@ import org.apache.commons.codec.Charsets; import org.jenkinsci.plugins.credentialsbinding.MultiBinding; +import org.jenkinsci.plugins.credentialsbinding.masking.PatternMaskerProvider; import org.jenkinsci.plugins.workflow.steps.AbstractStepExecutionImpl; import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback; import org.jenkinsci.plugins.workflow.steps.BodyInvoker; @@ -195,7 +196,7 @@ private static final class Filter extends ConsoleLogFilter implements Serializab private String charsetName; Filter(Collection secrets, String charsetName) { - pattern = Secret.fromString(MultiBinding.getPatternStringForSecrets(secrets)); + pattern = Secret.fromString(PatternMaskerProvider.getMaskingPattern(secrets).pattern()); this.charsetName = charsetName; } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java index 8be9129a..6aefeae4 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java @@ -35,6 +35,7 @@ import hudson.tasks.BuildWrapper; import hudson.tasks.BuildWrapperDescriptor; import org.jenkinsci.plugins.credentialsbinding.MultiBinding; +import org.jenkinsci.plugins.credentialsbinding.masking.PatternMaskerProvider; import org.kohsuke.stapler.DataBoundConstructor; import javax.annotation.CheckForNull; @@ -60,7 +61,7 @@ public class SecretBuildWrapper extends BuildWrapper { */ public static @CheckForNull Pattern getPatternForBuild(@Nonnull AbstractBuild build) { if (secretsForBuild.containsKey(build)) { - return Pattern.compile(MultiBinding.getPatternStringForSecrets(secretsForBuild.get(build))); + return PatternMaskerProvider.getMaskingPattern(secretsForBuild.get(build)); } else { return null; } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java new file mode 100644 index 00000000..1e3a4ae0 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java @@ -0,0 +1,62 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import javax.annotation.Nonnull; +import java.util.Collection; +import java.util.HashSet; +import java.util.regex.Pattern; + +/** + * Convenient base class for PatternMaskerProvider implementations for shells. + */ +abstract class AbstractShellPatternMaskerProvider implements PatternMaskerProvider { + + /** + * Performs any quoting/escaping necessary for the input string to be able to be surrounded in quotes and passed + * literally to a shell. This should not include the surrounding quotes. + */ + abstract @Nonnull String getQuotedForm(@Nonnull String input); + + /** + * Surrounds a string with the proper quote characters to make a quoted string for a shell. + */ + abstract @Nonnull String surroundWithQuotes(@Nonnull String input); + + /** + * Performs the inverse of {@link #getQuotedForm(String)}. + */ + abstract @Nonnull String getUnquotedForm(@Nonnull String input); + + @Override + public @Nonnull Collection getAlternativeForms(@Nonnull String input) { + Collection patterns = new HashSet<>(); + String quotedForm = getQuotedForm(input); + patterns.add(Pattern.quote(quotedForm)); + patterns.add(Pattern.quote(surroundWithQuotes(quotedForm))); + patterns.add(Pattern.quote(getUnquotedForm(input))); + return patterns; + } +} diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java new file mode 100644 index 00000000..bf1d075b --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java @@ -0,0 +1,64 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import hudson.Extension; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.annotation.Nonnull; +import java.util.regex.Pattern; + +@Extension +@Restricted(NoExternalUse.class) +public class BashPatternMaskerProvider extends AbstractShellPatternMaskerProvider { + + private static final Pattern QUOTED_CHARS = Pattern.compile("(\\\\)(\\\\?)"); + + @Override + @Nonnull String getQuotedForm(@Nonnull String input) { + StringBuilder sb = new StringBuilder(input.length()); + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + if (c == '\'') { + sb.append("'\\''"); + } else { + sb.append(c); + } + } + return sb.toString(); + } + + @Override + @Nonnull String surroundWithQuotes(@Nonnull String input) { + return "'" + input + "'"; + } + + @Override + @Nonnull String getUnquotedForm(@Nonnull String input) { + return QUOTED_CHARS.matcher(input).replaceAll("$2"); + } + +} diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java new file mode 100644 index 00000000..8aef003b --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java @@ -0,0 +1,85 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import hudson.Extension; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.annotation.Nonnull; +import java.util.regex.Pattern; + +@Extension +@Restricted(NoExternalUse.class) +public class BatchPatternMaskerProvider extends AbstractShellPatternMaskerProvider { + + // adapted from: + // https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ + // also from WindowsUtil in Jenkins + + private static final Pattern CMD_METACHARS = Pattern.compile("[()%!^\"<>&|]"); + private static final Pattern QUOTED_CHARS = Pattern.compile("(\\^)(\\^?)"); + + @Override + @Nonnull String getQuotedForm(@Nonnull String input) { + int end = input.length(); + StringBuilder sb = new StringBuilder(end); + for (int i = 0; i < end; i++) { + int nrBackslashes = 0; + while (i < end && input.charAt(i) == '\\') { + i++; + nrBackslashes++; + } + + if (i == end) { + // backslashes at the end of the argument must be escaped so the terminate quote isn't + nrBackslashes = nrBackslashes * 2; + } else if (input.charAt(i) == '"') { + // backslashes preceding a quote all need to be escaped along with the quote + nrBackslashes = nrBackslashes * 2 + 1; + } + // else backslashes have no special meaning and don't need to be escaped here + + for (int j = 0; j < nrBackslashes; j++) { + sb.append('\\'); + } + + if (i < end) { + sb.append(input.charAt(i)); + } + } + return CMD_METACHARS.matcher(sb).replaceAll("^$0"); + } + + @Override + @Nonnull String surroundWithQuotes(@Nonnull String input) { + return '"' + input + '"'; + } + + @Override + @Nonnull String getUnquotedForm(@Nonnull String input) { + return QUOTED_CHARS.matcher(input).replaceAll("$2"); + } +} diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java new file mode 100644 index 00000000..0f097e14 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java @@ -0,0 +1,77 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import hudson.ExtensionList; +import hudson.ExtensionPoint; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.annotation.Nonnull; +import java.util.Collection; +import java.util.Comparator; +import java.util.TreeSet; +import java.util.regex.Pattern; + +/** + * Quotes strings according to quotation rules of various programs. Sometimes confused with escaping, quoting is done + * to pass literal strings in a shell or other interpreter. + * + * @since TODO + */ +public interface PatternMaskerProvider extends ExtensionPoint { + + @Nonnull Collection getAlternativeForms(@Nonnull String input); + + static @Nonnull ExtensionList all() { + return ExtensionList.lookup(PatternMaskerProvider.class); + } + + @Restricted(NoExternalUse.class) + static @Nonnull Collection getAllAlternateForms(@Nonnull String input) { + Collection alternateForms = new TreeSet<>(BY_REVERSE_LENGTH); + for (PatternMaskerProvider provider : all()) { + alternateForms.addAll(provider.getAlternativeForms(input)); + } + return alternateForms; + } + + @Restricted(NoExternalUse.class) + static @Nonnull Pattern getMaskingPattern(@Nonnull Collection inputs) { + Collection patterns = new TreeSet<>(BY_REVERSE_LENGTH); + for (String input : inputs) { + if (input.isEmpty()) { + continue; + } + patterns.add(Pattern.quote(input)); + patterns.addAll(getAllAlternateForms(input)); + } + return Pattern.compile(String.join("|", patterns)); + } + + @Restricted(NoExternalUse.class) Comparator BY_REVERSE_LENGTH = + Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo); + +} diff --git a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html index 577d226c..9aaac351 100644 --- a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html +++ b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html @@ -73,13 +73,8 @@ foo'bar

- Mangled secrets are not detected by Jenkins and will appear in plain text in the log file, in this case as: -

-
+ echo 'foo'\''bar'
-****
-

- This particular issue can be worked around by turning off echo with set +x, - or simply avoiding possible shell metacharacters in secrets. + Mangled secrets can only be detected on a best effort basis using the PatternMaskerProvider + extension point. Default providers include one for handling common ways that data may be encoded by Bash and Batch.

For bindings which store a secret file, beware that diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java new file mode 100644 index 00000000..a1fd5094 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java @@ -0,0 +1,118 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.impl; + +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.util.Secret; +import org.jenkinsci.plugins.credentialsbinding.masking.BashPatternMaskerProvider; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; +import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.junit.Before; +import org.junit.Rule; +import org.junit.experimental.theories.DataPoints; +import org.junit.experimental.theories.Theories; +import org.junit.experimental.theories.Theory; +import org.junit.runner.RunWith; +import org.jvnet.hudson.test.For; +import org.jvnet.hudson.test.JenkinsRule; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; +import java.util.concurrent.ThreadLocalRandom; + +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.startsWith; +import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; +import static org.junit.Assume.assumeThat; + +@RunWith(Theories.class) +@For(BashPatternMaskerProvider.class) +public class CredentialsMaskingBashTest { + + @DataPoints + public static List generatePasswords() { + ThreadLocalRandom random = ThreadLocalRandom.current(); + List passwords = new ArrayList<>(10); + for (int i = 0; i < 10; i++) { + int length = random.nextInt(8, 32); + StringBuilder sb = new StringBuilder(length); + for (int j = 0; j < length; j++) { + char next = (char) random.nextInt(0x20, 0x7F); // space to tilde inclusive + sb.append(next); + } + passwords.add(sb.toString()); + } + return passwords; + } + + @Rule public JenkinsRule j = new JenkinsRule(); + + private WorkflowJob project; + private String credentialsId; + + @Before + public void setUp() throws IOException { + assumeThat("bash", is(executable())); + project = j.createProject(WorkflowJob.class); + credentialsId = UUID.randomUUID().toString(); + project.setDefinition(new CpsFlowDefinition( + "node {\n" + + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + + " sh ': \"$CREDENTIALS\"'\n" + // : will expand its parameters and do nothing with them + " sh ': \"< $CREDENTIALS >\"'\n" + + " }\n" + + "}", true)); + } + + @Theory + public void credentialsAreMaskedInLogs(String credentials) throws Exception { + assumeThat(credentials, not(startsWith("****"))); + + registerCredentials(credentials); + WorkflowRun run = runProject(); + + j.assertLogContains(": ****", run); + j.assertLogContains(": '< **** >'", run); + j.assertLogNotContains(credentials, run); + } + + private void registerCredentials(String password) throws IOException { + StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, null, Secret.fromString(password)); + CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); + } + + private WorkflowRun runProject() throws Exception { + return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); + } + +} diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBatchTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBatchTest.java new file mode 100644 index 00000000..2808d052 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBatchTest.java @@ -0,0 +1,300 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.impl; + +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.Functions; +import hudson.util.Secret; +import org.jenkinsci.plugins.credentialsbinding.masking.BatchPatternMaskerProvider; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; +import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.For; +import org.jvnet.hudson.test.JenkinsRule; + +import java.io.IOException; +import java.util.UUID; + +import static org.junit.Assume.assumeTrue; + +@For(BatchPatternMaskerProvider.class) +public class CredentialsMaskingBatchTest { + + private static final String SIMPLE = "abcABC123"; + private static final String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; + private static final String ESCAPE = "^<^>^(^)^^^&^|"; + + // ALL_ASCII - [ < > " ' ^ $ | % ] + private static final String NON_DANGEROUS = "!#$*+,-./ 0123456789:;=? @ABCDEFGHIJKLMNO PQRSTUVWXYZ[\\]_ `abcdefghijklmno pqrstuvwxyz{}~"; + + // <>"'^$| + private static final String NON_DANGEROUS_IN_DOUBLE = "abc$ghi|jkl"; + + private static final String ALL_ASCII = "!\"#$%&'()*+,-./ 0123456789:;<=>? @ABCDEFGHIJKLMNO PQRSTUVWXYZ[\\]^_ `abcdefghijklmno pqrstuvwxyz{|}~"; + + @Rule + public JenkinsRule j = new JenkinsRule(); + + private WorkflowJob project; + + private String credentialPlainText; + private String credentialId; + + @Before + public void assumeWindowsForBatch() throws Exception { + assumeTrue(Functions.isWindows()); + } + + private void registerCredentials(String password) throws IOException { + this.credentialPlainText = password; + this.credentialId = UUID.randomUUID().toString(); + StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); + CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); + } + + private WorkflowRun runProject() throws Exception { + return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); + } + + @Test + public void simple_noQuote() throws Exception { + registerCredentials(SIMPLE); + assertDirectNoPlainTextButStars(runDirectNoQuote()); + } + + @Test + public void simple_singleQuote() throws Exception { + registerCredentials(SIMPLE); + assertDirectNoPlainTextButStars(runDirectSingleQuote()); + } + + @Test + public void simple_doubleQuote() throws Exception { + registerCredentials(SIMPLE); + assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + } + + @Test + public void simple_delayed() throws Exception { + registerCredentials(SIMPLE); + assertDelayedNoPlainTextButStars(runDelayedAllQuotes()); + } + + @Test + public void nonDangerous_noQuote() throws Exception { + registerCredentials(NON_DANGEROUS); + assertDirectNoPlainTextButStars(runDirectNoQuote()); + } + + @Test + public void nonDangerous_singleQuote() throws Exception { + registerCredentials(NON_DANGEROUS); + assertDirectNoPlainTextButStars(runDirectSingleQuote()); + } + + @Test + public void nonDangerous_doubleQuote() throws Exception { + registerCredentials(NON_DANGEROUS); + assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + } + + @Test + public void nonDangerous_delayed() throws Exception { + registerCredentials(NON_DANGEROUS); + assertDelayedNoPlainTextButStars(runDelayedAllQuotes()); + } + + @Test + @Ignore("Cannot support the dangerous characters in direct expressions") + public void allAscii_direct() throws Exception { + registerCredentials(ALL_ASCII); + assertDirectNoPlainTextButStars(runDirectNoQuote()); + assertDirectNoPlainTextButStars(runDirectSingleQuote()); + assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + } + + @Test + public void allAscii_delayed() throws Exception { + registerCredentials(ALL_ASCII); + assertDelayedNoPlainTextButStars(runDelayedAllQuotes()); + } + + @Test + @Ignore("Cannot support dangerous character & in non-delayed expansion without double quotes") + public void samplePassword_noQuote() throws Exception { + registerCredentials(SAMPLE_PASSWORD); + assertDirectNoPlainTextButStars(runDirectNoQuote()); + } + + @Test + @Ignore("Cannot support dangerous character & in non-delayed expansion without double quotes") + public void samplePassword_singleQuote() throws Exception { + registerCredentials(SAMPLE_PASSWORD); + assertDirectNoPlainTextButStars(runDirectSingleQuote()); + } + + @Test + public void samplePassword_doubleQuote() throws Exception { + registerCredentials(SAMPLE_PASSWORD); + assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + } + + @Test + public void samplePassword_delayed() throws Exception { + registerCredentials(SAMPLE_PASSWORD); + assertDelayedNoPlainTextButStars(runDelayedAllQuotes()); + } + + @Test + public void escape_noQuote() throws Exception { + registerCredentials(ESCAPE); + + WorkflowRun run = runDirectNoQuote(); + j.assertLogNotContains(credentialPlainText, run); + j.assertLogContains("before1 **** after1", run); + j.assertLogContains("before2 **** after2", run); + } + + @Test + public void escape_singleQuote() throws Exception { + registerCredentials(ESCAPE); + + WorkflowRun run = runDirectSingleQuote(); + j.assertLogNotContains(credentialPlainText, run); + j.assertLogContains("before1 **** after1", run); + j.assertLogContains("before2 **** after2", run); + } + + @Test + public void escape_doubleQuote() throws Exception { + registerCredentials(ESCAPE); + assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + } + + @Test + public void escape_delayed() throws Exception { + registerCredentials(ESCAPE); + assertDelayedNoPlainTextButStars(runDelayedAllQuotes()); + } + + // special cases + @Test + public void dangerousOutOfDouble_doubleQuote() throws Exception { + registerCredentials(NON_DANGEROUS_IN_DOUBLE); + assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + } + + private void assertDirectNoPlainTextButStars(WorkflowRun run) throws Exception { + j.assertLogNotContains(credentialPlainText, run); + j.assertLogContains("before1 **** after1", run); + j.assertLogContains("before2 **** after2", run); + } + + private void assertDelayedNoPlainTextButStars(WorkflowRun run) throws Exception { + j.assertLogNotContains(credentialPlainText, run); + j.assertLogContains("before1 **** after1", run); + j.assertLogContains("before2 **** after2", run); + j.assertLogContains("before3 **** after3", run); + } + + private WorkflowRun runDirectNoQuote() throws Exception { + // DO NOT DO THIS IN PRODUCTION + // these commands would be completely broken if echo didn't work the way it does when CREDENTIALS contains special characters + setupProject("node {\n" + + " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + + " bat \"\"\"\n" + + " echo before1 $CREDENTIALS after1\n" + // DO NOT DO THIS IN PRODUCTION; IT IS USING THE WRONG VARIABLE SYNTAX + " \"\"\"\n" + + " bat '''\n" + + " echo before2 %CREDENTIALS% after2\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + " '''\n" + + " }\n" + + "}" + ); + return project.scheduleBuild2(0).get(); + } + + private WorkflowRun runDirectSingleQuote() throws Exception { + // DO NOT DO THIS IN PRODUCTION + // single quotes do not mean anything in batch scripts; use double quotes for escaping/quoting strings + setupProject("node {\n" + + " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + + " bat \"\"\"\n" + + " echo 'before1 $CREDENTIALS after1'\n" + // DO NOT DO THIS IN PRODUCTION; IT IS USING THE WRONG VARIABLE SYNTAX + " \"\"\"\n" + + " bat '''\n" + + " echo 'before2 %CREDENTIALS% after2'\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + " '''\n" + + " }\n" + + "}" + ); + return project.scheduleBuild2(0).get(); + } + + private WorkflowRun runDirectDoubleQuote() throws Exception { + setupProject("node {\n" + + " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + + " bat \"\"\"\n" + + " echo \"before1 $CREDENTIALS after1\"\n" + // DO NOT DO THIS IN PRODUCTION; IT IS USING THE WRONG VARIABLE SYNTAX + " \"\"\"\n" + + " bat '''\n" + + " echo \"before2 %CREDENTIALS% after2\"\n" + // THIS ONE IS OK THOUGH + " '''\n" + + " }\n" + + "}" + ); + return project.scheduleBuild2(0).get(); + } + + private WorkflowRun runDelayedAllQuotes() throws Exception { + setupProject("node {\n" + + " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + + " bat '''\n" + + " SETLOCAL EnableDelayedExpansion\n" + + " echo before1 !CREDENTIALS! after1\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + " echo 'before2 !CREDENTIALS! after2'\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + " echo \"before3 !CREDENTIALS! after3\"\n" + // THIS ONE IS OK THOUGH + " '''\n" + + " }\n" + + "}" + ); + return project.scheduleBuild2(0).get(); + } + + private void setupProject(String pipeline) throws Exception { + String projectName = UUID.randomUUID().toString(); + project = j.jenkins.createProject(WorkflowJob.class, projectName); + credentialId = UUID.randomUUID().toString(); + project.setDefinition(new CpsFlowDefinition(pipeline, true)); + } +} diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java new file mode 100644 index 00000000..2783cd4e --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java @@ -0,0 +1,139 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.impl; + +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.util.Secret; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; +import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +import java.io.IOException; +import java.util.UUID; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; +import static org.junit.Assert.assertThat; +import static org.junit.Assume.assumeThat; + +public class CredentialsMaskingPowerShellTest { + + private static final String SIMPLE = "abcABC123"; + private static final String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; + private static final String ALL_ASCII = "!\"#$%&'()*+,-./ 0123456789:;<=>? @ABCDEFGHIJKLMNO PQRSTUVWXYZ[\\]^_ `abcdefghijklmno pqrstuvwxyz{|}~"; + + @Rule + public JenkinsRule j = new JenkinsRule(); + + private WorkflowJob project; + + private String credentialPlainText; + private String credentialId; + + @Before + public void assumeWindowsForBatch() throws Exception { + // TODO: pwsh is also a valid executable name + assumeThat("powershell", is(executable())); + } + + private void registerCredentials(String password) throws IOException { + this.credentialPlainText = password; + this.credentialId = UUID.randomUUID().toString(); + StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); + CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); + } + + @Test + public void simple() throws Exception { + registerCredentials(SIMPLE); + assertDirectNoPlainTextButStars(runPowerShellInterpretation()); + } + + @Test + public void allAscii() throws Exception { + registerCredentials(ALL_ASCII); + assertDirectNoPlainTextButStars(runPowerShellInterpretation()); + } + + @Test + public void samplePassword() throws Exception { + registerCredentials(SAMPLE_PASSWORD); + assertDirectNoPlainTextButStars(runPowerShellInterpretation()); + } + + private void assertDirectNoPlainTextButStars(WorkflowRun run) throws Exception { + j.assertLogNotContains(credentialPlainText, run); + // powershell x y z => output in 3 different lines + assertStringPresentInOrder(run, "before1", "****", "after1"); + j.assertLogContains("before2 **** after2", run); + } + + private void assertStringPresentInOrder(WorkflowRun run, String... values) throws Exception { + String fullLog = run.getLog(); + int currentIndex = 0; + for (int i = 0; i < values.length; i++) { + String currentValue = values[i]; + int nextIndex = fullLog.indexOf(currentValue, currentIndex); + if(nextIndex == -1){ + // use assertThat to have better output + assertThat(fullLog.substring(currentIndex), containsString(currentValue)); + }else{ + currentIndex = nextIndex + currentValue.length(); + } + } + } + + private WorkflowRun runPowerShellInterpretation() throws Exception { + setupProject("node {\n" + + " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + + // interpreted by PowerShell + " powershell '''\n" + + // echo is an alias for Write-Output + " echo before1 $env:CREDENTIALS after1\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + // echo '...' does not let PowerShell to interpret the information + " echo \"before2 $env:CREDENTIALS after2\"\n" + // THIS IS OK THOUGH + " '''\n" + + " }\n" + + "}" + ); + return project.scheduleBuild2(0).get(); + } + + private void setupProject(String pipeline) throws Exception { + String projectName = UUID.randomUUID().toString(); + project = j.jenkins.createProject(WorkflowJob.class, projectName); + credentialId = UUID.randomUUID().toString(); + project.setDefinition(new CpsFlowDefinition(pipeline, true)); + } +} diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java new file mode 100644 index 00000000..45ecb20a --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -0,0 +1,84 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.util.Secret; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; +import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +import java.io.IOException; +import java.util.UUID; + +import static org.hamcrest.Matchers.is; +import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; +import static org.junit.Assume.assumeThat; + +public class BashPatternMaskerProviderTest { + + public @Rule JenkinsRule j = new JenkinsRule(); + + @Before + public void setUp() { + assumeThat("bash", is(executable())); + } + + @Test + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + public void testSecretsWithBackslashesStillMaskedWhenUsedWithoutProperQuoting() throws Exception { + WorkflowJob project = j.createProject(WorkflowJob.class); + String password = "foo\\bar\\"; + String credentialsId = registerStringCredentials(password); + project.setDefinition(new CpsFlowDefinition( + "node {\n" + + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + + " sh ': $CREDENTIALS'\n" + // forgot quotes + " sh \": $CREDENTIALS\"\n" + // using groovy variable and forgot quotes + " }\n" + + "}", true)); + + WorkflowRun run = j.assertBuildStatusSuccess(project.scheduleBuild2(0)); + + j.assertLogContains(": ****", run); + j.assertLogNotContains(password, run); + j.assertLogNotContains("foobar", run); + } + + private String registerStringCredentials(String password) throws IOException { + String credentialId = UUID.randomUUID().toString(); + StringCredentials creds = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); + CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), creds); + return credentialId; + } +} diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/ExecutableExists.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/ExecutableExists.java new file mode 100644 index 00000000..28ca2ea7 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/ExecutableExists.java @@ -0,0 +1,59 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.test; + +import hudson.Functions; +import org.hamcrest.Description; +import org.hamcrest.Matcher; +import org.hamcrest.TypeSafeMatcher; + +import java.io.IOException; + +/** + * Hamcrest matcher for strings to check if the given command is executable. + * This uses {@code which} or {@code where.exe} to determine this depending on operating system. + */ +public class ExecutableExists extends TypeSafeMatcher { + + public static Matcher executable() { + return new ExecutableExists(); + } + + private static final String LOCATOR = Functions.isWindows() ? "where.exe" : "which"; + + @Override + protected boolean matchesSafely(String item) { + try { + return new ProcessBuilder(LOCATOR, item).start().waitFor() == 0; + } catch (InterruptedException | IOException e) { + return false; + } + } + + @Override + public void describeTo(Description description) { + description.appendText("executable"); + } +} From 3fb359e25fcb91de2491c496364a6c7271786123 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:14:03 -0500 Subject: [PATCH 02/36] Improve assertions Signed-off-by: Matt Sicker --- .../masking/BashPatternMaskerProviderTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 45ecb20a..80c2ec31 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -72,7 +72,8 @@ public void testSecretsWithBackslashesStillMaskedWhenUsedWithoutProperQuoting() j.assertLogContains(": ****", run); j.assertLogNotContains(password, run); - j.assertLogNotContains("foobar", run); + j.assertLogNotContains("foo", run); + j.assertLogNotContains("bar", run); } private String registerStringCredentials(String password) throws IOException { From ee512983234ab831c822447c606027883aea4fc9 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:14:53 -0500 Subject: [PATCH 03/36] Move package and add link to relevant PR Signed-off-by: Matt Sicker --- .../{impl => masking}/CredentialsMaskingPowerShellTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/{impl => masking}/CredentialsMaskingPowerShellTest.java (97%) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/CredentialsMaskingPowerShellTest.java similarity index 97% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/CredentialsMaskingPowerShellTest.java index 2783cd4e..67814cff 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/CredentialsMaskingPowerShellTest.java @@ -22,7 +22,7 @@ * THE SOFTWARE. */ -package org.jenkinsci.plugins.credentialsbinding.impl; +package org.jenkinsci.plugins.credentialsbinding.masking; import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; @@ -64,6 +64,7 @@ public class CredentialsMaskingPowerShellTest { @Before public void assumeWindowsForBatch() throws Exception { // TODO: pwsh is also a valid executable name + // https://github.com/jenkinsci/durable-task-plugin/pull/88 assumeThat("powershell", is(executable())); } From 22733560e39a2ff225bd12cb9e5b09e27f78740b Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:15:02 -0500 Subject: [PATCH 04/36] Rename test Signed-off-by: Matt Sicker --- .../BatchPatternMaskerProviderTest.java} | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/{impl/CredentialsMaskingBatchTest.java => masking/BatchPatternMaskerProviderTest.java} (97%) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBatchTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java similarity index 97% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBatchTest.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java index 2808d052..56ee634d 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBatchTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java @@ -22,14 +22,13 @@ * THE SOFTWARE. */ -package org.jenkinsci.plugins.credentialsbinding.impl; +package org.jenkinsci.plugins.credentialsbinding.masking; import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.domains.Domain; import hudson.Functions; import hudson.util.Secret; -import org.jenkinsci.plugins.credentialsbinding.masking.BatchPatternMaskerProvider; import org.jenkinsci.plugins.plaincredentials.StringCredentials; import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; @@ -39,7 +38,6 @@ import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; -import org.jvnet.hudson.test.For; import org.jvnet.hudson.test.JenkinsRule; import java.io.IOException; @@ -47,8 +45,7 @@ import static org.junit.Assume.assumeTrue; -@For(BatchPatternMaskerProvider.class) -public class CredentialsMaskingBatchTest { +public class BatchPatternMaskerProviderTest { private static final String SIMPLE = "abcABC123"; private static final String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; From 19fec43f41c534e65f8f27b48eb3b5d938e74135 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:24:52 -0500 Subject: [PATCH 05/36] Rename test Signed-off-by: Matt Sicker --- ...ingPowerShellTest.java => PowerShellMaskerProviderTest.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/{CredentialsMaskingPowerShellTest.java => PowerShellMaskerProviderTest.java} (99%) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/CredentialsMaskingPowerShellTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java similarity index 99% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/CredentialsMaskingPowerShellTest.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java index 67814cff..89f1b591 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/CredentialsMaskingPowerShellTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java @@ -47,7 +47,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assume.assumeThat; -public class CredentialsMaskingPowerShellTest { +public class PowerShellMaskerProviderTest { private static final String SIMPLE = "abcABC123"; private static final String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; From 6dcc230a3d4c81832b1d524239fdd8d6e9488dab Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:26:23 -0500 Subject: [PATCH 06/36] Rename tests and improve some Signed-off-by: Matt Sicker --- .../BashPatternMaskerProvider2Test.java} | 81 ++++++------------- .../BashPatternMaskerProviderTest.java | 78 +++++++++++++----- 2 files changed, 80 insertions(+), 79 deletions(-) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/{impl/CredentialsMaskingBashTest.java => masking/BashPatternMaskerProvider2Test.java} (50%) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java similarity index 50% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java index a1fd5094..1e75be42 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java @@ -22,13 +22,12 @@ * THE SOFTWARE. */ -package org.jenkinsci.plugins.credentialsbinding.impl; +package org.jenkinsci.plugins.credentialsbinding.masking; import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.domains.Domain; import hudson.util.Secret; -import org.jenkinsci.plugins.credentialsbinding.masking.BashPatternMaskerProvider; import org.jenkinsci.plugins.plaincredentials.StringCredentials; import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; @@ -36,83 +35,51 @@ import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.junit.Before; import org.junit.Rule; -import org.junit.experimental.theories.DataPoints; -import org.junit.experimental.theories.Theories; -import org.junit.experimental.theories.Theory; -import org.junit.runner.RunWith; -import org.jvnet.hudson.test.For; +import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; import java.io.IOException; -import java.util.ArrayList; -import java.util.List; import java.util.UUID; -import java.util.concurrent.ThreadLocalRandom; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.startsWith; import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; import static org.junit.Assume.assumeThat; -@RunWith(Theories.class) -@For(BashPatternMaskerProvider.class) -public class CredentialsMaskingBashTest { +public class BashPatternMaskerProvider2Test { - @DataPoints - public static List generatePasswords() { - ThreadLocalRandom random = ThreadLocalRandom.current(); - List passwords = new ArrayList<>(10); - for (int i = 0; i < 10; i++) { - int length = random.nextInt(8, 32); - StringBuilder sb = new StringBuilder(length); - for (int j = 0; j < length; j++) { - char next = (char) random.nextInt(0x20, 0x7F); // space to tilde inclusive - sb.append(next); - } - passwords.add(sb.toString()); - } - return passwords; - } - - @Rule public JenkinsRule j = new JenkinsRule(); - - private WorkflowJob project; - private String credentialsId; + public @Rule JenkinsRule j = new JenkinsRule(); @Before - public void setUp() throws IOException { + public void setUp() { assumeThat("bash", is(executable())); - project = j.createProject(WorkflowJob.class); - credentialsId = UUID.randomUUID().toString(); + } + + @Test + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + public void testSecretsWithBackslashesStillMaskedWhenUsedWithoutProperQuoting() throws Exception { + WorkflowJob project = j.createProject(WorkflowJob.class); + String password = "foo\\bar\\"; + String credentialsId = registerStringCredentials(password); project.setDefinition(new CpsFlowDefinition( "node {\n" + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + - " sh ': \"$CREDENTIALS\"'\n" + // : will expand its parameters and do nothing with them - " sh ': \"< $CREDENTIALS >\"'\n" + + " sh ': $CREDENTIALS'\n" + // forgot quotes + " sh \": $CREDENTIALS\"\n" + // using groovy variable and forgot quotes " }\n" + "}", true)); - } - - @Theory - public void credentialsAreMaskedInLogs(String credentials) throws Exception { - assumeThat(credentials, not(startsWith("****"))); - registerCredentials(credentials); - WorkflowRun run = runProject(); + WorkflowRun run = j.assertBuildStatusSuccess(project.scheduleBuild2(0)); j.assertLogContains(": ****", run); - j.assertLogContains(": '< **** >'", run); - j.assertLogNotContains(credentials, run); + j.assertLogNotContains(password, run); + j.assertLogNotContains("foo", run); + j.assertLogNotContains("bar", run); } - private void registerCredentials(String password) throws IOException { - StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, null, Secret.fromString(password)); - CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); + private String registerStringCredentials(String password) throws IOException { + String credentialId = UUID.randomUUID().toString(); + StringCredentials creds = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); + CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), creds); + return credentialId; } - - private WorkflowRun runProject() throws Exception { - return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); - } - } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 80c2ec31..abe70647 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -35,51 +35,85 @@ import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.junit.Before; import org.junit.Rule; -import org.junit.Test; +import org.junit.experimental.theories.DataPoint; +import org.junit.experimental.theories.DataPoints; +import org.junit.experimental.theories.Theories; +import org.junit.experimental.theories.Theory; +import org.junit.runner.RunWith; import org.jvnet.hudson.test.JenkinsRule; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Random; import java.util.UUID; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.startsWith; import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; import static org.junit.Assume.assumeThat; +@RunWith(Theories.class) public class BashPatternMaskerProviderTest { - public @Rule JenkinsRule j = new JenkinsRule(); + public static final @DataPoint String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; + public static final @DataPoint String ANOTHER_SAMPLE_PASSWORD = "a'b\"c\\d(e)#"; - @Before - public void setUp() { - assumeThat("bash", is(executable())); + @DataPoints + public static List generatePasswords() { + Random random = new Random(100); + List passwords = new ArrayList<>(10); + for (int i = 0; i < 10; i++) { + int length = random.nextInt(24) + 8; + StringBuilder sb = new StringBuilder(length); + for (int j = 0; j < length; j++) { + char next = (char) (random.nextInt('~' - ' ' + 1) + ' '); // space = 0x20, tilde = 0x7E + sb.append(next); + } + passwords.add(sb.toString()); + } + return passwords; } - @Test - // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG - public void testSecretsWithBackslashesStillMaskedWhenUsedWithoutProperQuoting() throws Exception { - WorkflowJob project = j.createProject(WorkflowJob.class); - String password = "foo\\bar\\"; - String credentialsId = registerStringCredentials(password); + @Rule public JenkinsRule j = new JenkinsRule(); + + private WorkflowJob project; + private String credentialsId; + + @Before + public void setUp() throws IOException { + assumeThat("bash", is(executable())); + project = j.createProject(WorkflowJob.class); + credentialsId = UUID.randomUUID().toString(); project.setDefinition(new CpsFlowDefinition( "node {\n" + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + - " sh ': $CREDENTIALS'\n" + // forgot quotes - " sh \": $CREDENTIALS\"\n" + // using groovy variable and forgot quotes + " sh ': \"$CREDENTIALS\"'\n" + // : will expand its parameters and do nothing with them + " sh ': \"< $CREDENTIALS >\"'\n" + " }\n" + "}", true)); + } + + @Theory + public void credentialsAreMaskedInLogs(String credentials) throws Exception { + assumeThat(credentials, not(startsWith("****"))); - WorkflowRun run = j.assertBuildStatusSuccess(project.scheduleBuild2(0)); + registerCredentials(credentials); + WorkflowRun run = runProject(); j.assertLogContains(": ****", run); - j.assertLogNotContains(password, run); - j.assertLogNotContains("foo", run); - j.assertLogNotContains("bar", run); + j.assertLogContains(": '< **** >'", run); + j.assertLogNotContains(credentials, run); } - private String registerStringCredentials(String password) throws IOException { - String credentialId = UUID.randomUUID().toString(); - StringCredentials creds = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); - CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), creds); - return credentialId; + private void registerCredentials(String password) throws IOException { + StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, null, Secret.fromString(password)); + CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); } + + private WorkflowRun runProject() throws Exception { + return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); + } + } From cd94206ab507ccded5aa9ebf76583e01c15e3dad Mon Sep 17 00:00:00 2001 From: Daniel Beck <1831569+daniel-beck@users.noreply.github.com> Date: Fri, 3 May 2019 11:27:55 -0500 Subject: [PATCH 07/36] Improve help docs Co-Authored-By: jvz --- .../plugins/credentialsbinding/impl/BindingStep/help.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html index 9aaac351..dbe40bb3 100644 --- a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html +++ b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html @@ -74,7 +74,7 @@

Mangled secrets can only be detected on a best effort basis using the PatternMaskerProvider - extension point. Default providers include one for handling common ways that data may be encoded by Bash and Batch. + extension point. By default, Jenkins will attempt to mask mangled secrets as they would appear in output of Bash and Windows batch.

For bindings which store a secret file, beware that From c82bcc371a57aba1a42e45022582af7dc74fd6b2 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:30:22 -0500 Subject: [PATCH 08/36] Simplify set class usage Signed-off-by: Matt Sicker --- .../masking/PatternMaskerProvider.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java index 0f097e14..43c22af6 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java @@ -32,6 +32,7 @@ import javax.annotation.Nonnull; import java.util.Collection; import java.util.Comparator; +import java.util.HashSet; import java.util.TreeSet; import java.util.regex.Pattern; @@ -51,7 +52,7 @@ public interface PatternMaskerProvider extends ExtensionPoint { @Restricted(NoExternalUse.class) static @Nonnull Collection getAllAlternateForms(@Nonnull String input) { - Collection alternateForms = new TreeSet<>(BY_REVERSE_LENGTH); + Collection alternateForms = new HashSet<>(); for (PatternMaskerProvider provider : all()) { alternateForms.addAll(provider.getAlternativeForms(input)); } @@ -60,7 +61,7 @@ public interface PatternMaskerProvider extends ExtensionPoint { @Restricted(NoExternalUse.class) static @Nonnull Pattern getMaskingPattern(@Nonnull Collection inputs) { - Collection patterns = new TreeSet<>(BY_REVERSE_LENGTH); + Collection patterns = new TreeSet<>(Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo)); for (String input : inputs) { if (input.isEmpty()) { continue; @@ -70,8 +71,4 @@ public interface PatternMaskerProvider extends ExtensionPoint { } return Pattern.compile(String.join("|", patterns)); } - - @Restricted(NoExternalUse.class) Comparator BY_REVERSE_LENGTH = - Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo); - } From fa4ff53f1dd67edd1de265f3801bbd95cfb5c8b5 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:43:33 -0500 Subject: [PATCH 09/36] Improve javadoc Signed-off-by: Matt Sicker --- .../masking/PatternMaskerProvider.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java index 43c22af6..18fd8ae3 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java @@ -44,12 +44,24 @@ */ public interface PatternMaskerProvider extends ExtensionPoint { + /** + * Returns a collection of alternative forms as regular expressions the given input may show up as in logs. These + * patterns should be appropriately {@linkplain Pattern#quote(String) quoted} for matching literal patterns as + * the resulting strings are formed into a regular expression. + */ @Nonnull Collection getAlternativeForms(@Nonnull String input); + /** + * Returns all PatternMaskerProvider extensions known at runtime. + */ static @Nonnull ExtensionList all() { return ExtensionList.lookup(PatternMaskerProvider.class); } + /** + * Returns a collection of all alternative form regular expressions for the given input using all PatternMaskerProvider + * extensions. + */ @Restricted(NoExternalUse.class) static @Nonnull Collection getAllAlternateForms(@Nonnull String input) { Collection alternateForms = new HashSet<>(); @@ -59,6 +71,15 @@ public interface PatternMaskerProvider extends ExtensionPoint { return alternateForms; } + /** + * Constructs a regular expression to match against all known forms that the given collection of input strings may + * appear. This pattern is optimized such that longer masks are checked before shorter masks. By doing so, this + * makes inputs that are substrings of other inputs be masked as the longer input, though there is no guarantee + * this will work in every possible situation. Another consequence of this ordering is that inputs that require + * quoting will be masked before a substring of the input was matched, thus avoiding leakage of information. + * For example, {@code bash -x} will only quote arguments echoed when necessary. To avoid leaking the presence or + * absence of quoting, the longer form is masked. + */ @Restricted(NoExternalUse.class) static @Nonnull Pattern getMaskingPattern(@Nonnull Collection inputs) { Collection patterns = new TreeSet<>(Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo)); From 5f55aca5f892f14e6ec77d4b21a6a79300a91502 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 11:47:27 -0500 Subject: [PATCH 10/36] Improve help docs Signed-off-by: Matt Sicker --- .../plugins/credentialsbinding/impl/BindingStep/help.html | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html index dbe40bb3..c405a910 100644 --- a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html +++ b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html @@ -73,9 +73,12 @@ foo'bar

- Mangled secrets can only be detected on a best effort basis using the PatternMaskerProvider - extension point. By default, Jenkins will attempt to mask mangled secrets as they would appear in output of Bash and Windows batch. + Mangled secrets can only be detected on a best effort basis. By default, Jenkins will attempt to mask mangled + secrets as they would appear in output of Bash and Windows batch. Without these strategies in place, mangled secrets + would appear in plain text in log files. In the example above, this would result in:

+
+ echo 'foo'\''bar'
+****

For bindings which store a secret file, beware that

From a43297407c1b97e3b364719234e18ab580ce3f0f Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 12:23:20 -0500 Subject: [PATCH 11/36] Add batch test for fancier password Signed-off-by: Matt Sicker --- .../BatchPatternMaskerProviderTest.java | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java index 56ee634d..ca557fef 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java @@ -57,6 +57,9 @@ public class BatchPatternMaskerProviderTest { // <>"'^$| private static final String NON_DANGEROUS_IN_DOUBLE = "abc$ghi|jkl"; + // quoted form: "^^|\a\\" + private static final String NEEDS_QUOTING = "^|\\a\\"; + private static final String ALL_ASCII = "!\"#$%&'()*+,-./ 0123456789:;<=>? @ABCDEFGHIJKLMNO PQRSTUVWXYZ[\\]^_ `abcdefghijklmno pqrstuvwxyz{|}~"; @Rule @@ -211,6 +214,12 @@ public void dangerousOutOfDouble_doubleQuote() throws Exception { assertDirectNoPlainTextButStars(runDirectDoubleQuote()); } + @Test + public void needsQuoting_doubleQuote() throws Exception { + registerCredentials(NEEDS_QUOTING); + assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + } + private void assertDirectNoPlainTextButStars(WorkflowRun run) throws Exception { j.assertLogNotContains(credentialPlainText, run); j.assertLogContains("before1 **** after1", run); @@ -238,7 +247,7 @@ private WorkflowRun runDirectNoQuote() throws Exception { " }\n" + "}" ); - return project.scheduleBuild2(0).get(); + return runProject(); } private WorkflowRun runDirectSingleQuote() throws Exception { @@ -255,7 +264,7 @@ private WorkflowRun runDirectSingleQuote() throws Exception { " }\n" + "}" ); - return project.scheduleBuild2(0).get(); + return runProject(); } private WorkflowRun runDirectDoubleQuote() throws Exception { @@ -270,7 +279,7 @@ private WorkflowRun runDirectDoubleQuote() throws Exception { " }\n" + "}" ); - return project.scheduleBuild2(0).get(); + return runProject(); } private WorkflowRun runDelayedAllQuotes() throws Exception { @@ -278,14 +287,14 @@ private WorkflowRun runDelayedAllQuotes() throws Exception { " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + " bat '''\n" + " SETLOCAL EnableDelayedExpansion\n" + - " echo before1 !CREDENTIALS! after1\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG - " echo 'before2 !CREDENTIALS! after2'\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG - " echo \"before3 !CREDENTIALS! after3\"\n" + // THIS ONE IS OK THOUGH + " echo before1 !CREDENTIALS! after1\n" + + " echo 'before2 !CREDENTIALS! after2'\n" + + " echo \"before3 !CREDENTIALS! after3\"\n" + " '''\n" + " }\n" + "}" ); - return project.scheduleBuild2(0).get(); + return runProject(); } private void setupProject(String pipeline) throws Exception { From 0dde12f05580fec773f8e7d4adffc4683060c2df Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 13:31:41 -0500 Subject: [PATCH 12/36] Fix assertion Signed-off-by: Matt Sicker --- .../masking/BashPatternMaskerProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index abe70647..0870b8e0 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -103,7 +103,7 @@ public void credentialsAreMaskedInLogs(String credentials) throws Exception { WorkflowRun run = runProject(); j.assertLogContains(": ****", run); - j.assertLogContains(": '< **** >'", run); + j.assertLogContains("< **** >", run); j.assertLogNotContains(credentials, run); } From 30d0900dd10d26fe4fbd79a3378327bee64e3320 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 15:23:04 -0500 Subject: [PATCH 13/36] Improve tests to work with bash properly Signed-off-by: Matt Sicker --- .../BashPatternMaskerProvider2Test.java | 5 +- .../BashPatternMaskerProviderTest.java | 7 ++- .../masking/PowerShellMaskerProviderTest.java | 2 +- ...ExecutableExists.java => Executables.java} | 46 +++++++++++-------- 4 files changed, 36 insertions(+), 24 deletions(-) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/test/{ExecutableExists.java => Executables.java} (57%) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java index 1e75be42..70aa935b 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java @@ -27,7 +27,9 @@ import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.tasks.Shell; import hudson.util.Secret; +import org.jenkinsci.plugins.credentialsbinding.test.Executables; import org.jenkinsci.plugins.plaincredentials.StringCredentials; import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; @@ -42,7 +44,7 @@ import java.util.UUID; import static org.hamcrest.Matchers.is; -import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; +import static org.jenkinsci.plugins.credentialsbinding.test.Executables.executable; import static org.junit.Assume.assumeThat; public class BashPatternMaskerProvider2Test { @@ -52,6 +54,7 @@ public class BashPatternMaskerProvider2Test { @Before public void setUp() { assumeThat("bash", is(executable())); + j.jenkins.getDescriptorByType(Shell.DescriptorImpl.class).setShell(Executables.getPathToExecutable("bash")); } @Test diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 0870b8e0..9d6a174c 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -27,7 +27,9 @@ import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.tasks.Shell; import hudson.util.Secret; +import org.jenkinsci.plugins.credentialsbinding.test.Executables; import org.jenkinsci.plugins.plaincredentials.StringCredentials; import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; @@ -51,7 +53,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; -import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; +import static org.jenkinsci.plugins.credentialsbinding.test.Executables.executable; import static org.junit.Assume.assumeThat; @RunWith(Theories.class) @@ -84,6 +86,7 @@ public static List generatePasswords() { @Before public void setUp() throws IOException { assumeThat("bash", is(executable())); + j.jenkins.getDescriptorByType(Shell.DescriptorImpl.class).setShell(Executables.getPathToExecutable("bash")); project = j.createProject(WorkflowJob.class); credentialsId = UUID.randomUUID().toString(); project.setDefinition(new CpsFlowDefinition( @@ -103,7 +106,7 @@ public void credentialsAreMaskedInLogs(String credentials) throws Exception { WorkflowRun run = runProject(); j.assertLogContains(": ****", run); - j.assertLogContains("< **** >", run); + j.assertLogContains(": '< **** >'", run); j.assertLogNotContains(credentials, run); } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java index 89f1b591..e32ed336 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java @@ -43,7 +43,7 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.is; -import static org.jenkinsci.plugins.credentialsbinding.test.ExecutableExists.executable; +import static org.jenkinsci.plugins.credentialsbinding.test.Executables.executable; import static org.junit.Assert.assertThat; import static org.junit.Assume.assumeThat; diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/ExecutableExists.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/Executables.java similarity index 57% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/test/ExecutableExists.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/test/Executables.java index 28ca2ea7..483b63e2 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/ExecutableExists.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/Executables.java @@ -25,35 +25,41 @@ package org.jenkinsci.plugins.credentialsbinding.test; import hudson.Functions; -import org.hamcrest.Description; +import org.apache.commons.io.IOUtils; +import org.hamcrest.CustomTypeSafeMatcher; import org.hamcrest.Matcher; -import org.hamcrest.TypeSafeMatcher; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; import java.io.IOException; +import java.util.List; -/** - * Hamcrest matcher for strings to check if the given command is executable. - * This uses {@code which} or {@code where.exe} to determine this depending on operating system. - */ -public class ExecutableExists extends TypeSafeMatcher { - - public static Matcher executable() { - return new ExecutableExists(); - } - +public class Executables { private static final String LOCATOR = Functions.isWindows() ? "where.exe" : "which"; - @Override - protected boolean matchesSafely(String item) { + public static @CheckForNull String getPathToExecutable(@Nonnull String executable) { try { - return new ProcessBuilder(LOCATOR, item).start().waitFor() == 0; - } catch (InterruptedException | IOException e) { - return false; + Process process = new ProcessBuilder(LOCATOR, executable).start(); + List output = IOUtils.readLines(process.getInputStream()); + if (process.waitFor() != 0) { + return null; + } + return output.isEmpty() ? null : output.get(0); + } catch (IOException | InterruptedException e) { + return null; } } - @Override - public void describeTo(Description description) { - description.appendText("executable"); + public static Matcher executable() { + return new CustomTypeSafeMatcher("executable") { + @Override + protected boolean matchesSafely(String item) { + try { + return new ProcessBuilder(LOCATOR, item).start().waitFor() == 0; + } catch (InterruptedException | IOException e) { + return false; + } + } + }; } } From ad1b6aa4164c0eb304c25f62e17f8099fc4bcb1f Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 3 May 2019 15:26:31 -0500 Subject: [PATCH 14/36] Move bash assumption to BeforeClass Seems as though the theories runner fails otherwise if the assumption is violated. Signed-off-by: Matt Sicker --- .../masking/BashPatternMaskerProviderTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 9d6a174c..3b1471f6 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -36,6 +36,7 @@ import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Rule; import org.junit.experimental.theories.DataPoint; import org.junit.experimental.theories.DataPoints; @@ -83,9 +84,13 @@ public static List generatePasswords() { private WorkflowJob project; private String credentialsId; + @BeforeClass + public static void assumeBash() { + assumeThat("bash", is(executable())); + } + @Before public void setUp() throws IOException { - assumeThat("bash", is(executable())); j.jenkins.getDescriptorByType(Shell.DescriptorImpl.class).setShell(Executables.getPathToExecutable("bash")); project = j.createProject(WorkflowJob.class); credentialsId = UUID.randomUUID().toString(); From 9463aaf173e0a1e1811627dafe3dbef41bc00091 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Tue, 7 May 2019 09:29:56 -0500 Subject: [PATCH 15/36] Fix java 11 build issue Signed-off-by: Matt Sicker --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index d79d5abe..b326298e 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.jenkins-ci.plugins plugin - 3.42 + 3.43 From c18e1ec065cd52c483cc80b79786632e2af6a93b Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Tue, 7 May 2019 10:24:08 -0500 Subject: [PATCH 16/36] Add tests for /bin/sh Signed-off-by: Matt Sicker --- .../BourneShellPatternMaskerProviderTest.java | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java new file mode 100644 index 00000000..977c00ce --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java @@ -0,0 +1,124 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.util.Secret; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; +import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.experimental.theories.DataPoint; +import org.junit.experimental.theories.DataPoints; +import org.junit.experimental.theories.Theories; +import org.junit.experimental.theories.Theory; +import org.junit.runner.RunWith; +import org.jvnet.hudson.test.JenkinsRule; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Random; +import java.util.UUID; + +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.startsWith; +import static org.jenkinsci.plugins.credentialsbinding.test.Executables.executable; +import static org.junit.Assume.assumeThat; + +@RunWith(Theories.class) +public class BourneShellPatternMaskerProviderTest { + + public static final @DataPoint String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; + public static final @DataPoint String ANOTHER_SAMPLE_PASSWORD = "a'b\"c\\d(e)#"; + + @DataPoints + public static List generatePasswords() { + Random random = new Random(100); + List passwords = new ArrayList<>(10); + for (int i = 0; i < 10; i++) { + int length = random.nextInt(24) + 8; + StringBuilder sb = new StringBuilder(length); + for (int j = 0; j < length; j++) { + char next = (char) (random.nextInt('~' - ' ' + 1) + ' '); // space = 0x20, tilde = 0x7E + sb.append(next); + } + passwords.add(sb.toString()); + } + return passwords; + } + + @Rule public JenkinsRule j = new JenkinsRule(); + + private WorkflowJob project; + private String credentialsId; + + @BeforeClass + public static void assumeBash() { + assumeThat("sh", is(executable())); + } + + @Before + public void setUp() throws IOException { + project = j.createProject(WorkflowJob.class); + credentialsId = UUID.randomUUID().toString(); + project.setDefinition(new CpsFlowDefinition( + "node {\n" + + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + + " sh ': \"$CREDENTIALS\"'\n" + // : will expand its parameters and do nothing with them + " sh ': \"< $CREDENTIALS >\"'\n" + + " }\n" + + "}", true)); + } + + @Theory + public void credentialsAreMaskedInLogs(String credentials) throws Exception { + assumeThat(credentials, not(startsWith("****"))); + + registerCredentials(credentials); + WorkflowRun run = runProject(); + + j.assertLogContains(": ****", run); + j.assertLogContains("< **** >", run); + j.assertLogNotContains(credentials, run); + } + + private void registerCredentials(String password) throws IOException { + StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, null, Secret.fromString(password)); + CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); + } + + private WorkflowRun runProject() throws Exception { + return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); + } + +} From 394a08e494cf8a30bc1b40eab078c511a18d5ec0 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 9 May 2019 14:07:00 -0500 Subject: [PATCH 17/36] Simplify batch pattern masker Based on an inability to make batch echo things differently when quoted properly, this simplifies the pattern masker implementations. Signed-off-by: Matt Sicker --- .../AbstractShellPatternMaskerProvider.java | 62 ------------------- .../masking/BashPatternMaskerProvider.java | 22 ++++--- .../masking/BatchPatternMaskerProvider.java | 51 ++------------- 3 files changed, 21 insertions(+), 114 deletions(-) delete mode 100644 src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java deleted file mode 100644 index 1e3a4ae0..00000000 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/AbstractShellPatternMaskerProvider.java +++ /dev/null @@ -1,62 +0,0 @@ -/* - * The MIT License - * - * Copyright (c) 2019 CloudBees, Inc. - * - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to deal - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in - * all copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN - * THE SOFTWARE. - */ - -package org.jenkinsci.plugins.credentialsbinding.masking; - -import javax.annotation.Nonnull; -import java.util.Collection; -import java.util.HashSet; -import java.util.regex.Pattern; - -/** - * Convenient base class for PatternMaskerProvider implementations for shells. - */ -abstract class AbstractShellPatternMaskerProvider implements PatternMaskerProvider { - - /** - * Performs any quoting/escaping necessary for the input string to be able to be surrounded in quotes and passed - * literally to a shell. This should not include the surrounding quotes. - */ - abstract @Nonnull String getQuotedForm(@Nonnull String input); - - /** - * Surrounds a string with the proper quote characters to make a quoted string for a shell. - */ - abstract @Nonnull String surroundWithQuotes(@Nonnull String input); - - /** - * Performs the inverse of {@link #getQuotedForm(String)}. - */ - abstract @Nonnull String getUnquotedForm(@Nonnull String input); - - @Override - public @Nonnull Collection getAlternativeForms(@Nonnull String input) { - Collection patterns = new HashSet<>(); - String quotedForm = getQuotedForm(input); - patterns.add(Pattern.quote(quotedForm)); - patterns.add(Pattern.quote(surroundWithQuotes(quotedForm))); - patterns.add(Pattern.quote(getUnquotedForm(input))); - return patterns; - } -} diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java index bf1d075b..00b68e75 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java @@ -29,16 +29,17 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; import javax.annotation.Nonnull; +import java.util.Collection; +import java.util.HashSet; import java.util.regex.Pattern; @Extension @Restricted(NoExternalUse.class) -public class BashPatternMaskerProvider extends AbstractShellPatternMaskerProvider { +public class BashPatternMaskerProvider implements PatternMaskerProvider { private static final Pattern QUOTED_CHARS = Pattern.compile("(\\\\)(\\\\?)"); - @Override - @Nonnull String getQuotedForm(@Nonnull String input) { + private @Nonnull String getQuotedForm(@Nonnull String input) { StringBuilder sb = new StringBuilder(input.length()); for (int i = 0; i < input.length(); i++) { char c = input.charAt(i); @@ -51,14 +52,21 @@ public class BashPatternMaskerProvider extends AbstractShellPatternMaskerProvide return sb.toString(); } - @Override - @Nonnull String surroundWithQuotes(@Nonnull String input) { + private @Nonnull String surroundWithQuotes(@Nonnull String input) { return "'" + input + "'"; } - @Override - @Nonnull String getUnquotedForm(@Nonnull String input) { + private @Nonnull String getUnquotedForm(@Nonnull String input) { return QUOTED_CHARS.matcher(input).replaceAll("$2"); } + @Override + public @Nonnull Collection getAlternativeForms(@Nonnull String input) { + Collection patterns = new HashSet<>(); + String quotedForm = getQuotedForm(input); + patterns.add(Pattern.quote(quotedForm)); + patterns.add(Pattern.quote(surroundWithQuotes(quotedForm))); + patterns.add(Pattern.quote(getUnquotedForm(input))); + return patterns; + } } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java index 8aef003b..1a41c1da 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java @@ -29,57 +29,18 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; import javax.annotation.Nonnull; +import java.util.Collection; +import java.util.Collections; import java.util.regex.Pattern; @Extension @Restricted(NoExternalUse.class) -public class BatchPatternMaskerProvider extends AbstractShellPatternMaskerProvider { - - // adapted from: - // https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ - // also from WindowsUtil in Jenkins - - private static final Pattern CMD_METACHARS = Pattern.compile("[()%!^\"<>&|]"); +public class BatchPatternMaskerProvider implements PatternMaskerProvider { private static final Pattern QUOTED_CHARS = Pattern.compile("(\\^)(\\^?)"); @Override - @Nonnull String getQuotedForm(@Nonnull String input) { - int end = input.length(); - StringBuilder sb = new StringBuilder(end); - for (int i = 0; i < end; i++) { - int nrBackslashes = 0; - while (i < end && input.charAt(i) == '\\') { - i++; - nrBackslashes++; - } - - if (i == end) { - // backslashes at the end of the argument must be escaped so the terminate quote isn't - nrBackslashes = nrBackslashes * 2; - } else if (input.charAt(i) == '"') { - // backslashes preceding a quote all need to be escaped along with the quote - nrBackslashes = nrBackslashes * 2 + 1; - } - // else backslashes have no special meaning and don't need to be escaped here - - for (int j = 0; j < nrBackslashes; j++) { - sb.append('\\'); - } - - if (i < end) { - sb.append(input.charAt(i)); - } - } - return CMD_METACHARS.matcher(sb).replaceAll("^$0"); - } - - @Override - @Nonnull String surroundWithQuotes(@Nonnull String input) { - return '"' + input + '"'; - } - - @Override - @Nonnull String getUnquotedForm(@Nonnull String input) { - return QUOTED_CHARS.matcher(input).replaceAll("$2"); + public @Nonnull Collection getAlternativeForms(@Nonnull String input) { + return input.contains("^") ? Collections.singleton(QUOTED_CHARS.matcher(input).replaceAll("$2")) + : Collections.emptySet(); } } From bd82279629d4019c02bad9aabca63f0f9f000f1c Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Fri, 10 May 2019 13:39:16 -0500 Subject: [PATCH 18/36] Fix bash assumptions in tests and other test fixes Signed-off-by: Matt Sicker --- .../BashPatternMaskerProvider2Test.java | 3 + .../BashPatternMaskerProviderTest.java | 3 + .../BatchPatternMaskerProviderTest.java | 105 +++++++++++++++--- .../BourneShellPatternMaskerProviderTest.java | 3 + .../masking/PowerShellMaskerProviderTest.java | 4 +- 5 files changed, 99 insertions(+), 19 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java index 70aa935b..2c26916f 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java @@ -54,6 +54,9 @@ public class BashPatternMaskerProvider2Test { @Before public void setUp() { assumeThat("bash", is(executable())); + // due to https://github.com/jenkinsci/durable-task-plugin/blob/e75123eda986f20a390d92cc892c3d206e60aefb/src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java#L149 + // on Windows + assumeThat("nohup", is(executable())); j.jenkins.getDescriptorByType(Shell.DescriptorImpl.class).setShell(Executables.getPathToExecutable("bash")); } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 3b1471f6..0134f600 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -87,6 +87,9 @@ public static List generatePasswords() { @BeforeClass public static void assumeBash() { assumeThat("bash", is(executable())); + // due to https://github.com/jenkinsci/durable-task-plugin/blob/e75123eda986f20a390d92cc892c3d206e60aefb/src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java#L149 + // on Windows + assumeThat("nohup", is(executable())); } @Before diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java index ca557fef..a0983fcf 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java @@ -28,6 +28,7 @@ import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.domains.Domain; import hudson.Functions; +import hudson.model.Result; import hudson.util.Secret; import org.jenkinsci.plugins.plaincredentials.StringCredentials; import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; @@ -35,7 +36,6 @@ import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.junit.Before; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; @@ -43,6 +43,8 @@ import java.io.IOException; import java.util.UUID; +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertThat; import static org.junit.Assume.assumeTrue; public class BatchPatternMaskerProviderTest { @@ -134,13 +136,38 @@ public void nonDangerous_delayed() throws Exception { assertDelayedNoPlainTextButStars(runDelayedAllQuotes()); } + // we do NOT support dangerous characters in direct expansion mode @Test - @Ignore("Cannot support the dangerous characters in direct expressions") - public void allAscii_direct() throws Exception { + public void allAscii_direct_noQuote() throws Exception { registerCredentials(ALL_ASCII); - assertDirectNoPlainTextButStars(runDirectNoQuote()); - assertDirectNoPlainTextButStars(runDirectSingleQuote()); - assertDirectNoPlainTextButStars(runDirectDoubleQuote()); + + WorkflowRun run = runDirectNoQuote(); + j.assertLogNotContains(credentialPlainText, run); + + // EFGHIJK is a part of the credentials that should be masked + assertStringPresentInOrder(run, "before1", "EFGHIJK", "after1"); + j.assertLogNotContains("before1 **** after1", run); + } + + // we do NOT support dangerous characters in direct expansion mode + @Test + public void allAscii_direct_singleQuote() throws Exception { + registerCredentials(ALL_ASCII); + + WorkflowRun run = runDirectSingleQuote(); + j.assertLogNotContains(credentialPlainText, run); + + // EFGHIJK is a part of the credentials that should be masked + assertStringPresentInOrder(run, "before1", "EFGHIJK", "after1"); + j.assertLogNotContains("before1 **** after1", run); + } + + // we do NOT support dangerous characters in direct expansion mode + @Test + public void allAscii_direct_doubleQuote() throws Exception { + registerCredentials(ALL_ASCII); + + runDirectDoubleQuote_andFail(); } @Test @@ -148,27 +175,29 @@ public void allAscii_delayed() throws Exception { registerCredentials(ALL_ASCII); assertDelayedNoPlainTextButStars(runDelayedAllQuotes()); } - + + // we do NOT support dangerous characters in direct expansion mode @Test - @Ignore("Cannot support dangerous character & in non-delayed expansion without double quotes") public void samplePassword_noQuote() throws Exception { registerCredentials(SAMPLE_PASSWORD); - assertDirectNoPlainTextButStars(runDirectNoQuote()); + + runDirectNoQuote_andFail(); } - + + // we do NOT support dangerous characters in direct expansion mode @Test - @Ignore("Cannot support dangerous character & in non-delayed expansion without double quotes") public void samplePassword_singleQuote() throws Exception { registerCredentials(SAMPLE_PASSWORD); - assertDirectNoPlainTextButStars(runDirectSingleQuote()); + + runDirectSingleQuote_andFail(); } - + @Test public void samplePassword_doubleQuote() throws Exception { registerCredentials(SAMPLE_PASSWORD); assertDirectNoPlainTextButStars(runDirectDoubleQuote()); } - + @Test public void samplePassword_delayed() throws Exception { registerCredentials(SAMPLE_PASSWORD); @@ -234,6 +263,16 @@ private void assertDelayedNoPlainTextButStars(WorkflowRun run) throws Exception } private WorkflowRun runDirectNoQuote() throws Exception { + setupNoQuoteProject(); + return runProject(); + } + + private void runDirectNoQuote_andFail() throws Exception { + setupNoQuoteProject(); + j.assertBuildStatus(Result.FAILURE, project.scheduleBuild2(0)); + } + + private void setupNoQuoteProject() throws Exception { // DO NOT DO THIS IN PRODUCTION // these commands would be completely broken if echo didn't work the way it does when CREDENTIALS contains special characters setupProject("node {\n" + @@ -247,10 +286,19 @@ private WorkflowRun runDirectNoQuote() throws Exception { " }\n" + "}" ); - return runProject(); } private WorkflowRun runDirectSingleQuote() throws Exception { + setupSingleQuoteProject(); + return runProject(); + } + + private void runDirectSingleQuote_andFail() throws Exception { + setupSingleQuoteProject(); + j.assertBuildStatus(Result.FAILURE, project.scheduleBuild2(0)); + } + + private void setupSingleQuoteProject() throws Exception { // DO NOT DO THIS IN PRODUCTION // single quotes do not mean anything in batch scripts; use double quotes for escaping/quoting strings setupProject("node {\n" + @@ -264,10 +312,19 @@ private WorkflowRun runDirectSingleQuote() throws Exception { " }\n" + "}" ); - return runProject(); } private WorkflowRun runDirectDoubleQuote() throws Exception { + setupDoubleQuoteProject(); + return runProject(); + } + + private void runDirectDoubleQuote_andFail() throws Exception { + setupDoubleQuoteProject(); + j.assertBuildStatus(Result.FAILURE, project.scheduleBuild2(0)); + } + + private void setupDoubleQuoteProject() throws Exception { setupProject("node {\n" + " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + " bat \"\"\"\n" + @@ -279,7 +336,6 @@ private WorkflowRun runDirectDoubleQuote() throws Exception { " }\n" + "}" ); - return runProject(); } private WorkflowRun runDelayedAllQuotes() throws Exception { @@ -297,6 +353,21 @@ private WorkflowRun runDelayedAllQuotes() throws Exception { return runProject(); } + private void assertStringPresentInOrder(WorkflowRun run, String... values) throws Exception { + String fullLog = run.getLog(); + int currentIndex = 0; + for (int i = 0; i < values.length; i++) { + String currentValue = values[i]; + int nextIndex = fullLog.indexOf(currentValue, currentIndex); + if(nextIndex == -1){ + // use assertThat to have better output + assertThat(fullLog.substring(currentIndex), containsString(currentValue)); + }else{ + currentIndex = nextIndex + currentValue.length(); + } + } + } + private void setupProject(String pipeline) throws Exception { String projectName = UUID.randomUUID().toString(); project = j.jenkins.createProject(WorkflowJob.class, projectName); diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java index 977c00ce..af68af7b 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java @@ -85,6 +85,9 @@ public static List generatePasswords() { @BeforeClass public static void assumeBash() { assumeThat("sh", is(executable())); + // due to https://github.com/jenkinsci/durable-task-plugin/blob/e75123eda986f20a390d92cc892c3d206e60aefb/src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java#L149 + // on Windows + assumeThat("nohup", is(executable())); } @Before diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java index e32ed336..5bbadd52 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java @@ -121,9 +121,9 @@ private WorkflowRun runPowerShellInterpretation() throws Exception { // interpreted by PowerShell " powershell '''\n" + // echo is an alias for Write-Output - " echo before1 $env:CREDENTIALS after1\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG + " echo before1 $env:CREDENTIALS after1\n" + // echo '...' does not let PowerShell to interpret the information - " echo \"before2 $env:CREDENTIALS after2\"\n" + // THIS IS OK THOUGH + " echo \"before2 $env:CREDENTIALS after2\"\n" + " '''\n" + " }\n" + "}" From ee00db0010d9fc8d9a0d39990292c4d96c677cbb Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Mon, 20 May 2019 15:35:56 -0500 Subject: [PATCH 19/36] Clarify character range Co-Authored-By: Daniel Beck <1831569+daniel-beck@users.noreply.github.com> --- .../masking/BashPatternMaskerProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 0134f600..dd13d2b1 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -71,7 +71,7 @@ public static List generatePasswords() { int length = random.nextInt(24) + 8; StringBuilder sb = new StringBuilder(length); for (int j = 0; j < length; j++) { - char next = (char) (random.nextInt('~' - ' ' + 1) + ' '); // space = 0x20, tilde = 0x7E + char next = (char) (random.nextInt('~' - ' ' + 1) + ' '); // space/0x20 is the first printable ASCII char, tilde/0x7E the last sb.append(next); } passwords.add(sb.toString()); From f07204bf74093057cc56413617e9cd8c339a0895 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Tue, 21 May 2019 13:57:30 -0500 Subject: [PATCH 20/36] Refactor credentials registration in tests Signed-off-by: Matt Sicker --- .../BashPatternMaskerProvider2Test.java | 19 +----- .../BashPatternMaskerProviderTest.java | 14 +---- .../BatchPatternMaskerProviderTest.java | 16 +---- .../BourneShellPatternMaskerProviderTest.java | 14 +---- .../masking/PowerShellMaskerProviderTest.java | 18 ++---- .../test/CredentialsTestUtil.java | 58 +++++++++++++++++++ 6 files changed, 71 insertions(+), 68 deletions(-) create mode 100644 src/test/java/org/jenkinsci/plugins/credentialsbinding/test/CredentialsTestUtil.java diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java index 2c26916f..947c28bd 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java @@ -24,14 +24,9 @@ package org.jenkinsci.plugins.credentialsbinding.masking; -import com.cloudbees.plugins.credentials.CredentialsProvider; -import com.cloudbees.plugins.credentials.CredentialsScope; -import com.cloudbees.plugins.credentials.domains.Domain; import hudson.tasks.Shell; -import hudson.util.Secret; +import org.jenkinsci.plugins.credentialsbinding.test.CredentialsTestUtil; import org.jenkinsci.plugins.credentialsbinding.test.Executables; -import org.jenkinsci.plugins.plaincredentials.StringCredentials; -import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; @@ -40,9 +35,6 @@ import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; -import java.io.IOException; -import java.util.UUID; - import static org.hamcrest.Matchers.is; import static org.jenkinsci.plugins.credentialsbinding.test.Executables.executable; import static org.junit.Assume.assumeThat; @@ -65,7 +57,7 @@ public void setUp() { public void testSecretsWithBackslashesStillMaskedWhenUsedWithoutProperQuoting() throws Exception { WorkflowJob project = j.createProject(WorkflowJob.class); String password = "foo\\bar\\"; - String credentialsId = registerStringCredentials(password); + String credentialsId = CredentialsTestUtil.registerStringCredentials(j.jenkins, password); project.setDefinition(new CpsFlowDefinition( "node {\n" + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + @@ -81,11 +73,4 @@ public void testSecretsWithBackslashesStillMaskedWhenUsedWithoutProperQuoting() j.assertLogNotContains("foo", run); j.assertLogNotContains("bar", run); } - - private String registerStringCredentials(String password) throws IOException { - String credentialId = UUID.randomUUID().toString(); - StringCredentials creds = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); - CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), creds); - return credentialId; - } } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index dd13d2b1..427a043a 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -24,14 +24,9 @@ package org.jenkinsci.plugins.credentialsbinding.masking; -import com.cloudbees.plugins.credentials.CredentialsProvider; -import com.cloudbees.plugins.credentials.CredentialsScope; -import com.cloudbees.plugins.credentials.domains.Domain; import hudson.tasks.Shell; -import hudson.util.Secret; +import org.jenkinsci.plugins.credentialsbinding.test.CredentialsTestUtil; import org.jenkinsci.plugins.credentialsbinding.test.Executables; -import org.jenkinsci.plugins.plaincredentials.StringCredentials; -import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; @@ -110,7 +105,7 @@ public void setUp() throws IOException { public void credentialsAreMaskedInLogs(String credentials) throws Exception { assumeThat(credentials, not(startsWith("****"))); - registerCredentials(credentials); + CredentialsTestUtil.setStringCredentials(j.jenkins, credentialsId, credentials); WorkflowRun run = runProject(); j.assertLogContains(": ****", run); @@ -118,11 +113,6 @@ public void credentialsAreMaskedInLogs(String credentials) throws Exception { j.assertLogNotContains(credentials, run); } - private void registerCredentials(String password) throws IOException { - StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, null, Secret.fromString(password)); - CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); - } - private WorkflowRun runProject() throws Exception { return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java index a0983fcf..8da3a33e 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java @@ -24,14 +24,9 @@ package org.jenkinsci.plugins.credentialsbinding.masking; -import com.cloudbees.plugins.credentials.CredentialsProvider; -import com.cloudbees.plugins.credentials.CredentialsScope; -import com.cloudbees.plugins.credentials.domains.Domain; import hudson.Functions; import hudson.model.Result; -import hudson.util.Secret; -import org.jenkinsci.plugins.plaincredentials.StringCredentials; -import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.credentialsbinding.test.CredentialsTestUtil; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; @@ -41,7 +36,6 @@ import org.jvnet.hudson.test.JenkinsRule; import java.io.IOException; -import java.util.UUID; import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertThat; @@ -79,9 +73,7 @@ public void assumeWindowsForBatch() throws Exception { private void registerCredentials(String password) throws IOException { this.credentialPlainText = password; - this.credentialId = UUID.randomUUID().toString(); - StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); - CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); + this.credentialId = CredentialsTestUtil.registerStringCredentials(j.jenkins, password); } private WorkflowRun runProject() throws Exception { @@ -369,9 +361,7 @@ private void assertStringPresentInOrder(WorkflowRun run, String... values) throw } private void setupProject(String pipeline) throws Exception { - String projectName = UUID.randomUUID().toString(); - project = j.jenkins.createProject(WorkflowJob.class, projectName); - credentialId = UUID.randomUUID().toString(); + project = j.createProject(WorkflowJob.class); project.setDefinition(new CpsFlowDefinition(pipeline, true)); } } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java index af68af7b..11df0f6e 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java @@ -24,12 +24,7 @@ package org.jenkinsci.plugins.credentialsbinding.masking; -import com.cloudbees.plugins.credentials.CredentialsProvider; -import com.cloudbees.plugins.credentials.CredentialsScope; -import com.cloudbees.plugins.credentials.domains.Domain; -import hudson.util.Secret; -import org.jenkinsci.plugins.plaincredentials.StringCredentials; -import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.credentialsbinding.test.CredentialsTestUtil; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; @@ -107,7 +102,7 @@ public void setUp() throws IOException { public void credentialsAreMaskedInLogs(String credentials) throws Exception { assumeThat(credentials, not(startsWith("****"))); - registerCredentials(credentials); + CredentialsTestUtil.setStringCredentials(j.jenkins, credentialsId, credentials); WorkflowRun run = runProject(); j.assertLogContains(": ****", run); @@ -115,11 +110,6 @@ public void credentialsAreMaskedInLogs(String credentials) throws Exception { j.assertLogNotContains(credentials, run); } - private void registerCredentials(String password) throws IOException { - StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, null, Secret.fromString(password)); - CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); - } - private WorkflowRun runProject() throws Exception { return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java index 5bbadd52..9fedb87f 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/PowerShellMaskerProviderTest.java @@ -24,12 +24,7 @@ package org.jenkinsci.plugins.credentialsbinding.masking; -import com.cloudbees.plugins.credentials.CredentialsProvider; -import com.cloudbees.plugins.credentials.CredentialsScope; -import com.cloudbees.plugins.credentials.domains.Domain; -import hudson.util.Secret; -import org.jenkinsci.plugins.plaincredentials.StringCredentials; -import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; +import org.jenkinsci.plugins.credentialsbinding.test.CredentialsTestUtil; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; @@ -39,7 +34,6 @@ import org.jvnet.hudson.test.JenkinsRule; import java.io.IOException; -import java.util.UUID; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.is; @@ -70,9 +64,7 @@ public void assumeWindowsForBatch() throws Exception { private void registerCredentials(String password) throws IOException { this.credentialPlainText = password; - this.credentialId = UUID.randomUUID().toString(); - StringCredentials credentials = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialId, null, Secret.fromString(password)); - CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials); + this.credentialId = CredentialsTestUtil.registerStringCredentials(j.jenkins, password); } @Test @@ -128,13 +120,11 @@ private WorkflowRun runPowerShellInterpretation() throws Exception { " }\n" + "}" ); - return project.scheduleBuild2(0).get(); + return j.assertBuildStatusSuccess(project.scheduleBuild2(0)); } private void setupProject(String pipeline) throws Exception { - String projectName = UUID.randomUUID().toString(); - project = j.jenkins.createProject(WorkflowJob.class, projectName); - credentialId = UUID.randomUUID().toString(); + project = j.createProject(WorkflowJob.class); project.setDefinition(new CpsFlowDefinition(pipeline, true)); } } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/CredentialsTestUtil.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/CredentialsTestUtil.java new file mode 100644 index 00000000..b5b55481 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/test/CredentialsTestUtil.java @@ -0,0 +1,58 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.test; + +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.model.ModelObject; +import hudson.util.Secret; +import org.jenkinsci.plugins.plaincredentials.StringCredentials; +import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl; + +import java.io.IOException; +import java.util.UUID; + +public class CredentialsTestUtil { + + /** + * Registers the given value as a {@link StringCredentials} into the default {@link CredentialsProvider}. + * Returns the generated credential id for the registered credentials. + */ + public static String registerStringCredentials(ModelObject context, String value) throws IOException { + String credentialsId = UUID.randomUUID().toString(); + setStringCredentials(context, credentialsId, value); + return credentialsId; + } + + /** + * Registers the given value as a {@link StringCredentials} into the default {@link CredentialsProvider} using the + * specified credentials id. + */ + public static void setStringCredentials(ModelObject context, String credentialsId, String value) throws IOException { + StringCredentials creds = new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, null, Secret.fromString(value)); + CredentialsProvider.lookupStores(context).iterator().next().addCredentials(Domain.global(), creds); + } +} From 5425b67f0ad448a39623428effb4b65194740cc4 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Tue, 21 May 2019 13:58:40 -0500 Subject: [PATCH 21/36] Add one more data point to bash test Signed-off-by: Matt Sicker --- .../masking/BashPatternMaskerProviderTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 427a043a..889278e7 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -57,6 +57,7 @@ public class BashPatternMaskerProviderTest { public static final @DataPoint String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; public static final @DataPoint String ANOTHER_SAMPLE_PASSWORD = "a'b\"c\\d(e)#"; + public static final @DataPoint String ONE_MORE = "'\"'(foo)'\"'"; @DataPoints public static List generatePasswords() { From ebb4a537093f696642c71e1ec0a9ec732a24374a Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Tue, 21 May 2019 14:08:47 -0500 Subject: [PATCH 22/36] Improve password generation readability Signed-off-by: Matt Sicker --- .../masking/BashPatternMaskerProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 889278e7..586bbb42 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -67,7 +67,7 @@ public static List generatePasswords() { int length = random.nextInt(24) + 8; StringBuilder sb = new StringBuilder(length); for (int j = 0; j < length; j++) { - char next = (char) (random.nextInt('~' - ' ' + 1) + ' '); // space/0x20 is the first printable ASCII char, tilde/0x7E the last + char next = (char) (' ' + random.nextInt('\u007f' - ' ')); // 0x7f is DEL, 0x7e is ~, and space is the first printable ASCII character sb.append(next); } passwords.add(sb.toString()); From 15128307e2243ac48fa1071dff8cfac68fa55995 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Wed, 22 May 2019 11:50:16 -0500 Subject: [PATCH 23/36] Fix test regressions Signed-off-by: Matt Sicker --- .../credentialsbinding/masking/BatchPatternMaskerProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java index 1a41c1da..adb4ccd4 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java @@ -40,7 +40,7 @@ public class BatchPatternMaskerProvider implements PatternMaskerProvider { @Override public @Nonnull Collection getAlternativeForms(@Nonnull String input) { - return input.contains("^") ? Collections.singleton(QUOTED_CHARS.matcher(input).replaceAll("$2")) + return input.contains("^") ? Collections.singleton(Pattern.quote(QUOTED_CHARS.matcher(input).replaceAll("$2"))) : Collections.emptySet(); } } From c707d9226ad14bb8aedd1a35810af5b59c79ecae Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 10:43:36 -0500 Subject: [PATCH 24/36] Remove unneeded TODO Co-Authored-By: Jesse Glick --- .../credentialsbinding/masking/PatternMaskerProvider.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java index 18fd8ae3..4b539b29 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java @@ -40,7 +40,6 @@ * Quotes strings according to quotation rules of various programs. Sometimes confused with escaping, quoting is done * to pass literal strings in a shell or other interpreter. * - * @since TODO */ public interface PatternMaskerProvider extends ExtensionPoint { From 7b05cd28485ffd7c4a4ddc54fb3855e33a69101f Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 10:43:58 -0500 Subject: [PATCH 25/36] Clean up javadoc Co-Authored-By: Jesse Glick --- .../credentialsbinding/masking/PatternMaskerProvider.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java index 4b539b29..1270ed36 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java @@ -37,7 +37,8 @@ import java.util.regex.Pattern; /** - * Quotes strings according to quotation rules of various programs. Sometimes confused with escaping, quoting is done + * Quotes strings according to quotation rules of various programs. + * Sometimes confused with escaping, quoting is done * to pass literal strings in a shell or other interpreter. * */ From f7690e07ade17bc4b59d96e7a47a892e7edcd24c Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 10:52:49 -0500 Subject: [PATCH 26/36] Apply suggestions from code review Co-Authored-By: Jesse Glick --- .../plugins/credentialsbinding/impl/BindingStep/help.html | 2 +- .../masking/BashPatternMaskerProvider2Test.java | 5 +++-- .../masking/BatchPatternMaskerProviderTest.java | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html index c405a910..7783af41 100644 --- a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html +++ b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html @@ -73,7 +73,7 @@ foo'bar

- Mangled secrets can only be detected on a best effort basis. By default, Jenkins will attempt to mask mangled + Mangled secrets can only be detected on a best-effort basis. By default, Jenkins will attempt to mask mangled secrets as they would appear in output of Bash and Windows batch. Without these strategies in place, mangled secrets would appear in plain text in log files. In the example above, this would result in:

diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java index 947c28bd..fec97938 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java @@ -39,6 +39,7 @@ import static org.jenkinsci.plugins.credentialsbinding.test.Executables.executable; import static org.junit.Assume.assumeThat; +@For(BashPatternMaskerProvider.class) public class BashPatternMaskerProvider2Test { public @Rule JenkinsRule j = new JenkinsRule(); @@ -60,9 +61,9 @@ public void testSecretsWithBackslashesStillMaskedWhenUsedWithoutProperQuoting() String credentialsId = CredentialsTestUtil.registerStringCredentials(j.jenkins, password); project.setDefinition(new CpsFlowDefinition( "node {\n" + - " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + " sh ': $CREDENTIALS'\n" + // forgot quotes - " sh \": $CREDENTIALS\"\n" + // using groovy variable and forgot quotes + " sh(/: $CREDENTIALS/)\n" + // using Groovy variable and forgot quotes " }\n" + "}", true)); diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java index 8da3a33e..b14c7ea8 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java @@ -270,7 +270,7 @@ private void setupNoQuoteProject() throws Exception { setupProject("node {\n" + " withCredentials([string(credentialsId: '" + credentialId + "', variable: 'CREDENTIALS')]) {\n" + " bat \"\"\"\n" + - " echo before1 $CREDENTIALS after1\n" + // DO NOT DO THIS IN PRODUCTION; IT IS USING THE WRONG VARIABLE SYNTAX + " echo before1 $CREDENTIALS after1\n" + // DO NOT DO THIS IN PRODUCTION; IT IS USING GROOVY INTERPOLATION " \"\"\"\n" + " bat '''\n" + " echo before2 %CREDENTIALS% after2\n" + // DO NOT DO THIS IN PRODUCTION; IT IS QUOTED WRONG From c9938de7894c69a1de3f015468ceeae21801313d Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 10:59:44 -0500 Subject: [PATCH 27/36] Re-add docs on set +x Signed-off-by: Matt Sicker --- .../plugins/credentialsbinding/impl/BindingStep/help.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html index 7783af41..06d2b7f6 100644 --- a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html +++ b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html @@ -79,6 +79,10 @@ 

+ echo 'foo'\''bar'
 ****
+

+ This particular issue can be more safely prevented by turning off echo with set +x or avoiding the use + of shell metacharacters in secrets. +

For bindings which store a secret file, beware that

From 14f8c624795ef02affd2b0c544a00aef193eaea0 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 11:00:42 -0500 Subject: [PATCH 28/36] Fix imports Signed-off-by: Matt Sicker --- .../masking/BashPatternMaskerProvider2Test.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java index fec97938..64a57ab6 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java @@ -33,6 +33,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.For; import org.jvnet.hudson.test.JenkinsRule; import static org.hamcrest.Matchers.is; From 4ebbd28a252395eb914ffa284895d1a50efff1ea Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 11:03:59 -0500 Subject: [PATCH 29/36] Use @ClassRule JenkinsRule and improve comments Signed-off-by: Matt Sicker --- .../masking/BashPatternMaskerProviderTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java index 586bbb42..785be09d 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java @@ -32,7 +32,7 @@ import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.junit.Before; import org.junit.BeforeClass; -import org.junit.Rule; +import org.junit.ClassRule; import org.junit.experimental.theories.DataPoint; import org.junit.experimental.theories.DataPoints; import org.junit.experimental.theories.Theories; @@ -67,7 +67,9 @@ public static List generatePasswords() { int length = random.nextInt(24) + 8; StringBuilder sb = new StringBuilder(length); for (int j = 0; j < length; j++) { - char next = (char) (' ' + random.nextInt('\u007f' - ' ')); // 0x7f is DEL, 0x7e is ~, and space is the first printable ASCII character + // choose a (printable) character in the closed range [' ', '~'] + // 0x7f is DEL, 0x7e is ~, and space is the first printable ASCII character + char next = (char) (' ' + random.nextInt('\u007f' - ' ')); sb.append(next); } passwords.add(sb.toString()); @@ -75,7 +77,7 @@ public static List generatePasswords() { return passwords; } - @Rule public JenkinsRule j = new JenkinsRule(); + @ClassRule public static JenkinsRule j = new JenkinsRule(); private WorkflowJob project; private String credentialsId; @@ -97,7 +99,7 @@ public void setUp() throws IOException { "node {\n" + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'CREDENTIALS')]) {\n" + " sh ': \"$CREDENTIALS\"'\n" + // : will expand its parameters and do nothing with them - " sh ': \"< $CREDENTIALS >\"'\n" + + " sh ': \"< $CREDENTIALS >\"'\n" + // surround credentials with identifiable text for partial quoting " }\n" + "}", true)); } From 9bd38cae252b28d96fc2fc4ad83e20eba0ec6ed1 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 11:45:37 -0500 Subject: [PATCH 30/36] Refactor masking API to use Pattern This removes the exposure of Pattern.quote() in the consumer calls along with other cleanups around this idea. Signed-off-by: Matt Sicker --- .../credentialsbinding/impl/BindingStep.java | 4 +- .../impl/SecretBuildWrapper.java | 4 +- ...der.java => BashSecretPatternFactory.java} | 13 ++-- ...er.java => BatchSecretPatternFactory.java} | 7 ++- .../masking/LiteralSecretPatternFactory.java | 47 ++++++++++++++ .../masking/SecretPatternFactory.java | 61 +++++++++++++++++++ ...askerProvider.java => SecretPatterns.java} | 59 ++++++------------ ...ava => BashSecretPatternFactory2Test.java} | 4 +- ...java => BashSecretPatternFactoryTest.java} | 2 +- ...ava => BatchSecretPatternFactoryTest.java} | 2 +- ... BourneShellSecretPatternFactoryTest.java} | 2 +- 11 files changed, 145 insertions(+), 60 deletions(-) rename src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/{BashPatternMaskerProvider.java => BashSecretPatternFactory.java} (81%) rename src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/{BatchPatternMaskerProvider.java => BatchSecretPatternFactory.java} (83%) create mode 100644 src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java create mode 100644 src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java rename src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/{PatternMaskerProvider.java => SecretPatterns.java} (55%) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/{BashPatternMaskerProvider2Test.java => BashSecretPatternFactory2Test.java} (97%) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/{BashPatternMaskerProviderTest.java => BashSecretPatternFactoryTest.java} (99%) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/{BatchPatternMaskerProviderTest.java => BatchSecretPatternFactoryTest.java} (99%) rename src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/{BourneShellPatternMaskerProviderTest.java => BourneShellSecretPatternFactoryTest.java} (98%) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java index d13533e9..bbbceece 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java @@ -54,7 +54,7 @@ import org.apache.commons.codec.Charsets; import org.jenkinsci.plugins.credentialsbinding.MultiBinding; -import org.jenkinsci.plugins.credentialsbinding.masking.PatternMaskerProvider; +import org.jenkinsci.plugins.credentialsbinding.masking.SecretPatterns; import org.jenkinsci.plugins.workflow.steps.AbstractStepExecutionImpl; import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback; import org.jenkinsci.plugins.workflow.steps.BodyInvoker; @@ -196,7 +196,7 @@ private static final class Filter extends ConsoleLogFilter implements Serializab private String charsetName; Filter(Collection secrets, String charsetName) { - pattern = Secret.fromString(PatternMaskerProvider.getMaskingPattern(secrets).pattern()); + pattern = Secret.fromString(SecretPatterns.getAggregateSecretPattern(secrets).pattern()); this.charsetName = charsetName; } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java index 6aefeae4..57c8b1ab 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/SecretBuildWrapper.java @@ -35,7 +35,7 @@ import hudson.tasks.BuildWrapper; import hudson.tasks.BuildWrapperDescriptor; import org.jenkinsci.plugins.credentialsbinding.MultiBinding; -import org.jenkinsci.plugins.credentialsbinding.masking.PatternMaskerProvider; +import org.jenkinsci.plugins.credentialsbinding.masking.SecretPatterns; import org.kohsuke.stapler.DataBoundConstructor; import javax.annotation.CheckForNull; @@ -61,7 +61,7 @@ public class SecretBuildWrapper extends BuildWrapper { */ public static @CheckForNull Pattern getPatternForBuild(@Nonnull AbstractBuild build) { if (secretsForBuild.containsKey(build)) { - return PatternMaskerProvider.getMaskingPattern(secretsForBuild.get(build)); + return SecretPatterns.getAggregateSecretPattern(secretsForBuild.get(build)); } else { return null; } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java similarity index 81% rename from src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java rename to src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java index 00b68e75..03dd2eac 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java @@ -35,7 +35,7 @@ @Extension @Restricted(NoExternalUse.class) -public class BashPatternMaskerProvider implements PatternMaskerProvider { +public class BashSecretPatternFactory implements SecretPatternFactory { private static final Pattern QUOTED_CHARS = Pattern.compile("(\\\\)(\\\\?)"); @@ -61,12 +61,13 @@ public class BashPatternMaskerProvider implements PatternMaskerProvider { } @Override - public @Nonnull Collection getAlternativeForms(@Nonnull String input) { - Collection patterns = new HashSet<>(); + public @Nonnull Collection getSecretPatterns(@Nonnull String input) { + Collection patterns = new HashSet<>(); String quotedForm = getQuotedForm(input); - patterns.add(Pattern.quote(quotedForm)); - patterns.add(Pattern.quote(surroundWithQuotes(quotedForm))); - patterns.add(Pattern.quote(getUnquotedForm(input))); + patterns.add(SecretPatternFactory.quotedCompile(quotedForm)); + patterns.add(SecretPatternFactory.quotedCompile(quotedForm)); + patterns.add(SecretPatternFactory.quotedCompile(surroundWithQuotes(quotedForm))); + patterns.add(SecretPatternFactory.quotedCompile(getUnquotedForm(input))); return patterns; } } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactory.java similarity index 83% rename from src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java rename to src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactory.java index adb4ccd4..479700ac 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactory.java @@ -35,12 +35,13 @@ @Extension @Restricted(NoExternalUse.class) -public class BatchPatternMaskerProvider implements PatternMaskerProvider { +public class BatchSecretPatternFactory implements SecretPatternFactory { private static final Pattern QUOTED_CHARS = Pattern.compile("(\\^)(\\^?)"); @Override - public @Nonnull Collection getAlternativeForms(@Nonnull String input) { - return input.contains("^") ? Collections.singleton(Pattern.quote(QUOTED_CHARS.matcher(input).replaceAll("$2"))) + public @Nonnull Collection getSecretPatterns(@Nonnull String input) { + return input.contains("^") + ? Collections.singleton(SecretPatternFactory.quotedCompile(QUOTED_CHARS.matcher(input).replaceAll("$2"))) : Collections.emptySet(); } } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java new file mode 100644 index 00000000..404b67c2 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java @@ -0,0 +1,47 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import hudson.Extension; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.annotation.Nonnull; +import java.util.Collection; +import java.util.Collections; +import java.util.regex.Pattern; + +/** + * Trivial secret pattern factory that matches the literal value of the secret. + */ +@Extension +@Restricted(NoExternalUse.class) +public class LiteralSecretPatternFactory implements SecretPatternFactory { + @Nonnull + @Override + public Collection getSecretPatterns(@Nonnull String input) { + return Collections.singleton(SecretPatternFactory.quotedCompile(input)); + } +} diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java new file mode 100644 index 00000000..dd713676 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java @@ -0,0 +1,61 @@ +/* + * The MIT License + * + * Copyright (c) 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.credentialsbinding.masking; + +import hudson.ExtensionList; +import hudson.ExtensionPoint; + +import javax.annotation.Nonnull; +import java.util.Collection; +import java.util.regex.Pattern; + +/** + * Creates regular expressions to match encoded forms of secrets in logs. + * These are typically implemented to handle various shell quoting algorithms (sometimes confused with escaping) to + * pass literal string values to an interpreter. + */ +public interface SecretPatternFactory extends ExtensionPoint { + + /** + * Returns a collection of alternative forms the given input may show up as in logs. + * Note that these patterns must embed their flags in the pattern rather than as parameters to Pattern. + */ + @Nonnull Collection getSecretPatterns(@Nonnull String input); + + /** + * Returns all SecretPatternFactory extensions known at runtime. + */ + static @Nonnull ExtensionList all() { + return ExtensionList.lookup(SecretPatternFactory.class); + } + + /** + * Composes {@link Pattern#compile(String)} and {@link Pattern#quote(String)} for convenience. + */ + static @Nonnull Pattern quotedCompile(@Nonnull String input) { + return Pattern.compile(Pattern.quote(input)); + } + +} diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java similarity index 55% rename from src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java rename to src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java index 1270ed36..4935c5fc 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java @@ -24,52 +24,22 @@ package org.jenkinsci.plugins.credentialsbinding.masking; -import hudson.ExtensionList; -import hudson.ExtensionPoint; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import javax.annotation.Nonnull; import java.util.Collection; import java.util.Comparator; -import java.util.HashSet; import java.util.TreeSet; import java.util.regex.Pattern; +import java.util.stream.Collectors; -/** - * Quotes strings according to quotation rules of various programs. - * Sometimes confused with escaping, quoting is done - * to pass literal strings in a shell or other interpreter. - * - */ -public interface PatternMaskerProvider extends ExtensionPoint { - - /** - * Returns a collection of alternative forms as regular expressions the given input may show up as in logs. These - * patterns should be appropriately {@linkplain Pattern#quote(String) quoted} for matching literal patterns as - * the resulting strings are formed into a regular expression. - */ - @Nonnull Collection getAlternativeForms(@Nonnull String input); - - /** - * Returns all PatternMaskerProvider extensions known at runtime. - */ - static @Nonnull ExtensionList all() { - return ExtensionList.lookup(PatternMaskerProvider.class); - } +@Restricted(NoExternalUse.class) +public class SecretPatterns { - /** - * Returns a collection of all alternative form regular expressions for the given input using all PatternMaskerProvider - * extensions. - */ - @Restricted(NoExternalUse.class) - static @Nonnull Collection getAllAlternateForms(@Nonnull String input) { - Collection alternateForms = new HashSet<>(); - for (PatternMaskerProvider provider : all()) { - alternateForms.addAll(provider.getAlternativeForms(input)); - } - return alternateForms; - } + private static final Comparator EXTRACT_PATTERN = Comparator.comparing(Pattern::pattern); + private static final Comparator BY_LENGTH_DESCENDING = Comparator.comparing(Pattern::pattern, + Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo)); /** * Constructs a regular expression to match against all known forms that the given collection of input strings may @@ -80,16 +50,21 @@ public interface PatternMaskerProvider extends ExtensionPoint { * For example, {@code bash -x} will only quote arguments echoed when necessary. To avoid leaking the presence or * absence of quoting, the longer form is masked. */ - @Restricted(NoExternalUse.class) - static @Nonnull Pattern getMaskingPattern(@Nonnull Collection inputs) { - Collection patterns = new TreeSet<>(Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo)); + public static @Nonnull Pattern getAggregateSecretPattern(@Nonnull Collection inputs) { + Collection patterns = new TreeSet<>(EXTRACT_PATTERN); for (String input : inputs) { if (input.isEmpty()) { continue; } - patterns.add(Pattern.quote(input)); - patterns.addAll(getAllAlternateForms(input)); + patterns.add(SecretPatternFactory.quotedCompile(input)); + for (SecretPatternFactory provider : SecretPatternFactory.all()) { + patterns.addAll(provider.getSecretPatterns(input)); + } } - return Pattern.compile(String.join("|", patterns)); + String pattern = patterns.stream() + .sorted(BY_LENGTH_DESCENDING) + .map(Pattern::pattern) + .collect(Collectors.joining("|")); + return Pattern.compile(pattern); } } diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory2Test.java similarity index 97% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory2Test.java index 64a57ab6..708298f4 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory2Test.java @@ -40,8 +40,8 @@ import static org.jenkinsci.plugins.credentialsbinding.test.Executables.executable; import static org.junit.Assume.assumeThat; -@For(BashPatternMaskerProvider.class) -public class BashPatternMaskerProvider2Test { +@For(BashSecretPatternFactory.class) +public class BashSecretPatternFactory2Test { public @Rule JenkinsRule j = new JenkinsRule(); diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactoryTest.java similarity index 99% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactoryTest.java index 785be09d..e9f2fa48 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactoryTest.java @@ -53,7 +53,7 @@ import static org.junit.Assume.assumeThat; @RunWith(Theories.class) -public class BashPatternMaskerProviderTest { +public class BashSecretPatternFactoryTest { public static final @DataPoint String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; public static final @DataPoint String ANOTHER_SAMPLE_PASSWORD = "a'b\"c\\d(e)#"; diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactoryTest.java similarity index 99% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactoryTest.java index b14c7ea8..7a8a7211 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactoryTest.java @@ -41,7 +41,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assume.assumeTrue; -public class BatchPatternMaskerProviderTest { +public class BatchSecretPatternFactoryTest { private static final String SIMPLE = "abcABC123"; private static final String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellSecretPatternFactoryTest.java similarity index 98% rename from src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java rename to src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellSecretPatternFactoryTest.java index 11df0f6e..8516e13a 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellPatternMaskerProviderTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BourneShellSecretPatternFactoryTest.java @@ -51,7 +51,7 @@ import static org.junit.Assume.assumeThat; @RunWith(Theories.class) -public class BourneShellPatternMaskerProviderTest { +public class BourneShellSecretPatternFactoryTest { public static final @DataPoint String SAMPLE_PASSWORD = "}#T14'GAz&H!{$U_"; public static final @DataPoint String ANOTHER_SAMPLE_PASSWORD = "a'b\"c\\d(e)#"; From cf2178c44104307cf38f6d58f9adeb3f07e1dbb5 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 11:48:19 -0500 Subject: [PATCH 31/36] Remove redundant pattern This one was moved over to LiteralSecretPatternFactory Signed-off-by: Matt Sicker --- .../plugins/credentialsbinding/masking/SecretPatterns.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java index 4935c5fc..81c3579f 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java @@ -56,7 +56,6 @@ public class SecretPatterns { if (input.isEmpty()) { continue; } - patterns.add(SecretPatternFactory.quotedCompile(input)); for (SecretPatternFactory provider : SecretPatternFactory.all()) { patterns.addAll(provider.getSecretPatterns(input)); } From 89d9c746080a685655c72e9e191e7f18d39c8d61 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 11:49:25 -0500 Subject: [PATCH 32/36] Remove redundant pattern Signed-off-by: Matt Sicker --- .../credentialsbinding/masking/BashSecretPatternFactory.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java index 03dd2eac..a47bfeb5 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java @@ -65,7 +65,6 @@ public class BashSecretPatternFactory implements SecretPatternFactory { Collection patterns = new HashSet<>(); String quotedForm = getQuotedForm(input); patterns.add(SecretPatternFactory.quotedCompile(quotedForm)); - patterns.add(SecretPatternFactory.quotedCompile(quotedForm)); patterns.add(SecretPatternFactory.quotedCompile(surroundWithQuotes(quotedForm))); patterns.add(SecretPatternFactory.quotedCompile(getUnquotedForm(input))); return patterns; From c7d3cf67778afc681de2236d9fbb781e4bf4817e Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 11:55:47 -0500 Subject: [PATCH 33/36] Update log message to reflect masking improvements Signed-off-by: Matt Sicker --- .../jenkinsci/plugins/credentialsbinding/impl/BindingStep.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java index bbbceece..36b9ec82 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java @@ -138,7 +138,7 @@ private void doStart() throws Exception { } if (!overrides.isEmpty()) { boolean unix = launcher != null ? launcher.isUnix() : true; - listener.getLogger().println("Masking only exact matches of " + overrides.keySet().stream().map( + listener.getLogger().println("Masking supported pattern matches of " + overrides.keySet().stream().map( v -> unix ? "$" + v : "%" + v + "%" ).collect(Collectors.joining(" or "))); } From e2efe9a0e6ade1649f663517960935ded3089772 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 13:05:41 -0500 Subject: [PATCH 34/36] Restrict API Signed-off-by: Matt Sicker --- .../credentialsbinding/masking/SecretPatternFactory.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java index dd713676..9480a9a3 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java @@ -26,6 +26,8 @@ import hudson.ExtensionList; import hudson.ExtensionPoint; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import javax.annotation.Nonnull; import java.util.Collection; @@ -36,6 +38,7 @@ * These are typically implemented to handle various shell quoting algorithms (sometimes confused with escaping) to * pass literal string values to an interpreter. */ +@Restricted(NoExternalUse.class) public interface SecretPatternFactory extends ExtensionPoint { /** From 717d5db4b66b99036c15430c4d48232f018e75b9 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 13:33:23 -0500 Subject: [PATCH 35/36] Update test for log message update Signed-off-by: Matt Sicker --- .../plugins/credentialsbinding/impl/BindingStepTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java index fba27c4f..e8a12e17 100644 --- 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 { + "}", true)); WorkflowRun b = p.scheduleBuild2(0).waitForStart(); SemaphoreStep.waitForStart("basics/1", b); - story.j.assertLogContains(Functions.isWindows() ? "Masking only exact matches of %USERNAME% or %PASSWORD%" : "Masking only exact matches of $USERNAME or $PASSWORD", b); + story.j.assertLogContains(Functions.isWindows() ? "Masking supported pattern matches of %USERNAME% or %PASSWORD%" : "Masking supported pattern matches of $USERNAME or $PASSWORD", b); } }); story.addStep(new Statement() { From 163fdbaf3dfdb91cbdc26bec52ba6c35717ceab0 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Thu, 30 May 2019 13:33:34 -0500 Subject: [PATCH 36/36] Simplify API Signed-off-by: Matt Sicker --- .../masking/BashSecretPatternFactory.java | 10 ++++---- .../masking/BatchSecretPatternFactory.java | 4 ++-- .../masking/LiteralSecretPatternFactory.java | 5 ++-- .../masking/SecretPatternFactory.java | 15 +++--------- .../masking/SecretPatterns.java | 24 +++++++------------ 5 files changed, 21 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java index a47bfeb5..66cad78a 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java @@ -61,12 +61,12 @@ public class BashSecretPatternFactory implements SecretPatternFactory { } @Override - public @Nonnull Collection getSecretPatterns(@Nonnull String input) { - Collection patterns = new HashSet<>(); + public @Nonnull Collection getEncodedForms(@Nonnull String input) { + Collection patterns = new HashSet<>(); String quotedForm = getQuotedForm(input); - patterns.add(SecretPatternFactory.quotedCompile(quotedForm)); - patterns.add(SecretPatternFactory.quotedCompile(surroundWithQuotes(quotedForm))); - patterns.add(SecretPatternFactory.quotedCompile(getUnquotedForm(input))); + patterns.add(quotedForm); + patterns.add(surroundWithQuotes(quotedForm)); + patterns.add(getUnquotedForm(input)); return patterns; } } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactory.java index 479700ac..d200f5f2 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchSecretPatternFactory.java @@ -39,9 +39,9 @@ public class BatchSecretPatternFactory implements SecretPatternFactory { private static final Pattern QUOTED_CHARS = Pattern.compile("(\\^)(\\^?)"); @Override - public @Nonnull Collection getSecretPatterns(@Nonnull String input) { + public @Nonnull Collection getEncodedForms(@Nonnull String input) { return input.contains("^") - ? Collections.singleton(SecretPatternFactory.quotedCompile(QUOTED_CHARS.matcher(input).replaceAll("$2"))) + ? Collections.singleton(QUOTED_CHARS.matcher(input).replaceAll("$2")) : Collections.emptySet(); } } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java index 404b67c2..f961f510 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/LiteralSecretPatternFactory.java @@ -31,7 +31,6 @@ import javax.annotation.Nonnull; import java.util.Collection; import java.util.Collections; -import java.util.regex.Pattern; /** * Trivial secret pattern factory that matches the literal value of the secret. @@ -41,7 +40,7 @@ public class LiteralSecretPatternFactory implements SecretPatternFactory { @Nonnull @Override - public Collection getSecretPatterns(@Nonnull String input) { - return Collections.singleton(SecretPatternFactory.quotedCompile(input)); + public Collection getEncodedForms(@Nonnull String input) { + return Collections.singleton(input); } } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java index 9480a9a3..3c54920d 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java @@ -31,10 +31,9 @@ import javax.annotation.Nonnull; import java.util.Collection; -import java.util.regex.Pattern; /** - * Creates regular expressions to match encoded forms of secrets in logs. + * Provides encoded forms to an input for use in masking those forms in logs. * These are typically implemented to handle various shell quoting algorithms (sometimes confused with escaping) to * pass literal string values to an interpreter. */ @@ -42,10 +41,9 @@ public interface SecretPatternFactory extends ExtensionPoint { /** - * Returns a collection of alternative forms the given input may show up as in logs. - * Note that these patterns must embed their flags in the pattern rather than as parameters to Pattern. + * Returns a collection of alternative forms the given input may be encoded as in logs. */ - @Nonnull Collection getSecretPatterns(@Nonnull String input); + @Nonnull Collection getEncodedForms(@Nonnull String input); /** * Returns all SecretPatternFactory extensions known at runtime. @@ -54,11 +52,4 @@ public interface SecretPatternFactory extends ExtensionPoint { return ExtensionList.lookup(SecretPatternFactory.class); } - /** - * Composes {@link Pattern#compile(String)} and {@link Pattern#quote(String)} for convenience. - */ - static @Nonnull Pattern quotedCompile(@Nonnull String input) { - return Pattern.compile(Pattern.quote(input)); - } - } diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java index 81c3579f..632ba9e3 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java @@ -30,16 +30,14 @@ import javax.annotation.Nonnull; import java.util.Collection; import java.util.Comparator; -import java.util.TreeSet; import java.util.regex.Pattern; import java.util.stream.Collectors; @Restricted(NoExternalUse.class) public class SecretPatterns { - private static final Comparator EXTRACT_PATTERN = Comparator.comparing(Pattern::pattern); - private static final Comparator BY_LENGTH_DESCENDING = Comparator.comparing(Pattern::pattern, - Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo)); + private static final Comparator BY_LENGTH_DESCENDING = + Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo); /** * Constructs a regular expression to match against all known forms that the given collection of input strings may @@ -51,18 +49,14 @@ public class SecretPatterns { * absence of quoting, the longer form is masked. */ public static @Nonnull Pattern getAggregateSecretPattern(@Nonnull Collection inputs) { - Collection patterns = new TreeSet<>(EXTRACT_PATTERN); - for (String input : inputs) { - if (input.isEmpty()) { - continue; - } - for (SecretPatternFactory provider : SecretPatternFactory.all()) { - patterns.addAll(provider.getSecretPatterns(input)); - } - } - String pattern = patterns.stream() + String pattern = inputs.stream() + .filter(input -> !input.isEmpty()) + .flatMap(input -> + SecretPatternFactory.all().stream().flatMap(factory -> + factory.getEncodedForms(input).stream())) .sorted(BY_LENGTH_DESCENDING) - .map(Pattern::pattern) + .distinct() + .map(Pattern::quote) .collect(Collectors.joining("|")); return Pattern.compile(pattern); }