[Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable #44898

Jayfrown · 2022-01-03T16:37:28Z

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #44089
License	MIT

Return a 500 ISE if the LDAP server is unreachable.

I have tested the following scenarios:

LDAP server unreachable

Adapter/ExtLdap/Connection.php throws a ConnectionException which 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 LdapUserProvider tries to search the LDAP directory for the provided user. To search, it tries to authenticate using the (invalid) bind credentials, resulting in Adapter/ExtLdap/Connection.php throwing an InvalidCredentialsException which 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 InvalidCredentialsException inside the LdapUserProvider and 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 LdapUserProvider searches the LDAP directory, finds no entries, and throws a UserNotFoundException which 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.php throws an InvalidCredentialsException. The ldap/Security/CheckLdapCredentialsListener.php now catches the InvalidCredentialsException (instead of the ConnectionException) and throws a BadCredentialsException such 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

Jayfrown · 2022-01-03T16:37:52Z

src/Symfony/Component/Ldap/Security/LdapUserProvider.php

-
-            throw $e;
-        }
+        $this->ldap->bind($this->searchDn, $this->searchPassword);


We could opt to catch InvalidCredentialsException 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.

Same in CheckLdapCredentialsListener.php line 77

Point being that throwing the InvalidCredentialsException can be ambiguous in either meaning "user-provided credentials are invalid" or "configured search credentials are invalid"

7ab0611 introduces an InvalidSearchCredentialsException which is thrown when binding fails when using the configured search credentials.

Jayfrown · 2022-01-03T16:54:26Z

@wouterj FWIW, I have the following docker-compose setup for a local LDAP:

docker-compose.yml:

services:

  openldap:
    image: osixia/openldap:1.5.0
    restart: unless-stopped
    environment:
      LDAP_LOG_LEVEL: "256"
      LDAP_ORGANISATION: ${LDAP_ORGANISATION:-Example Inc.}
      LDAP_DOMAIN: ${LDAP_DOMAIN:-example.org}
      LDAP_BASE_DN: ${LDAP_BASE_DN:-"dc=example,dc=org"}
      LDAP_ADMIN_PASSWORD: ${LDAP_ADMIN_PASSWORD:-admin}
      LDAP_CONFIG_PASSWORD: ${LDAP_CONFIG_PASSWORD:-config}
      LDAP_RFC2307BIS_SCHEMA: "false"
      LDAP_BACKEND: "mdb"
      LDAP_TLS: "true"
      LDAP_TLS_CRT_FILENAME: "ldap.crt"
      LDAP_TLS_KEY_FILENAME: "ldap.key"
      LDAP_TLS_DH_PARAM_FILENAME: "dhparam.pem"
      LDAP_TLS_CA_CRT_FILENAME: "ca.crt"
      LDAP_TLS_ENFORCE: "false"
      LDAP_TLS_CIPHER_SUITE: "SECURE256:-VERS-SSL3.0"
      LDAP_TLS_VERIFY_CLIENT: "demand"
      LDAP_REPLICATION: "false"
      KEEP_EXISTING_CONFIG: "false"
      LDAP_REMOVE_CONFIG_AFTER_SETUP: "true"
      LDAP_SSL_HELPER_PREFIX: "ldap"
    tty: true
    stdin_open: true
    volumes:
      - ldap_data:/var/lib/ldap
      - ldap_config:/etc/ldap/slapd.d
      - ldap_certs:/container/service/slapd/assets/certs/
    ports:
      - "389:389"
      - "636:636"

  phpldapadmin:
    image: osixia/phpldapadmin:latest
    environment:
      PHPLDAPADMIN_LDAP_HOSTS: "openldap"
      PHPLDAPADMIN_HTTPS: "false"
    ports:
      - "8080:80"
    depends_on:
      - openldap

volumes:
  ldap_data:
  ldap_config:
  ldap_certs:

And the following configuration in Symfony:

services.yaml:

    Symfony\Component\Ldap\Ldap:
        arguments: ['@Symfony\Component\Ldap\Adapter\ExtLdap\Adapter']
        tags: ['ldap']
    Symfony\Component\Ldap\Adapter\ExtLdap\Adapter:
        arguments:
            - host: '%env(resolve:LDAP_HOST)%'
              port: '%env(resolve:LDAP_PORT)%'
              encryption: '%env(resolve:LDAP_ENCRYPTION)%'
              options:
                  protocol_version: 3
                  referrals: false

security.yaml:

    providers:
        app_ldap:
            ldap:
                service: Symfony\Component\Ldap\Ldap
                base_dn: '%env(resolve:LDAP_BASE_DN)%'
                search_dn: '%env(resolve:LDAP_SEARCH_DN)%'
                search_password: '%env(resolve:LDAP_SEARCH_PASSWORD)%'
                default_roles: ROLE_USER
                uid_key: uid
                
    firewalls:
        main:
            provider: app_ldap

.env:

LDAP_HOST="openldap"
LDAP_PORT="389"
LDAP_ENCRYPTION="none"
LDAP_SEARCH_DN="cn=admin,dc=example,dc=org"
LDAP_SEARCH_PASSWORD="admin"
LDAP_BASE_DN="ou=Users,dc=example,dc=org"

You'd have to create the rest of the schema by hand, if needed I can try to provide an LDIF.

carsonbot · 2022-01-04T12:39:04Z

Hey!

I think @karlshea has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Jayfrown · 2022-03-22T18:35:30Z

@wouterj if there's anything I can do to make reviewing easier, let me know

src/Symfony/Component/Ldap/CHANGELOG.md

stof · 2022-03-22T18:44:31Z

CheckLdapCredentialsListener has 2 modes. Your PR description lets me think that you tested only one of them.

Jayfrown · 2022-03-22T19:09:49Z

CheckLdapCredentialsListener has 2 modes. Your PR description lets me think that you tested only one of them.

@stof Thanks. It is very possible I might have missed something, as I'm not familiar with the two modes you mention. Could you expand on this or send some documentation my way?

stof · 2022-03-22T19:13:42Z

I'm not familiar with the ldap part either. But for those 2 modes, see the if/else inside the try/catch you changed

Jayfrown · 2022-03-22T21:05:46Z

@stof Thanks. If I understand correctly the difference is when query_string is set, which, according to the documentation:

makes the user provider search for a user and then use the found DN for the bind process

Basically allowing the LDAP component to search for a given user in multiple OUs instead of simply interpolating the username into the dn_string.

Expand default behavior

Interpolate the user-provided username in the dn_string, eg. uid={username},dc=example,dc=com becomes uid=jayfrown,dc=example,dc=com
Use the user-provided password to bind as the interpolated dn_string
- If the bind is successful, the credentials are accepted and log-in continues
- If the bind is not successful, the credentials are rejected and log-in is prohibited

Expand query_string behavior

Interpolate the user-provided username in the query_string, eg. (&(uid={username})) becomes (&(uid=jayfrown))
Bind with the configured search credentials
Perform an LDAP search inside the dn_string using the query_string, eg. if dn_string is dc=example,dc=com, this can return uid=jayfrown,dc=companyA,dc=example,dc=com or uid=jayfrown,dc=companyB,dc=example,dc=com, etc.
- If the search does not yield exactly one result, throw a BadCredentialsException indicating that the user-provided username is invalid
- If the search yields exactly one result, use the user-provided password to bind as the found DN
  - If the bind is successful, the credentials are accepted and log-in continues
  - If the bind is not successful, the credentials are rejected and log-in is prohibited

Having said all that, my changes should not impact either scenario apart from what I've already described. However, when I have some more time I will set up a configuration using query_string and test with that as well.

[`Jayfrown/ldap dev-ldap-server-unavailable`](https://github.com/Jayfrown/ldap/tree/ldap-server-unavailable) is `symfony/ldap 6.1.*` with symfony/symfony#44898 applied.

Jayfrown · 2022-03-23T02:12:57Z

I made an example application to make it easier to test and verify behavior. The repository includes a docker-compose configuration that spins up an OpenLDAP container, seeds it with some users, and exposes the ports such that the local application can communicate with it.

There are a couple of branches to test all permutations (with/without query_string as well as with/without this PR applied)

The master branch uses the normal configuration, without this PR applied
The query_string branch uses the query_string configuration without this PR applied
The 44898 branch uses the normal configuration, with this PR applied
The 44898_query_string branch uses the query_string configuration, with this PR applied

Branches with this PR applied require Jayfrown/ldap dev-ldap-server-unavailable which is a clone of the symfony/ldap repository which includes the work in this PR.

Note that after switching to/from branches with this PR applied it is necessary to run composer install.

@stof I've tested the same scenarios as mentioned in the OP with the query_string configuration and the behavior remains the same.

…LDAP server is unavailable

fabpot · 2022-03-29T06:21:46Z

Thank you @Jayfrown.

Jayfrown requested review from chalasr and wouterj as code owners January 3, 2022 16:37

carsonbot added Status: Needs Review Feature labels Jan 3, 2022

carsonbot added this to the 6.1 milestone Jan 3, 2022

Jayfrown commented Jan 3, 2022

View reviewed changes

Jayfrown mentioned this pull request Jan 3, 2022

LDAP authentication should return a meaningful error when the LDAP server is unavailable #44896

Closed

Nyholm added the Ldap label Jan 4, 2022

carsonbot changed the title ~~LDAP authentication should return a meaningful error when the LDAP server is unavailable~~ [Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable Jan 4, 2022

stof reviewed Mar 22, 2022

View reviewed changes

src/Symfony/Component/Ldap/CHANGELOG.md Outdated Show resolved Hide resolved

Jayfrown force-pushed the ldap-server-unavailable branch from ec7a0be to 58716bb Compare March 22, 2022 19:07

Jayfrown force-pushed the ldap-server-unavailable branch from 58716bb to 8452c7a Compare March 22, 2022 21:07

Jayfrown added a commit to Jayfrown/ldap that referenced this pull request Mar 23, 2022

Apply symfony/symfony#44898

57a9216

Jayfrown force-pushed the ldap-server-unavailable branch from 65aaf0c to 7ab0611 Compare March 23, 2022 02:31

nicolas-grekas approved these changes Mar 28, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 28, 2022

fabpot approved these changes Mar 29, 2022

View reviewed changes

[Ldap] LDAP authentication should return a meaningful error when the …

ad2cc0e

…LDAP server is unavailable

fabpot force-pushed the ldap-server-unavailable branch from 7ab0611 to ad2cc0e Compare March 29, 2022 06:21

fabpot mentioned this pull request Mar 29, 2022

[Ldap] LDAP authentication should return a meaningful error when the LD… symfony/symfony-docs#16665

Closed

fabpot merged commit a4592e1 into symfony:6.1 Mar 29, 2022

fabpot mentioned this pull request Apr 15, 2022

Release v6.1.0-BETA1 #46049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable #44898

[Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable #44898

Uh oh!

Jayfrown commented Jan 3, 2022

Uh oh!

Jayfrown Jan 3, 2022 •

edited

Loading

Uh oh!

Jayfrown Mar 22, 2022

Uh oh!

Jayfrown Mar 23, 2022

Uh oh!

Jayfrown commented Jan 3, 2022 •

edited

Loading

Uh oh!

carsonbot commented Jan 4, 2022

Uh oh!

Jayfrown commented Mar 22, 2022

Uh oh!

Uh oh!

stof commented Mar 22, 2022

Uh oh!

Jayfrown commented Mar 22, 2022

Uh oh!

stof commented Mar 22, 2022

Uh oh!

Jayfrown commented Mar 22, 2022

Uh oh!

Jayfrown commented Mar 23, 2022

Uh oh!

fabpot commented Mar 29, 2022

Uh oh!

Uh oh!

Uh oh!

[Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable #44898

[Ldap] LDAP authentication should return a meaningful error when the LDAP server is unavailable #44898

Uh oh!

Conversation

Jayfrown commented Jan 3, 2022

Uh oh!

Jayfrown Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jayfrown Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

Jayfrown Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

Jayfrown commented Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented Jan 4, 2022

Uh oh!

Jayfrown commented Mar 22, 2022

Uh oh!

Uh oh!

stof commented Mar 22, 2022

Uh oh!

Jayfrown commented Mar 22, 2022

Uh oh!

stof commented Mar 22, 2022

Uh oh!

Jayfrown commented Mar 22, 2022

Uh oh!

Jayfrown commented Mar 23, 2022

Uh oh!

fabpot commented Mar 29, 2022

Uh oh!

Uh oh!

Jayfrown Jan 3, 2022 •

edited

Loading

Jayfrown commented Jan 3, 2022 •

edited

Loading