stonith404 / pingvin-share

A self-hosted file sharing platform that combines lightness and beauty, perfect for seamless and efficient file sharing.
https://stonith404.github.io/pingvin-share/
BSD 2-Clause "Simplified" License
2.82k stars 204 forks source link

πŸ› Bug Report: LDAP Auth not working for Active Directory #582

Open regiolis opened 2 weeks ago

regiolis commented 2 weeks ago

πŸ‘Ÿ Reproduction steps

Server URL: ldap://domain.local Bind DN: CN=bind,CN=Users,DC=domain,DC=local Bind password: password User base: OU=Accounts,DC=domain,DC=local User query: (sAMAccountName=%username%) Admin group: CN=APP_PINGVIN_ADMINS,CN=User,DC=domain,DC=local

πŸ‘ Expected behavior

AD login should works.

πŸ‘Ž Actual Behavior

It doesn't works when trying to login with samAccountName and password.

Error:Wrong email or password

πŸ“œ Logs

[Nest] 59 - 08/31/2024, 2:22:40 PM DEBUG [AuthService] Trying LDAP login for user regiolis

[Nest] 59 - 08/31/2024, 2:22:40 PM LOG [AuthService] Failed login attempt for user regiolis from IP 172.18.0.1

[Nest] 59 - 08/31/2024, 2:23:18 PM DEBUG [AuthService] Trying LDAP login for user regiolis

[Nest] 59 - 08/31/2024, 2:23:18 PM LOG [AuthService] Failed login attempt for user regiolis from IP 172.18.0.1

stonith404 commented 2 weeks ago

@WolverinDEV Do you know what could cause this issue?

WolverinDEV commented 2 weeks ago

Hey @regiolis, Have you tried running the user query manually with an LDAP explorer?
Running the query manually could help narrow down which of these might be happening. Let me know if you find anything!

From experience, silently failing LDAP authentication can typically result from one of the following scenarios:

  1. User not found by the query
    For example, a query like (sAMAccountName=regiolis) might not return any results.

  2. Incorrect password A user is found, but the password provided doesn't match.

  3. Username contains unsupported characters If the username contains characters outside of [a-zA-Z0-9]+ (source), authentication could fail. This check is in place to prevent injection attacks in the LDAP query, though it could be relaxed to allow characters like . and @. (I might submit a PR for this someday).

  4. Multiple user results Although rare, if your query yields multiple users, only the first one is authenticated against, which could lead to unexpected behaviour.

Edit:
PS: Actually there is one bug when it comes to LDAP and a login with an e-mail address.
As the authentication dto differentiates between an username and an email, login by an emails does not work. The LDAP authentication expects a username (source) and will fail if only an email is provided.
@stonith404 do I may ask, why you differentiate so explicitly between them?

stonith404 commented 2 weeks ago

Thanks, @WolverinDEV. That likely is the issue. I’m not sure I fully understand your questionβ€”could you clarify what you mean? Are you suggesting we eliminate the username attribute altogether?

WolverinDEV commented 2 weeks ago

I mean this.
Is there any particular reason you do not pass it as emailOrUsername to the server?

stonith404 commented 2 weeks ago

@WolverinDEV Not really, I just thought that it is cleaner to handle it in the frontend. But does it make a difference for LDAP if we handle this in the frontend because it would need a different query nevertheless or I'm wrong? I thought about something like this or would you recommend some other solution?

public async authenticateUser(usernameOrEmail: string, password: string): Promise<LdapAuthenticateResult | null> {
    const usernamePattern = /^[a-zA-Z0-9]+$/;
    const emailPattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;

    // Determine whether the input is a valid email or username to prevent injection 
    // (not sure if this is necessary)
    const isEmail = emailPattern.test(usernameOrEmail);
    const isUsername = usernamePattern.test(usernameOrEmail);

    if (!isEmail && !isUsername) {
        return null;
    }

    // Determine the appropriate search query based on input type
    const searchBase = this.serviceConfig.get("ldap.searchBase");
    let searchQuery: string;

    if (isEmail) {
        searchQuery = this.serviceConfig.get("ldap.searchQueryForEmail")
            .replaceAll("%email%", usernameOrEmail);
    } else {
        searchQuery = this.serviceConfig.get("ldap.searchQueryForUsername")
            .replaceAll("%username%", usernameOrEmail);
    }
    ...
}
regiolis commented 1 week ago

Hey @regiolis, Have you tried running the user query manually with an LDAP explorer?
Running the query manually could help narrow down which of these might be happening. Let me know if you find anything!

From experience, silently failing LDAP authentication can typically result from one of the following scenarios:

  1. User not found by the query
    For example, a query like (sAMAccountName=regiolis) might not return any results.

  2. Incorrect password A user is found, but the password provided doesn't match.

  3. Username contains unsupported characters If the username contains characters outside of [a-zA-Z0-9]+ (source), authentication could fail. This check is in place to prevent injection attacks in the LDAP query, though it could be relaxed to allow characters like . and @. (I might submit a PR for this someday).

  4. Multiple user results Although rare, if your query yields multiple users, only the first one is authenticated against, which could lead to unexpected behaviour.

Edit:
PS: Actually there is one bug when it comes to LDAP and a login with an e-mail address.
As the authentication dto differentiates between an username and an email, login by an emails does not work. The LDAP authentication expects a username (source) and will fail if only an email is provided.
@stonith404 do I may ask, why you differentiate so explicitly between them?

Hey,

Yeah but still doesn't works...

WolverinDEV commented 1 week ago

Have you tried your query manually and verified, that the user can be found with that query?

regiolis commented 1 week ago

Have you tried your query manually and verified, that the user can be found with that query?

Yeah but even with (sAMAccountName=regiolis) it still not working... I would like to add my others webapps using LDAP Auth are working well so it's not an AD issue...

WolverinDEV commented 5 days ago

By any chance, does any of the user entry properties contains any special characters (non ascii characters)?
E.g.: Àâü etc