[JENKINS-42950] Support more credential masking scenarios #59

jvz · 2019-05-02T19:23:28Z

This adds a new extension point, SecretPatternFactory, 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

@reviewbybees @jeffret-b @jglick @jtnord @daniel-beck

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 <wadeck.follonier@gmail.com>

src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html

Wadeck

Test failures

[2019-05-02T19:31:42.909Z] [ERROR] org.jenkinsci.plugins.credentialsbinding.impl.CredentialsMaskingBashTest.credentialsAreMaskedInLogs(org.jenkinsci.plugins.credentialsbinding.impl.CredentialsMaskingBashTest)

[2019-05-02T19:31:42.909Z] [ERROR] Run 1: CredentialsMaskingBashTest.credentialsAreMaskedInLogs credentialsAreMaskedInLogs("PjsQ&^S6nVl!NZM3RQ6)J6" <from generatePasswords[0]>)

[2019-05-02T19:31:42.909Z] [ERROR] Run 2: CredentialsMaskingBashTest.credentialsAreMaskedInLogs credentialsAreMaskedInLogs(")4a= (nG" <from generatePasswords[0]>)

[2019-05-02T19:31:42.909Z] [ERROR] Run 3: CredentialsMaskingBashTest.credentialsAreMaskedInLogs credentialsAreMaskedInLogs("YI'1JHTC{>20lSz)'.7VS5b6" <from generatePasswords[0]>)

[2019-05-02T19:31:42.909Z] [ERROR] Run 4: CredentialsMaskingBashTest.credentialsAreMaskedInLogs credentialsAreMaskedInLogs("UQjhT6"Oy04Uv{5>Op"UZ,oHh0<%" <from generatePasswords[0]>)

[2019-05-02T19:31:42.909Z] [ERROR] Run 5: CredentialsMaskingBashTest.credentialsAreMaskedInLogs credentialsAreMaskedInLogs("JR{r'u@*D<Ld"Kj^N5dkwZ*" <from generatePasswords[0]>)

Some comments in addition, esp. around test

src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProvider.java

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java

src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingBashTest.java

...est/java/org/jenkinsci/plugins/credentialsbinding/impl/CredentialsMaskingPowerShellTest.java

...est/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java

Signed-off-by: Matt Sicker <boards@gmail.com>

Co-Authored-By: jvz <boards@gmail.com>

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2019-05-03T17:25:12Z

Alright, I think I've addressed all your feedback. Not sure why CI is failing as that's giving a different output than my own testing locally for bash.

Signed-off-by: Matt Sicker <boards@gmail.com>

jglick · 2019-05-03T19:54:34Z

Not sure why CI is failing as that's giving a different output than my own testing locally for bash.

Perhaps because the CI build is running on Ubuntu. Your test seems to assume without checking that sh is actually Bash, which it is on some less-compliant Linux distributions or platforms.

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2019-05-03T20:23:36Z

Not sure why CI is failing as that's giving a different output than my own testing locally for bash.

Perhaps because the CI build is running on Ubuntu. Your test seems to assume without checking that sh is actually Bash, which it is on some less-compliant Linux distributions or platforms.

Good catch. I've updated the tests to change the default shell to bash.

Seems as though the theories runner fails otherwise if the assumption is violated. Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2019-05-03T20:36:51Z

Now I'm confused as to what's failing this time.

Wadeck · 2019-05-06T11:50:15Z

...st/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java

@@ -52,6 +54,7 @@
    @Before
    public void setUp() {
        assumeThat("bash", is(executable()));
+        j.jenkins.getDescriptorByType(Shell.DescriptorImpl.class).setShell(Executables.getPathToExecutable("bash"));


Due to that, you remove the test for "pure Shell" right? But we want to support it along Bash, Batch and PowerShell.

Not sure what the issue is—this is a new test suite.

jvz · 2019-05-06T13:41:40Z

Non-bash implementations of Bourne shell do not perform any quoting of echoed commands. It works more like Windows Batch where it just echoes out what you typed. I don’t think it’s documented here, but I did find that during discovery.

On Mon, May 6, 2019 at 06:50, Wadeck Follonier ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/test/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProvider2Test.java <#59 (comment)> : > @@ -52,6 +54,7 @@ @before public void setUp() { assumeThat("bash", is(executable())); + j.jenkins.getDescriptorByType(Shell.DescriptorImpl.class).setShell(Executables.getPathToExecutable("bash")); Due to that, you remove the test for "pure Shell" right? But we want to support it along Bash, Batch and PowerShell. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#59 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGBF2ZOKHXXZMIRZYRALD3PUALP3ANCNFSM4HKFF2XA> .

-- Matt Sicker Senior Software Engineer, CloudBees

Wadeck · 2019-05-07T06:48:01Z

@jvz Re Shell => so a non-regression test can be written for such case easily

jvz · 2019-05-28T18:46:47Z

Can I get one of the maintainers to merge? @abayer @jglick @jtnord @batmat @Vlatombe @stephenc

jglick · 2019-05-28T18:47:29Z

That would be me. I have not yet had time to review.

jglick · 2019-05-29T19:25:43Z

See also #55.

jglick

Some questions and minor comments. Basically looks good to me except for the one decision to bake Pattern.quote into the SPI, which seems unnecessary and undesirable.

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java

jglick · 2019-05-29T19:55:43Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/PatternMaskerProvider.java

+     * 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.


Can you give an example of a situation where the presence or absence of quoting would be leaked otherwise? I am not sure I follow this logic. Just as an aside.

...est/java/org/jenkinsci/plugins/credentialsbinding/masking/BashPatternMaskerProviderTest.java

jglick · 2019-05-29T20:12:06Z

...st/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java

+
+    @Test
+    public void simple_singleQuote() throws Exception {
+        registerCredentials(SIMPLE);


Why is the Bash version using the theory system, and this is done manually? Smells like there could be some consolidation of test code.

Different test authors originally. I can likely figure out how to break this into a theorized test.

...st/java/org/jenkinsci/plugins/credentialsbinding/masking/BatchPatternMaskerProviderTest.java

src/test/java/org/jenkinsci/plugins/credentialsbinding/test/Executables.java

stephenc

I do not feel there is any value I can add by reviewing

jglick

Oh and you need to either delete or modify the message printed from BindingStep after #55, since we are no longer masking only exact matches.

Co-Authored-By: Jesse Glick <jglick@cloudbees.com>

Signed-off-by: Matt Sicker <boards@gmail.com>

This removes the exposure of Pattern.quote() in the consumer calls along with other cleanups around this idea. Signed-off-by: Matt Sicker <boards@gmail.com>

This one was moved over to LiteralSecretPatternFactory Signed-off-by: Matt Sicker <boards@gmail.com>

Signed-off-by: Matt Sicker <boards@gmail.com>

blocker supposedly addressed in 9bd38ca

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2019-05-30T16:56:42Z

I've addressed most of your review comments, @jglick. I'll take a look at improving the test differences later today.

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz · 2019-05-30T18:22:14Z

Updating the log message has tests? Color me surprised, ok. I'll also have a fix for that.

Signed-off-by: Matt Sicker <boards@gmail.com>

jglick · 2019-05-31T12:46:47Z

src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java

@@ -146,7 +146,7 @@
                        + "}", 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);


Feel free to introduce a package-private utility method for this message to be shared between src/main/ and src/test/.

jglick · 2019-05-31T12:51:13Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/BashSecretPatternFactory.java

+        patterns.add(quotedForm);
+        patterns.add(surroundWithQuotes(quotedForm));
+        patterns.add(getUnquotedForm(input));
+        return patterns;


or a bit more simply

return Arrays.asList(quotedForm, surroundWithQuotes(quotedForm), getUnquotedForm(input));

jglick · 2019-05-31T12:53:25Z

src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html

-    Mangled secrets are not detected by Jenkins and will appear in plain text in the log file, in this case as:
+    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:


BTW for natural-language text files it is a good idea to use one physical line per sentence or major phrase. Easier to review & edit.

jglick requested review from jglick, Wadeck, jeffret-b, daniel-beck and jtnord May 2, 2019 21:37

daniel-beck reviewed May 2, 2019

View reviewed changes

src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html Outdated Show resolved Hide resolved

daniel-beck reviewed May 2, 2019

View reviewed changes

src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html Outdated Show resolved Hide resolved

daniel-beck reviewed May 2, 2019

View reviewed changes

src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html Outdated Show resolved Hide resolved

Wadeck requested changes May 3, 2019

View reviewed changes

jvz and others added 10 commits May 3, 2019 11:14

Improve assertions

3fb359e

Signed-off-by: Matt Sicker <boards@gmail.com>

Move package and add link to relevant PR

ee51298

Signed-off-by: Matt Sicker <boards@gmail.com>

Rename test

2273356

Signed-off-by: Matt Sicker <boards@gmail.com>

Rename test

19fec43

Signed-off-by: Matt Sicker <boards@gmail.com>

Rename tests and improve some

6dcc230

Signed-off-by: Matt Sicker <boards@gmail.com>

Improve help docs

cd94206

Co-Authored-By: jvz <boards@gmail.com>

Simplify set class usage

c82bcc3

Signed-off-by: Matt Sicker <boards@gmail.com>

Improve javadoc

fa4ff53

Signed-off-by: Matt Sicker <boards@gmail.com>

Improve help docs

5f55aca

Signed-off-by: Matt Sicker <boards@gmail.com>

Add batch test for fancier password

a432974

Signed-off-by: Matt Sicker <boards@gmail.com>

Fix assertion

0dde12f

Signed-off-by: Matt Sicker <boards@gmail.com>

Improve tests to work with bash properly

30d0900

Signed-off-by: Matt Sicker <boards@gmail.com>

Move bash assumption to BeforeClass

ad1b6aa

Seems as though the theories runner fails otherwise if the assumption is violated. Signed-off-by: Matt Sicker <boards@gmail.com>

Wadeck reviewed May 6, 2019

View reviewed changes

daniel-beck approved these changes May 28, 2019

View reviewed changes

daniel-beck requested a review from stephenc May 29, 2019 14:10

jglick requested changes May 29, 2019

View reviewed changes

stephenc reviewed May 30, 2019

View reviewed changes

jglick previously requested changes May 30, 2019

View reviewed changes

jvz and others added 9 commits May 30, 2019 10:43

Remove unneeded TODO

c707d92

Co-Authored-By: Jesse Glick <jglick@cloudbees.com>

Clean up javadoc

7b05cd2

Co-Authored-By: Jesse Glick <jglick@cloudbees.com>

Apply suggestions from code review

f7690e0

Co-Authored-By: Jesse Glick <jglick@cloudbees.com>

Re-add docs on set +x

c9938de

Signed-off-by: Matt Sicker <boards@gmail.com>

Fix imports

14f8c62

Signed-off-by: Matt Sicker <boards@gmail.com>

Use @ClassRule JenkinsRule and improve comments

4ebbd28

Signed-off-by: Matt Sicker <boards@gmail.com>

Refactor masking API to use Pattern

9bd38ca

This removes the exposure of Pattern.quote() in the consumer calls along with other cleanups around this idea. Signed-off-by: Matt Sicker <boards@gmail.com>

Remove redundant pattern

cf2178c

This one was moved over to LiteralSecretPatternFactory Signed-off-by: Matt Sicker <boards@gmail.com>

Remove redundant pattern

89d9c74

Signed-off-by: Matt Sicker <boards@gmail.com>

jglick self-requested a review May 30, 2019 16:50

Update log message to reflect masking improvements

c7d3cf6

Signed-off-by: Matt Sicker <boards@gmail.com>

jglick requested changes May 30, 2019

View reviewed changes

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatternFactory.java Outdated Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java Outdated Show resolved Hide resolved

Restrict API

e2efe9a

Signed-off-by: Matt Sicker <boards@gmail.com>

jvz added 2 commits May 30, 2019 13:33

Update test for log message update

717d5db

Signed-off-by: Matt Sicker <boards@gmail.com>

Simplify API

163fdba

Signed-off-by: Matt Sicker <boards@gmail.com>

jglick approved these changes May 31, 2019

View reviewed changes

jglick merged commit 9bed48e into jenkinsci:master May 31, 2019

jvz deleted the credentials-masking branch May 31, 2019 14:31

jglick mentioned this pull request Jun 23, 2021

Introduced MaskingOutputStream to DRY between Pipeline & freestyle #139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-42950] Support more credential masking scenarios #59

[JENKINS-42950] Support more credential masking scenarios #59

jvz commented May 2, 2019 •

edited by jglick

Loading

Wadeck left a comment

jvz commented May 3, 2019

jglick commented May 3, 2019

jvz commented May 3, 2019

jvz commented May 3, 2019

Wadeck May 6, 2019

jglick May 29, 2019

jvz commented May 6, 2019 via email

Wadeck commented May 7, 2019

jvz commented May 28, 2019

jglick commented May 28, 2019

jglick commented May 29, 2019

jglick left a comment

jglick May 29, 2019

jglick May 29, 2019

jvz May 30, 2019

stephenc left a comment

jglick left a comment

jvz commented May 30, 2019

jvz commented May 30, 2019

jglick May 31, 2019

jglick May 31, 2019

jglick May 31, 2019

[JENKINS-42950] Support more credential masking scenarios #59

[JENKINS-42950] Support more credential masking scenarios #59

Conversation

jvz commented May 2, 2019 • edited by jglick Loading

Wadeck left a comment

Choose a reason for hiding this comment

jvz commented May 3, 2019

jglick commented May 3, 2019

jvz commented May 3, 2019

jvz commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz commented May 6, 2019 via email

Wadeck commented May 7, 2019

jvz commented May 28, 2019

jglick commented May 28, 2019

jglick commented May 29, 2019

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

jvz commented May 30, 2019

jvz commented May 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz commented May 2, 2019 •

edited by jglick

Loading