[JENKINS-75914] Do not mask text less than 2 characters

[MarkEWaite]

[JENKINS-75914] Do not mask text less than 2 characters

The bug submitter was using a zero length secret text and it significantly increased the log output. Require that the matched regular expression pattern must contain at least two characters.

Testing done

• Wrote automated tests to confirm original exception is returned when string is less than 3 characters
• Ran the revised plugin in Jenkins and confirmed that the excess asterisks are not displayed

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 demonstrate the feature works or the issue is fixed

[pzygielo]

Just out of curiosity - what is the value/purpose of using 4 (constant) number of replacement characters?

credentials-binding-plugin/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/MaskedException.java

Line 27 in 1f5aab3

var masked = new MaskedException(pattern.matcher(Objects.requireNonNullElse(unmasked.getMessage(), "")).replaceAll("****"));

Wouldn't 1 do just fine?

[MarkEWaite]

Just out of curiosity - what is the value/purpose of using 4 (constant) number of replacement characters?

credentials-binding-plugin/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/MaskedException.java

Line 27 in 1f5aab3

var masked = new MaskedException(pattern.matcher(Objects.requireNonNullElse(unmasked.getMessage(), "")).replaceAll("****"));

Wouldn't 1 do just fine?

I believe that the choice of "****" as the replacement text is for the convenience of the person reading the log. It is easier to detect a sequence of characters than a single character. One of the stated goals of the pattern used to match the masked text is that it should prefer the longest match. By using 4 characters, we don't disclose the length of the matched text and we still provide enough text that a person scanning the log file is likely to see the "****" string as indicating sensitive information that has been masked.

[jtnord]

possibly doesn't work when you have bound multiple credentials (and one is empty?) as the pattern will be built from all of them and still match?
I would rather suggest that the pattern/masker is not even created for an empty credential / match.

Additionally why 2 characters here - is that different to what is masked in the logs? is a user warned in the credential plugin when creating a that a single character password will not be masked?

[MarkEWaite]

I wonder if a different heuristic (fallible rule) would be a better choice. If the size of the masked exception message is more than 10% larger than the original exception, then we refuse to mask the original exception. That would replace the heuristic that uses regex pattern length with a heuristic that tries to avoid the specific case that was seen at Eclipse where the size of the logged exception message was much larger than the original exception message.

What do you think of that alternative?

[jtnord]

wonder if a different heuristic (fallible rule) would be a better choice. If the size of the masked exception message is more than 10% larger than the original exception, then we refuse to mask the original exception. That would replace the heuristic that uses regex pattern length with a heuristic that tries to avoid the specific case that was seen at Eclipse where the size of the logged exception message was much larger than the original exception message.

What do you think of that alternative?

I am wondering why the console log did not blow up before this change (ie we previously still masked the console log, the security change just added masking of an exception). Is there a reason we can not apply the same logic, perhaps @Kevin-CB recalls?

If the regular console masks a single character (which I believe it does) then I would say the masked exception should also do the same - the whole point of this code is to behave the same and mask what would have been masked in the console to prevent leakage.
A single character secret is insecure and should not be used (you can extrapolate that to small length too), and would create interesting masked console output.

• TBH not sure why an empty file is any better but well - that should not cause any masking just like in the console.

I would say that heuristics shouldn't be used, we just need the behaviour to match the console.

[jtnord]

credentials-binding-plugin/src/main/java/org/jenkinsci/plugins/credentialsbinding/masking/SecretPatterns.java

Lines 62 to 76 in baac667

	public static @NonNull Pattern getAggregateSecretPattern(@NonNull Collection<String> inputs) {
	JenkinsJVM.checkJenkinsJVM();
	List<SecretPatternFactory> secretPatternFactories = SecretPatternFactory.all();
	String pattern = inputs.stream()
	.filter(input -> !input.isEmpty())
	.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)
	.collect(Collectors.joining("|"));
	return Pattern.compile(pattern);
	}

something strange is going on

[Kevin-CB]

I am wondering why the console log did not blow up before this change (ie we previously still masked the console log, the security change just added masking of an exception). Is there a reason we can not apply the same logic, perhaps @Kevin-CB recalls?

There is a check for empty credentials for regular messages on the console log, which wasn't implemented for exceptions.

[pzygielo]

I believe that the choice of "****" as the replacement text is for the convenience of the person reading the log.

Right. In regular case

My secret is **** and that's it

indeed is better with ****.

I was under the influence of the result with empty pattern, which gives

****M****y**** ****s****e****c****r****e****t**** ****i****s**** **** ****(****e****m****p****t****y**** ****s****t****r****i****n****g****)**** ****a****n****d**** ****t****h****a****t****'****s**** ****i****t****

It is easier to detect a sequence of characters than a single character.

Got it. Thanks.

[Kevin-CB]

the whole point of this code is to behave the same and mask what would have been masked in the console to prevent leakage.

I agree with Jesse James, the behavior should be consistent. I filed #378 to handle empty credentials in the same way.

[jtnord]

I agree with Jesse, the behavior should be consistent.

🙄

I filed #378 to handle empty credentials in the same way.

closing in favour of #378