jotaelesalinas / laravel-adminless-ldap-auth

Authenticate users in Laravel against an adminless LDAP server
MIT License
210 stars 33 forks source link

which placeholders for LDAP_USER_FORMAT ? #38

Closed sgohl closed 4 years ago

sgohl commented 4 years ago

Assume the following ActiveDirectory user:

sAMAccountName: jdoe
cn: John Doe
distinguishedName: CN=John Doe,OU=users,DC=acme,DC=corp

User Login should happen with sAMAccountName (=jdoe), but according to the documentation, as I understand the LDAP_USER_FORMAT must be built with distinguishedName (=John Doe). But there seems only %s available as placeholder, which equals the entered sAMAccountName

I have this .env :

LDAP_USER_ATTRIBUTE=sAMAccountName
LDAP_BASE_DN=OU=users,DC=acme,DC=corp
LDAP_USER_FORMAT=CN=%s,${LDAP_BASE_DN}

which of course results in the following logfile:

Operation: Failed - Username: CN=jdoe,OU=users,DC=acme,DC=corp - Reason: Invalid credentials

what actually should be

Username: CN=John Doe,OU=users,DC=acme,DC=corp

my config/ldap_auth.php:

        'ldap' => [
            'locate_users_by' => env('LDAP_USER_ATTRIBUTE', 'sAMAccountName'),
            'bind_users_by' => env('LDAP_USER_DN', 'distinguishedName'),
            'user_format' => env('LDAP_USER_FORMAT', ''),
        ],

is there any other placeholder than %s available for LDAP_USER_FORMAT ?

How to accomplish this logic: use cnas %s found by sAMAccountName

jotaelesalinas commented 4 years ago

Hi @port22 , sorry, I never used ActiveDirectory, only OpenLDAP.

I see a big problem here. I'll try to explain.

This is how this library works (well, this is actually how Laravel Auth works):

Now, in the login form, you have one field for the username and one for the password. That means that, as the developer, you have only one identifier to pass to these methods.

You cannot pass 'jdoe' to one function and 'John Doe' to another one.

In the login form, what are you going to ask your user to enter, 'jdoe' or 'John Doe'?

BTW, what happens if there are two John Does? What about the distinguished name?

sgohl commented 4 years ago

thanks for your answer! Yes, the problem is that the CN in distinguishedName is not equal to sAMAccountName, which I understand can not be worked around ?

what are you going to ask your user to enter, 'jdoe' or 'John Doe'?

That's what I meant with when I explained:

User Login should happen with sAMAccountName (=jdoe)

either way, CN is also unique, so no other John Doe can exist.

My current workaround is to let users login with their CN, but that differs from all other tools, and you know how users are :-)

jotaelesalinas commented 4 years ago

Hi! I made some changes. You can see them at https://github.com/jotaelesalinas/laravel-simple-ldap-auth/commit/c6032308b43abdcc9d067a7599795ff72dc373b7.

I made two different LDAP_USER_ATTRIBUTE_* env variables, one for the search and one for the binding.

Can you try?

If it doesn't work, can you please try forking the repo, making your changes until it works (I can try to help you there) and do a pull request?

jotaelesalinas commented 4 years ago

Hi @port22 , any news on this issue? Were you able to solve it?

sgohl commented 4 years ago

ok, I did a deep dive into it, using with composer.json

        "jotaelesalinas/laravel-simple-ldap-auth": "dev-master",

the login page now says:

These credentials do not match our records.

tailing the laravel.log it seems that the search is correct (correct SAMAccountName value) then the Binding Operation with the bindDN-auth-user is also successful. then it stops. no error complaining whatsoever. I assumed to find an error binding with some wrong identifier or so

To be clear, what exactly will be injected into the %s placeholder? Is it what just typed into the username html form input field, or is it the result of a search containing the value of cn of the user?

my .env

LDAP_BASE_DN=OU=user,OU=somegroup,DC=acme,DC=org
LDAP_USER_SEARCH_ATTRIBUTE=sAMAccountName
LDAP_USER_BIND_ATTRIBUTE=cn
LDAP_USER_FULL_DN_FMT=${LDAP_USER_ATTRIBUTE_BIND}=%s,${LDAP_BASE_DN}

Can I set the 'logging' => env('LDAP_LOGGING', true) to something more verbose?

sgohl commented 4 years ago

Is it actually possible to have a search filter ?

LDAP_BASE_DN is containing every user exists. And it is used to bind the user, so it's the DN where the user is actually located and therefore can not be modified.

But the application should be only allowed to users which are memberOf a specific directory path.

somethin like this?

LDAP_SEARCH_FILTER="(&(sAMAccountName=%s)(memberOf=CN=somegroup,OU=groups,DC=acme,DC=org))"
jotaelesalinas commented 4 years ago

Hi @port22 ! With the configuration that you pasted, if your user has the following attributes: SAMAccountName = "jdoe" and cn = "John Doe", the package will first search a user with SAMAccountName = "jdoe" and then check the password of the user with full distinguished name = "cn=John Doe,OU=user,OU=somegroup,DC=acme,DC=org".

To debug it (I would appreciate it if you contributed the fix back), follow these steps:

Check that everything is as it should be.

Add you own logging messages where appropriate https://laravel.com/docs/7.x/logging.

Sorry but I don't have access to any ActiveDirectory server. Hope this helps.

sgohl commented 4 years ago

I came a bit closer ... after spitting var_dumps like an idiot :-D

The var_dump from (1) shows perfectly the correct ldap result.

But the following var_dump($userdata) shows null in the username array field:

    public function retrieveByCredentials(array $credentials) : ?Authenticatable
    {
        // despite the name, Laravel docs clearly specify that this method should not
        // check if the password is ok, only retrieve a user by identifier
        $username = $credentials[LdapUser::keyName()];

        $userdata = $this->ldap_helper->retrieveUser($username);
        if (!$userdata) {
            return null;
        }

        var_dump($userdata);
        exit;
        return new LdapUser($userdata);
    }

likely connected to the config array:

    'sync_attributes' => [
        'email' => 'mail',
        'username' => 'sAMAccountName',
    ],

Funny thing: If I change it to

        'username' => 'cn',

var_dump($userdata) will show my CN as value for username. I suppose those keys have to be kind of registered anyhow before I can use them? Tested it with some additional keys like phone etc. nothing worked, only cn does.

I don't know if this has anything to do with the database setup, but the mysql table users is alwqays empty.

mysql> select * from users;
Empty set (0.00 sec)

mysql> describe users;
+-------------------+---------------------+------+-----+---------+----------------+
| Field             | Type                | Null | Key | Default | Extra          |
+-------------------+---------------------+------+-----+---------+----------------+
| id                | bigint(20) unsigned | NO   | PRI | NULL    | auto_increment |
| name              | varchar(255)        | NO   |     | NULL    |                |
| email             | varchar(255)        | NO   | UNI | NULL    |                |
| email_verified_at | timestamp           | YES  |     | NULL    |                |
| password          | varchar(255)        | NO   |     | NULL    |                |
| remember_token    | varchar(100)        | YES  |     | NULL    |                |
| created_at        | timestamp           | YES  |     | NULL    |                |
| updated_at        | timestamp           | YES  |     | NULL    |                |
+-------------------+---------------------+------+-----+---------+----------------+
8 rows in set (0.00 sec)
sgohl commented 4 years ago

oh god. samaccountname is case-sensitive and must be written lowercase in the config. Also in .env

LDAP_USER_SEARCH_ATTRIBUTE=samaccountname

Then, to make mysql-table match the config, I changed the tablename name to username.

Now it works. Most basically that was the reason. So, yeah, I can happily confirm that your master branch works :-)

And thanks a hell of a lot for your help!! :1st_place_medal:

jotaelesalinas commented 4 years ago

I’m glad it worked in the end. I will add a note about the case sensitivity in the Readme.