Skip to content
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

LDAP authentication saves password hash to database #4755

Closed
3 tasks done
paul1278 opened this issue Apr 8, 2024 · 8 comments
Closed
3 tasks done

LDAP authentication saves password hash to database #4755

paul1278 opened this issue Apr 8, 2024 · 8 comments
Labels
Milestone

Comments

@paul1278
Copy link

paul1278 commented Apr 8, 2024

Describe the issue

The documentation states, that kimai does not save the password when using LDAP authentication. After a look into the database I noticed, that it does save the password hash to the database when logging in using LDAP.

When I clear the hashes and simply login using my LDAP-account, it saves the hash again. I also checked the hash with my password, it is indeed derived from my password. Also if I delete my LDAP-settings, login with the password hash is possible.

I don't know if there is something wrong with my setup or a database migration not working, the ldap user has the following attributes on kimai2_users-table:

[kimai]> select * from kimai2_users;
+----+----------+------------------------+--------------------------------------------------------------+-------+---------+---------------------+-------+--------+------------------------------------+---------------------+--------------------+-----------------------+-----------+------+-------+---------+------------------------------------------------------+--------------+----------------+---------------+
| id | username | email                  | password                                                     | alias | enabled | registration_date   | title | avatar | roles                              | last_login          | confirmation_token | password_requested_at | api_token | auth | color | account | totp_secret                                          | totp_enabled | system_account | supervisor_id |
+----+----------+------------------------+--------------------------------------------------------------+-------+---------+---------------------+-------+--------+------------------------------------+---------------------+--------------------+-----------------------+-----------+------+-------+---------+------------------------------------------------------+--------------+----------------+---------------+
|  5 | REDACTED | REDACTED               | REDACTED                                                     | -     |       1 | 2023-04-REDACTED    | NULL  | NULL   | a:1:{i:0;s:16:"ROLE_SUPER_ADMIN";} | 2024-04-REDACTED    | NULL               | NULL                  | NULL      | ldap | NULL  | NULL    | REDACTED                                             |            0 |              0 |          NULL |

LDAP config local.yaml:

kimai:
    ldap:
        activate: true
        connection:
            ....
        user:
            baseDn: ou=...
            attributes:
                - { ldap_attr: "usernameAttribute", user_method: setUsername }
                - { ldap_attr: "mail", user_method: setEmail }
                - { ldap_attr: "cn", user_method: setAlias }

#4182 someone mentions, that there are no hashes on the database. So it must be a quite new bug.

I already tried

Kimai version

2.14.0

How do you run Kimai?

Docker

Which PHP version are you using?

8.2

Logfile

No response

Screenshots

No response

@paul1278 paul1278 added the bug label Apr 8, 2024
@kevinpapst
Copy link
Member

kevinpapst commented Apr 8, 2024

If you configured a mapping for the field plainTextPassword that might be true.
Otherwise you found code that I am not aware of.

That's the only place where LDAP attributes are hydrated:
https://github.com/kimai/kimai/blob/main/src/Ldap/LdapManager.php#L171

@paul1278
Copy link
Author

paul1278 commented Apr 8, 2024

I am currently checking on this, it seems to happen somewhere after this line is executed.

@kevinpapst
Copy link
Member

I don't have an LDAP on hand, so I can't debug end-to-end right now.

Search for the methods setPlainPassword or setPassword on src/Entity/User.php

@paul1278
Copy link
Author

paul1278 commented Apr 8, 2024

I made a small setup with xdebug and followed the login request. I'm not that good with symfony, but I hope this can help to trace the bug:

The onCheckPassport-function inside LdapCredentialsSubscriber.php receives a passport with several badges. This passport has the Symfony\Component\Security\Http\Authenticator\Passport\Badge\PasswordUpgradeBadge set including the plaintextPassword of the user.

After successful authentication, the AuthenticatorManager of symfony triggers all login-handlers using the LoginSuccessEvent. There is one listener waiting named Symfony\Component\Security\Http\EventListener\PasswordMigratingListener, it checks for the PasswordUpgradeBadge, which triggers a hashing-process of the given password. It then triggers upgradePassword inside src/Security/KimaiUserProvider.php, resulting in the upgradePassword-function of src/Repository/UserRepository.php.

After this call, the newly hashed password will be saved to the database.

I don't know, where this badge comes from except some deep code inside symfony, but I hope this helps!

@paul1278
Copy link
Author

paul1278 commented Apr 8, 2024

It seems to be fixed when I add a small check inside the UserRepository:

    public function upgradePassword(PasswordAuthenticatedUserInterface $user, string $newHashedPassword): void
    {
        if (!($user instanceof User)) {
            return;
        }
        if($user->getAuth() != User::AUTH_INTERNAL) {
            return;
        }
        try {
            $user->setPassword($newHashedPassword);
            $this->saveUser($user);
        } catch (\Exception $ex) {
            // happens during login: if it fails, ignore it!
        }
    }

@kevinpapst
Copy link
Member

Wow, how super weird.
Thank you for that excellent input, I will add a fix for the next release.

Probably resetting the password on every login, what do you think?
To empty it for everyone who logged-in via LDAP (likely SAML as well if it behaves the same there, will have to check).

@kevinpapst kevinpapst added this to the 2.15.0 milestone Apr 8, 2024
@paul1278
Copy link
Author

paul1278 commented Apr 8, 2024

No problem, sounds great!

This would work and also removes the possibility for this to happen somewhere else, but it also reduces the chance of noticing bugs around this.

Maybe with a db-migration as a one-time-fix you could also clean current LDAP-users?

@kevinpapst kevinpapst mentioned this issue Apr 11, 2024
6 tasks
@kevinpapst
Copy link
Member

Next release will stop saving the hashed password.
Thanks for your support and input 👍

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

No branches or pull requests

2 participants