Adjust credentials masking tests for base64 masking improvement

[MarkEWaite]

Adjust credentials masking tests for base64 masking improvement

Credentials masking now includes masking of base64 credentials. The tests were using a very short user name, "bot". The base64 encoding of that short user name matches with other log content and causes the tests to fail.

Use a longer user name and a longer password in the test so that spurious matches are much less likely.

Testing done

Confirmed the test fails when I run without b36d0a7

mvn clean -Dtest=MavenSettingsConfigTest verify

Confirmed that tests pass when I include b36d0a7

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[MarkEWaite]

More details are available in:

• config-file-provider plugin fails MavenSettingsConfigTest.freestyleWithCredentials test bom#2513
• Pin previous credentials-binding release for LTS profiles bom#2514

[jglick]

Sorry, accidentally clicked Re-run! FTR https://github.com/jenkinsci/config-file-provider-plugin/pull/296/checks?check_run_id=16858023665

java.io.IOException: Illegal base64 character 0x2a
	at java.base/java.util.Base64$DecInputStream.read(Base64.java:1160)
	at java.base/java.io.DataInputStream.read(DataInputStream.java:151)
	at java.base/java.io.DataInputStream.readFully(DataInputStream.java:201)
	at hudson.util.IOUtils.skip(IOUtils.java:90)
	at hudson.console.ConsoleNote.skip(ConsoleNote.java:311)
	at hudson.console.PlainTextConsoleOutputStream.eol(PlainTextConsoleOutputStream.java:67)
	at hudson.console.LineTransformationOutputStream.eol(LineTransformationOutputStream.java:61)
	at hudson.console.LineTransformationOutputStream.write(LineTransformationOutputStream.java:57)
	at hudson.console.LineTransformationOutputStream.write(LineTransformationOutputStream.java:75)
	at org.apache.commons.io.output.ProxyOutputStream.write(ProxyOutputStream.java:92)
	at org.kohsuke.stapler.framework.io.LargeText.writeLogTo(LargeText.java:233)
	at hudson.console.AnnotatedLargeText.writeLogTo(AnnotatedLargeText.java:164)
	at org.jvnet.hudson.test.JenkinsRule.getLog(JenkinsRule.java:1536)
	at org.jvnet.hudson.test.JenkinsRule.assertLogNotContains(JenkinsRule.java:1525)
	at org.jenkinsci.plugins.configfiles.maven.MavenSettingsConfigTest.freestyleWithCredentials(MavenSettingsConfigTest.java:157)

Regardless of whether the regression is caused by jenkinsci/credentials-binding-plugin#269, I think the error is also fixed by jenkinsci/jenkins#8378.

[MarkEWaite]

Regardless of whether the regression is caused by jenkinsci/credentials-binding-plugin#269, I think the error is also fixed by jenkinsci/jenkins#8378.

That explains why the failure is not visible in the weekly release line and is only visible in the LTS release line. Thanks very much for pointing to that change. I've confirmed that this pull request passes when built with:

mvn -Djenkins.version=2.420 -Dtest=MavenSettingsConfigTest verify

It fails when built with:

mvn -Djenkins.version=2.419 -Dtest=MavenSettingsConfigTest verify

Jenkins 2.420 is the first weekly release that includes the fix in jenkinsci/jenkins#8378.

I think that the next baseline Jenkins version for the config-file-provider plugin should be 2.387.3 so that it does not use too new a version. If the next baseline Jenkins version for the config-file-provider plugin is anything less than Jenkins 2.420, then I think we will want some change to the config-file-provider tests so that they pass on the LTS baselines. That change could be as simple as "skip the test if the Jenkins version is too old", but I think that we need the tests to pass.

Does that make sense to you @jglick or is there a better way to handle the failing config-file-provider tests when they run with the current credentials binding plugin in an older Jenkins release?

[jglick]

jenkinsci/jenkins#8378 merely prevents a corrupted console note from breaking log streaming. Why the note became corrupted to begin with is another question; it sounds like jenkinsci/credentials-binding-plugin#269 introduced a genuine regression to production code, by accidentally masking some snippet of serialized ConsoleNote that was unrelated to any secrets in scope. Perhaps the secret was very short and its Base64 form happens to appear in other unrelated contexts by coincidence, in which case there should be some minimum length requirement perhaps. Someone will need to debug the test to see exactly what is getting masked and why.

[Kevin-CB]

accidentally masking some snippet of serialized ConsoleNote that was unrelated to any secrets in scope. Perhaps the secret was very short and its Base64 form happens to appear in other unrelated contexts by coincidence

That's exactly it! In this test, one of the secrets is bot, which in one of its base64 forms without padding is b3.
As a result, it gets masked in the console output, leading to corrupted base64:

Building remotely on �[8mha:////4DoiLVFH0[...]SjKDWzX****RdlLBUSYG[...]eFxQAAAA==�[0mslave0 in workspace 

[Kevin-CB]

Does that make sense to you @jglick or is there a better way to handle the failing config-file-provider tests when they run with the current credentials binding plugin in an older Jenkins release?

Using a longer secret for bot should fix this issue; something like bot123 works fine for me.

[MarkEWaite]

I've modified the tests as suggested by @Kevin-CB and confirmed that it now passes with Jenkins 2.419. Changes are in b36d0a7

[MarkEWaite]

Does that make sense to you @jglick or is there a better way to handle the failing config-file-provider tests when they run with the current credentials binding plugin in an older Jenkins release?

Using a longer secret for bot should fix this issue; something like bot123 works fine for me.

@Kevin-CB do you also want to consider the addition of a minimum length criteria to the base64 masking in the credentials binding plugin? I think that the suggestion from @jglick may help avoid user complaints when they use a very short user name or a very short password.

[MarkEWaite]

@alecharp if you'd be willing to label this as developer, review it, and if approved, merge it, that will allow me to remove the plugin bill of materials workaround that I inserted in:

• Pin previous credentials-binding release for LTS profiles bom#2514

You're welcome to squash merge this change (once it has been reviewed and approved) so that the git history remains tidy.

[alecharp]

Looks simple enough. Thank you @MarkEWaite.

[jglick]

do you also want to consider the addition of a minimum length criteria to the base64 masking

This might be prudent, though you also have to ask why you are trying to mask such short secrets to begin with—clearly they would not be secure. For usernames, we guide users to disable username masking for newly created credentials, but continue to mask older credentials unless and until the user ops out.

The particular issue could I think also be addressed by disabling the Base64 masker on lines containing the magic header sequence for ConsoleNote, since we would never expect a user secret to occur (much less in Base64-encoded form) in a line that contains a note. That would prevent notes from becoming corrupted. You could still have spurious masking of short sequences in other parts of the log which would be annoying; of course this could also occur with the literal pattern factory, if you just happened to have a “password” like 123 that winds up occurring in all sorts of unrelated places.

[Kevin-CB]

I'm tracking this issue under JENKINS-72035.