New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEC-2367: ProviderManager doesn't report InternalAuthenticationServiceExceptions #2589

Closed
spring-issuemaster opened this Issue Oct 17, 2013 · 8 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster

spring-issuemaster commented Oct 17, 2013

John Gibson (Migrated from SEC-2367) said:

If the ProviderManager has multiple AuthenticationProviders and one of the earlier providers throws an InternalAuthenticationServiceException then it will not be reported if a later provider throws any kind of AuthenticationException.

I experienced this when the errors that were occurring from a misconfigured LDAP provider were masked by the UsernameNotFoundException from the subsequent dao provider.

Given the javadocs for InternalAuthenticationServiceException it sounds like these errors should be logged even if a subsequent authentication provider succeeds (or fails). Perhaps ProviderManager.authenticate() should have a special case for handling IASEs like it does for AccountStatusExceptions.

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Dec 4, 2013

Rob Winch said:

ProviderManager now rethrows InternalAuthenticationServiceExceptions

spring-issuemaster commented Dec 4, 2013

Rob Winch said:

ProviderManager now rethrows InternalAuthenticationServiceExceptions

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Jun 29, 2014

zyro said:

i think the fix (i.e. re-throwing the exception) causes totally undesired behavior requiring a manual work-around e.g. in the following situation:

  • two authentication providers (ldap and db)
  • ldap is not reachable (java.net.UnknownHostException --> javax.naming.CommunicationException --> org.springframework.security.authentication.InternalAuthenticationServiceException)

before this change, the db authentication was attempted when ldap was down. now, no further authentication providers are polled when one throws a InternalAuthenticationServiceException.

looks like this is intentional when the exception is rethrown (see the AccountStatusException case).
the OPs primary concern was logging - perhaps the ProviderManager should just log instead of rethrow the exception?

my dirty current workaround is to catch the InternalAuthenticationServiceException in my LdapAuthenticationProvider and rethrow a AuthenticationServiceException...

thanks, zyro

spring-issuemaster commented Jun 29, 2014

zyro said:

i think the fix (i.e. re-throwing the exception) causes totally undesired behavior requiring a manual work-around e.g. in the following situation:

  • two authentication providers (ldap and db)
  • ldap is not reachable (java.net.UnknownHostException --> javax.naming.CommunicationException --> org.springframework.security.authentication.InternalAuthenticationServiceException)

before this change, the db authentication was attempted when ldap was down. now, no further authentication providers are polled when one throws a InternalAuthenticationServiceException.

looks like this is intentional when the exception is rethrown (see the AccountStatusException case).
the OPs primary concern was logging - perhaps the ProviderManager should just log instead of rethrow the exception?

my dirty current workaround is to catch the InternalAuthenticationServiceException in my LdapAuthenticationProvider and rethrow a AuthenticationServiceException...

thanks, zyro

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Sep 29, 2014

zyro said:

maybe this is slipping through cause its already closed... @rwinch: im a bit reluctant to raise a new one for the described behavior. a bit of feedback would be much appreciated. if a fresh issue helps tracking this, im happy to open it.

spring-issuemaster commented Sep 29, 2014

zyro said:

maybe this is slipping through cause its already closed... @rwinch: im a bit reluctant to raise a new one for the described behavior. a bit of feedback would be much appreciated. if a fresh issue helps tracking this, im happy to open it.

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Nov 20, 2014

zyro said:

one more bump here given it looks like 4.0 is closing in :) --> should i open up a new jira for the described issue?
thanks, zyro

spring-issuemaster commented Nov 20, 2014

zyro said:

one more bump here given it looks like 4.0 is closing in :) --> should i open up a new jira for the described issue?
thanks, zyro

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Nov 21, 2014

Rob Winch said:

Can you outline why you want to ignore the fact that the host is not found? This sounds like a configuration error or networking error that should be alerted to the operations team.

spring-issuemaster commented Nov 21, 2014

Rob Winch said:

Can you outline why you want to ignore the fact that the host is not found? This sounds like a configuration error or networking error that should be alerted to the operations team.

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Nov 25, 2014

zyro said:

i am currently facing a sceanrio where a web-app has some users authenticating against ldap and some users authenticating against the app db. if the ldap server is (temporarily) not available, that should not impact those users authenticating against the db.

spring-issuemaster commented Nov 25, 2014

zyro said:

i am currently facing a sceanrio where a web-app has some users authenticating against ldap and some users authenticating against the app db. if the ldap server is (temporarily) not available, that should not impact those users authenticating against the db.

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Nov 25, 2014

Rob Winch said:

The problem with silently ignoring an error like this is it makes the LDAP server's temporary state of being down to something much longer. It also masks a configuration error (i.e. if the wrong LDAP URL were used) which is not desirable.

If you need to totally ignore the error you can wrap the AuthenticationProvider in an implementation that ignores the exception, but this is not something that should really be in the framework.

spring-issuemaster commented Nov 25, 2014

Rob Winch said:

The problem with silently ignoring an error like this is it makes the LDAP server's temporary state of being down to something much longer. It also masks a configuration error (i.e. if the wrong LDAP URL were used) which is not desirable.

If you need to totally ignore the error you can wrap the AuthenticationProvider in an implementation that ignores the exception, but this is not something that should really be in the framework.

@spring-issuemaster

This comment has been minimized.

Show comment
Hide comment
@spring-issuemaster

spring-issuemaster Nov 25, 2014

zyro said:

in fact thats exactly what we are currently doing. will stick to it. thanks again for the lightning-fast feedback :)

spring-issuemaster commented Nov 25, 2014

zyro said:

in fact thats exactly what we are currently doing. will stick to it. thanks again for the lightning-fast feedback :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment