kimai / kimai

Kimai is a web-based multi-user time-tracking application. Works great for everyone: freelancers, companies, organizations - everyone can track their times, generate reports, create invoices and do so much more. SaaS version available at https://www.kimai.cloud
https://www.kimai.org
GNU Affero General Public License v3.0
3.01k stars 529 forks source link

LDAP authentication saves password hash to database #4755

Closed paul1278 closed 2 months ago

paul1278 commented 2 months ago

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 }

https://github.com/kimai/kimai/discussions/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

kevinpapst commented 2 months ago

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 commented 2 months ago

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

kevinpapst commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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).

paul1278 commented 2 months ago

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 commented 2 months ago

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