Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
LDAP authentication should return a meaningful error when the LDAP server is unavailable#44896
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- No longer catch ConnectionException so that it will result in a 500 ISE- Catch the InvalidCredentialsException so we can still throw a BadCredentialsException if the provided credentials are known to be invalidref:symfony#44089
- No longer catch ConnectionException so that it will result in a 500 ISE- Expect a ConnectionException instead of a UserNotFoundException if the LDAP server is unreachableref:symfony#44089
carsonbot commentedJan 3, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| throw$e; | ||
| } | ||
| $this->ldap->bind($this->searchDn,$this->searchPassword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could opt to catchInvalidCredentialsException here in order to throw something more-specific that would indicate it is the search bind credentials (used during enumeration) that are invalid, as opposed to the credentials that the user has provided.
wouterj commentedJan 3, 2022
Thanks for your investigation on this issue and potential fix,@Jayfrown! I'll try to set-up a test LDAP docker container to experiment with the changes in this PR (probably on Wednesday). For now, to answer your question:
Generally, we want to be very stable with released versions. Expected wrong behavior is often better than unexpected errors. So when in doubt, we tend to lean on the safe side and call "distinguishing connection errors from invalid password errors" a feature (thus 6.1). Generally, you should be able to use |
Uh oh!
There was an error while loading.Please reload this page.
Return a 500 ISE if the LDAP server is unreachable.
Not sure if this is seen as a "bug fix" or a "new feature", or neither. It's not a bug per se, but it feels like wrong behavior to state that credentials are invalid if the LDAP server is unreachable.
I have tested the following scenarios:
LDAP server unreachable
Adapter/ExtLdap/Connection.phpthrows aConnectionExceptionwhich is not caught, and this results in a 500 ISE shown to the user. In development mode, you see a relevant trace and the correct error hinting to an unreachable LDAP server, and in production mode this wouldn't be shown to the user, and would be logged.LDAP server reachable, with invalid search bind credentials
The
LdapUserProvidertries to search the LDAP directory for the provided user. To search, it tries to authenticate using the (invalid) bind credentials, resulting inAdapter/ExtLdap/Connection.phpthrowing anInvalidCredentialsExceptionwhich is not caught. As this is indicative of a misconfiguration within the application (ie. the app's search bind credentials are wrong, and searching for any user will always fail) I believe a 500 ISE is appropriate.We could opt to catch the
InvalidCredentialsExceptioninside theLdapUserProviderand throw something more specific, to indicate it is the applications search bind credentials that are invalid, as opposed to the credentials the user provided during authentication.LDAP server reachable, with valid search bind credentials, authenticating as an unknown user
As the search bind credentials are valid, the
LdapUserProvidersearches the LDAP directory, finds no entries, and throws aUserNotFoundExceptionwhich results in the authenticator returning{"code":401,"message":"Invalid credentials."}. I believe this is good / expected behavior.LDAP server reachable, with valid search bind credentials, using known user with invalid password
The
ldap/Adapter/ExtLdap/Connection.phpthrows anInvalidCredentialsException. Theldap/Security/CheckLdapCredentialsListener.phpnow catches theInvalidCredentialsException(instead of theConnectionException) and throws aBadCredentialsExceptionsuch that the authenticator returns{"code":401,"message":"Invalid credentials."}LDAP server reachable, with valid search bind credentials, using known user with valid password
The credentials are validated and the user is logged in, everything works as expected