Make the DelegatingPasswordEncoder work correctly, even if the prefix and suffix are the same

[ghoonch4]

You can inject prefix and suffix when calling the constructor of DelegatingPasswordEncoder. And you can also inject the "same" prefix and suffix. In this case the instantiation of DelegatingPasswordEncoder works fine, but calling the matches(CharSequence, String) method throws an exception. There is a bug in the methods extractId(String) and extractEncodedPassword(String).

[sjohnr]

Hi @ghoonch4, I apologize for the delay on a review. Note that often it's best to open an issue to discuss potential changes prior to submitting a PR.

Can you go into detail about why this change is necessary, meaning what use case exists for using the same prefix/suffix? Is the goal to prevent user confusion? Could it also be solved by rejecting the same prefix/suffix? Should the documentation be enhanced to prevent confusion?

[ghoonch4]

According to the DelegatingPasswordEncoder documentation, prefix and suffix can be customized.

spring-security/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java

Lines 53 to 58 in 0a2a327

	* Such that "id" is an identifier used to look up which {@link PasswordEncoder} should be
	* used and "encodedPassword" is the original encoded password for the selected
	* {@link PasswordEncoder}. The "id" must be at the beginning of the password, start with
	* "{" (id prefix) and end with "}" (id suffix). Both id prefix and id suffix can be
	* customized via {@link #DelegatingPasswordEncoder(String, Map, String, String)}. If the
	* "id" cannot be found, the "id" will be null.

This seems to have been added in gh-10278. It means users can use any prefix and suffix they want instead of the default prefix {, default suffix }. And the DelegatingPasswordEncoder instance can be created like below:

public static PasswordEncoder createDelegatingPasswordEncoder() {
    Map<String, PasswordEncoder> encoders = new HashMap<>();
    encoders.put("bcrypt", new BCryptPasswordEncoder());
    encoders.put("some", new SomePasswordEncoder());
    return new DelegatingPasswordEncoder("bcrypt", encoders, "$", "$");
}

The instance is created correctly. And users can call the matches(String, String) method to verify that the password match:

PasswordEncoder passwordEncoder = createDelegatingPasswordEncoder();
boolean matches = passwordEncoder.matches("password", "$bcrypt$$2a$10$dXJ3SW6G7P50lGmMkkmwe.20cQQubK3.HZWzG3YB1tlRy.fqvM/BG");
assertTrue(matches);

Expected result is true, but it throws an exception:

java.lang.StringIndexOutOfBoundsException: begin 1, end 0, length 68
	at java.base/java.lang.String.checkBoundsBeginEnd(String.java:4601)
	at java.base/java.lang.String.substring(String.java:2704)
	at org.springframework.security.crypto.password.DelegatingPasswordEncoder.extractId(DelegatingPasswordEncoder.java:251)
	at org.springframework.security.crypto.password.DelegatingPasswordEncoder.matches(DelegatingPasswordEncoder.java:230)

The problem starts here:

spring-security/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java

Line 247 in 0a2a327

int end = prefixEncodedPassword.indexOf(this.idSuffix, start);

This code is fine before gh-10278, where prefix and suffix were fixed as {, }, but after gh-10278 the possibility that prefix and suffix might be the same should be considered.

[sjohnr]

@ghoonch4 thanks for your detailed reply!

I do understand that it is now technically possible to set the prefix/suffix to the same value, hence the bug. What I'm asking is more specifically is there a good reason to do so? The intention, as far as I can tell, is to use different values. For example "[" and "]". I'm not clear on why you would want to use "$" and "$", even though it is possible. If there's a good reason to do so (usually discussed in an issue prior to the PR being opened), we can go from there. I was wondering if you could provide the use case you're running into that requires this functionality to work.

[ghoonch4]

@sjohnr There is no use case for me. 😅
"$" and "$" was just an example. I just wanted to say that technically the same prefix/suffix can be used, but this doesn't work as expected. I thought this was obviously a bug, so I submitted a PR without discussion, and I think it was a hasty decision. I'm sorry. And I think the prefix/suffix allowed to be customized without any restrictions means that we don't all know what use cases there are, but we support it for any use case.

[sjohnr]

@ghoonch4 not a problem, that’s just the ideal flow.

And I think the prefix/suffix allowed to be customized without any restrictions means that we don't all know what use cases there are, but we support it for any use case.

But this isn’t 100% true though is it? Based on your reported issue, it will never actually work, so no one can successfully use this in the wild, right? They can certainly construct an instance successfully but it won’t work for them.

The reason I’m going down this path is that I’m worried there’s a hidden case here we could open up by allowing this. I really think the intended config is two different characters. When we allow two $ chars, our prefix can actually look like bcrypt without a prefix, eg. $2a$. Not that anyone would do that on purpose… But I’m not sure I like that it’s easily possible.

I feel like it would be better to start by preventing this case proactively with a check in the constructor. We can always change our mind and allow this later, but we wouldn’t be able to easily change our mind if we apply your current suggestion.

What do you think?

[ghoonch4]

@sjohnr Okay, I understand what you're worried about. I think instantiation should be prevented if prefix contains suffix. Would this be enough?

[sjohnr]

I think so, yes!

[ghoonch4]

@sjohnr Please review it again.

[sjohnr]

Thanks @ghoonch4!

See a few minor items below, then please squash all commits and rebase on main using your final commit message (you can omit the back-ticks, e.g. "Prevent instantiation of DelegatingPasswordEncoder if idPrefix contains idSuffix").

[heowc]

Oops.. I missed it. Thank you! 👍