NullPointerException thrown if principal or credentials are null

[philwebb]

Originally raised in spring-projects/spring-boot#17861

The org.springframework.ldap.core.support.AbstractContextSource class has a setupAuthenticatedEnvironment which is passed a principal and credentials. If either of these are null a NullPointerException is thrown because null values are not permitted in a Hashtable.

I think it might be better if DirContextAuthenticationStrategy implementations could guard against null values and not add entries in such cases.

[filiphr]

Thanks for raising the issue in spring-ldap @philwebb.

Just some extra info. There could be a guard when setting the user and password in the AbstractContextSource. Something like that is already done for setBase.

[rwinch]

Thanks for the report @philwebb

I don't think I agree with not adding null entries. If the credentials are null and anonymousReadOnly = false, then the user should get an error because it is misconfigured. Is there a reason you would expect it to ignore the credentials?

[philwebb]

@rwinch I'd have to defer to @filiphr on that.

My original understanding of the issue was that if the credentials are null you get an exception regardless of the anonymousReadOnly setting. I think Boot triggered the error because it called setAnonymousReadOnly(true), setUserDn(null) and setPassword(null). Those last two call changed the defaults from an empty string to null.

I guess a better exception message is needed regardless. Perhaps it's just a case of doing Assert.notNull in the setUserDn/setPassword methods.

[philwebb]

BTW, we fixed Boot in the meantime to not call the setters with null.

[filiphr]

Was looking at the master code of spring-ldap and I saw that this should not be an issue anymore if the user is set to null, since anonymousReadOnly is set to true when there is no userDn. Was actually fixed as part of #473. Which is the exact problem that I was seeing. And the best thing is that this was already reported by @philwebb due to a similar problem in Spring Boot.

However, if the user is set and the password is somehow set to null then the same NPE would occur again.

The logging message if the password has no text is a bit misleading as well

LOG.info("Property 'password' not set - " + "blank password will be used");

Which is not quite correct by looking at the code. Since the password would be send as is (so a null in this case).

[rwinch]

Thanks for the additional details @filiphr and @philwebb! A better error message would be useful.

Perhaps it's just a case of doing Assert.notNull in the setUserDn/setPassword methods.

I don't think this will help much since the values are null by default. This means if a user doesn't set them at all, the improved error message wouldn't be used.

@filiphr Would you be interested in sending a PR to clean up the error message and logging?

[filiphr]

@rwinch yes I can try to craft a PR, but what I don't get what the resolution is now?

If you ask me the fix should be in

spring-ldap/core/src/main/java/org/springframework/ldap/core/support/AbstractContextSource.java

Lines 680 to 689 in 15142d4

	class SimpleAuthenticationSource implements AuthenticationSource {
	public String getPrincipal() {
	return userDn;
	}
	public String getCredentials() {
	return password;
	}
	}

And return empty string if the userDn or password is null. That is what is being done in SpringSecurityAuthenticationSource.

btw the values are empty string by default, not null, that is why not setting null works :)

[jzheaux]

Hi, @filiphr. I think the fix is based on Rob's comment here:

If the credentials are null and anonymousReadOnly = false, then the user should get an error because it is misconfigured.

Given that, empty creds should not be inferred from null creds. Instead, the application should error to alert the user.

And return empty string if the userDn or password is null.

I can see your point here, though I believe there is a difference between the framework inferring an empty string and the context source setting a different value than what the application asked for. That is, calling setPassword(null) is more likely an indication of an application error, and we should alert instead of infer.