Skip to content

fix(auth): use generic wording for failed login logs/events - #3671

Open
IshanA2007 wants to merge 1 commit into
DefGuard:stable/2.xfrom
IshanA2007:fix/issue-712-invalid-password-log
Open

IshanA2007 wants to merge 1 commit into
DefGuard:stable/2.xfrom
IshanA2007:fix/issue-712-invalid-password-log

Conversation

@IshanA2007

Copy link
Copy Markdown

📝 New contributors

  • I have read, understand, and agree to the Contributor Agreement. By checking this box, I confirm I have the right to contribute this work and I grant Defguard sp. z o.o. the necessary rights to use my contribution as outlined in the full agreement.: https://github.com/DefGuard/defguard/blob/stable/2.x/CLA.md

📖 Description

Addresses the first checklist item of #712 (backend logging). Not using a closing
keyword, since the second item (red border on the username field in the web UI)
is not part of this PR.

A failed local password check logged, and stored in the activity log as a
UserLoginFailed event, the underlying argon2 error, which renders literally as
invalid password. Per the issue, both places that emit this now use the fixed,
generic wording invalid username or password:

  • crates/defguard_core/src/handlers/auth.rs: the "password check failed, LDAP not
    enabled" branch and the "password check failed, LDAP fallback also failed" branch.

Beyond readability, this also removes a small username-enumeration signal: that
message is only ever emitted for a username/email that was found in the database,
so an admin reading the logs could previously tell a valid username with a wrong
password apart from one that does not exist. The HTTP response was already generic
(401, no body), and is unchanged.

Two things worth flagging:

  • The LDAP-fallback event no longer carries the underlying LDAP error. The
    warn! on that path still logs LDAP error: {ldap_err}, so operators can still
    diagnose LDAP problems from the server log. Happy to keep it in the event too if
    you would rather not lose it there.
  • This does not make failed logins fully indistinguishable: no UserLoginFailed
    event is emitted at all when the username does not exist (there is no user to
    build ApiRequestContext from), so the presence of the event is still a signal.
    That is pre-existing and structural; let me know if you want it addressed
    separately.

Updated the 3 existing tests that hard-coded the old message string. No new test was
added because those tests already exercise exactly this path; I verified they bind
the change by reverting auth.rs and confirming all 3 fail.

🛠️ Dev Branch Merge Checklist

Documentation

  • Not applicable: no environment or deployment changes.
  • I have commented on my code, particularly in hard-to-understand areas (added a
    short note explaining why the message must stay generic).

Testing

  • I have prepared end-to-end tests for all new functionalities: not applicable,
    no new functionality.
  • I have performed end-to-end tests manually: not performed; verification was
    via the integration suite.
  • New and existing unit tests pass locally with my changes
    (cargo test -p defguard_core --test integration -- test_authenticate test_login_bruteforce test_change_self_password: 3 passed). cargo +nightly fmt --all --check and cargo clippy --all-targets --all-features -- -D warnings
    are clean across the workspace.

Deployment

  • Not applicable.

Prepared with AI assistance (Claude Code).

Local password verification failures previously logged and stored an
activity-log event containing the literal reason 'invalid password'.
Since this message is only ever emitted for a username that was
actually found in the database, it let an attacker distinguish a
valid username with a wrong password from a username that does not
exist at all (username enumeration), even though the HTTP response
itself was already generic ('Authentication error').

Use a fixed, generic 'invalid username or password' message instead
of interpolating the underlying error for both the plain
username/password failure path and the LDAP-fallback failure path.

Part of DefGuard#712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant