From 224f989697817dc5ff9f0499cd6c17d38ca9507d Mon Sep 17 00:00:00 2001 From: gallardo Date: Fri, 1 Dec 2023 17:50:12 +0100 Subject: [PATCH 1/3] Demonstrate wrong masking of one-char-secrets Test to reproduce issue JENKINS-72412. --- .../impl/BindingStepTest.java | 19 +++++++++++++++++++ .../Base64SecretPatternFactoryTest.java | 16 ++++++++++++++++ 2 files changed, 35 insertions(+) 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 810d4743..1006d565 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java @@ -321,6 +321,25 @@ public void widerRequiredContext() throws Throwable { }); } + @Issue("JENKINS-72412") + @Test public void maskingOfOneCharSecretShouldNotMangleOutput() throws Throwable { + rr.then(r -> { + String credentialsId = "creds"; + String secret = "a"; + CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), new StringCredentialsImpl(CredentialsScope.GLOBAL, credentialsId, "sample", Secret.fromString(secret))); + WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition("" + + "node {\n" + + " withCredentials([string(credentialsId: '" + credentialsId + "', variable: 'SECRET')]) {\n" + // forgot set +x, ran /usr/bin/env, etc. + + " if (isUnix()) {sh 'echo $SECRET > oops'} else {bat 'echo %SECRET% > oops'}\n" + + " }\n" + + "}", true)); + WorkflowRun b = r.assertBuildStatusSuccess(p.scheduleBuild2(0).get()); + r.assertLogContains("echo", b); + }); + } + @Issue("JENKINS-30326") @Test public void testGlobalBindingWithAuthorization() throws Throwable { diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java index 1e48690a..039ddd6f 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java @@ -9,10 +9,14 @@ import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; +import java.util.Collection; +import java.util.List; + public class Base64SecretPatternFactoryTest { @Rule @@ -52,4 +56,16 @@ public void base64SecretsAreMaskedInLogs() throws Exception { j.assertLogContains("****", run); j.assertLogNotContains(SAMPLE_PASSWORD, run); } + + @Test + public void base64FormsAreNotEmpty() { + Base64SecretPatternFactory factory = new Base64SecretPatternFactory(); + List secrets = List.of("", "s", "s3", "s3cr3t"); + + secrets.forEach(secret -> { + Collection base64Forms = factory.getBase64Forms(secret); + Assert.assertTrue("Failed for secret: '" + secret + "'. Method returned an empty string.", + base64Forms.stream().noneMatch(String::isEmpty)); + }); + } } \ No newline at end of file From 89bbe4e9df9fd171fc63018dcefceaad4648b7ba Mon Sep 17 00:00:00 2001 From: gallardo Date: Fri, 1 Dec 2023 17:51:08 +0100 Subject: [PATCH 2/3] Fix wrong masking of one-char-secrets Fixes JENKINS-72412. --- .../masking/Base64SecretPatternFactory.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java index 96063519..36bf7147 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java @@ -38,8 +38,13 @@ public Collection getBase64Forms(@NonNull String secret) { String shiftedSecret = shift + secret; String encoded = encoder.encodeToString(shiftedSecret.getBytes(StandardCharsets.UTF_8)); String processedEncoded = shift.length() > 0 ? encoded.substring(2 * shift.length()) : encoded; - result.add(processedEncoded); - result.add(removeTrailingEquals(processedEncoded)); + if (!processedEncoded.isEmpty()) { + result.add(processedEncoded); + } + String processedWithoutTrailingEquals = removeTrailingEquals(processedEncoded); + if (!processedWithoutTrailingEquals.isEmpty()) { + result.add(removeTrailingEquals(processedEncoded)); + } } } return result; From 46a32f85b621a0f201e6187cf7cb38a4fe40221f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 24 Apr 2024 17:08:23 -0400 Subject: [PATCH 3/3] More general fix: `MINIMUM_ENCODED_LENGTH` --- .../masking/Base64SecretPatternFactory.java | 9 ++------- .../masking/SecretPatterns.java | 7 +++++++ .../masking/Base64SecretPatternFactoryTest.java | 16 ---------------- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java index 36bf7147..96063519 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactory.java @@ -38,13 +38,8 @@ public Collection getBase64Forms(@NonNull String secret) { String shiftedSecret = shift + secret; String encoded = encoder.encodeToString(shiftedSecret.getBytes(StandardCharsets.UTF_8)); String processedEncoded = shift.length() > 0 ? encoded.substring(2 * shift.length()) : encoded; - if (!processedEncoded.isEmpty()) { - result.add(processedEncoded); - } - String processedWithoutTrailingEquals = removeTrailingEquals(processedEncoded); - if (!processedWithoutTrailingEquals.isEmpty()) { - result.add(removeTrailingEquals(processedEncoded)); - } + result.add(processedEncoded); + result.add(removeTrailingEquals(processedEncoded)); } } return result; 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 f1f90f25..829d4620 100644 --- a/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java +++ b/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java @@ -44,6 +44,12 @@ public class SecretPatterns { private static final Comparator BY_LENGTH_DESCENDING = Comparator.comparingInt(String::length).reversed().thenComparing(String::compareTo); + /** + * Masking (encoded) “secrets” shorter than this would just be ridiculous. + * Particularly relevant with {@link Base64SecretPatternFactory}. + */ + private static final int MINIMUM_ENCODED_LENGTH = 3; + /** * 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 @@ -61,6 +67,7 @@ public class SecretPatterns { .flatMap(input -> secretPatternFactories.stream().flatMap(factory -> factory.getEncodedForms(input).stream())) + .filter(encoded -> encoded.length() >= MINIMUM_ENCODED_LENGTH) .sorted(BY_LENGTH_DESCENDING) .distinct() .map(Pattern::quote) diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java index 039ddd6f..1e48690a 100644 --- a/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java +++ b/src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/Base64SecretPatternFactoryTest.java @@ -9,14 +9,10 @@ import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; -import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; -import java.util.Collection; -import java.util.List; - public class Base64SecretPatternFactoryTest { @Rule @@ -56,16 +52,4 @@ public void base64SecretsAreMaskedInLogs() throws Exception { j.assertLogContains("****", run); j.assertLogNotContains(SAMPLE_PASSWORD, run); } - - @Test - public void base64FormsAreNotEmpty() { - Base64SecretPatternFactory factory = new Base64SecretPatternFactory(); - List secrets = List.of("", "s", "s3", "s3cr3t"); - - secrets.forEach(secret -> { - Collection base64Forms = factory.getBase64Forms(secret); - Assert.assertTrue("Failed for secret: '" + secret + "'. Method returned an empty string.", - base64Forms.stream().noneMatch(String::isEmpty)); - }); - } } \ No newline at end of file