[pacmano1]

Fixes #240.

A failed login gave away whether the username was real, in two separate ways. This fixes both, and adds a server event so an admin can still see when an account locks.

Release note

Failed logins no longer reveal whether a username exists. A wrong password, an unknown username and a locked account now all get the same response, FAIL with the message Login failed., and take about the same time. The response no longer reports how many attempts are left or how long a lock has to run, and the built-in login no longer returns the FAIL_LOCKED_OUT status, so anything that checks for that status or for the old Incorrect username or password. text needs updating. When an account locks, the server logs a User account locked event at Warning level with the username and the time the lock ends.

The messages

The lockout paths in DefaultUserController.authorizeUser named the account and reported how long the lock had left to run, and a wrong password for a real account reported the attempts left before the lock. An unauthenticated caller could tell a valid username from an invalid one. A wrong password, an unknown username and a locked account now all return FAIL and Login failed.

Per @tonygermano, the message doesn't say "incorrect username or password", because for a locked account given the right password that isn't true. There is no property to bring the detailed messages back.

The lockout event

Per @tonygermano, the failed login that locks an account dispatches a User account locked server event, level WARNING, outcome FAILURE, with the username and the time it unlocks (locked until). Attempts against a locked account are rejected before a strike is counted, so there is one event per lock, plus one for each attempt that was already in flight when it locked.

It is dispatched from DefaultUserController, so it covers HTTP basic auth on the API too, which goes through MirthServlet and logs no Login event at all. It carries no IP address, since the controller doesn't have one; the Login event logged for the same /users/_login request has it.

On a running engine with retrylimit = 30, the 31st wrong password logged one User account locked event with username and a locked until an hour later. The 32 attempts after it against the locked account, the right password included, logged no more. Basic auth on GET /api/users/current did the same, and there the lock event was the only event the 33 failed requests left.

The timing

Fixing the messages alone left a second and much larger signal in place. Closing #240 on the message fix by itself would have overstated what changed.

The password digest is only reached for a valid, unlocked user. An unknown username never enters the validUser != null block, and a locked account returns before it, so both skipped 600,000 PBKDF2 iterations. Measured with generic messages already enabled, 250 samples per cohort:

cohort	median	p10	p90
valid + unlocked	230.90 ms	227.52	234.27
valid + locked	0.49 ms	0.42	0.72
invalid username	0.76 ms	0.66	0.93

The distributions do not overlap. The fastest unlocked sample was still 240x slower than the slowest locked one, so one timed request classified any username whatever the response body said. An attacker could enumerate accounts without reading a message at all.

A same-account control confirms the cause. An account that crossed the strike limit partway through a run stepped from 232.03 ms to 0.33 ms at the boundary, with no samples in between. The account did not change, only its lockout state did.

Every login now does the same work whatever the outcome: one user lookup, one credentials query (an unknown username looks up an id no user has), and one full digest, against the stored hash or, when there isn't a usable one, a throwaway hash. That covers an unknown username, a locked account, an account that has never had a password set, and a pre-2.2 hash, which keeps its own cheap check alongside the digest. The outcome is decided once at the end. The throwaway hash is generated once from the live digester, so it follows the configured algorithm and iteration count. Per @mgaffigan, this replaces a helper that ran a throwaway digest on each of those paths separately.

Measured on this commit on a slower machine, where one digest costs about 1.25 s, 30 interleaved samples per group:

group	median	p10	p90
invalid username	1248.16 ms	992.05	1339.21
valid + unlocked	1277.08 ms	1154.99	1378.11
valid + locked	1257.25 ms	901.05	1337.59
valid, no password set	1256.33 ms	1172.63	1335.02

No pair of groups can be told apart: a Mann-Whitney test gives p above 0.17 for every pair.

This is not constant time and I am not claiming it is. The difference left is the strikes. A wrong password for a real, unlocked account calls incrementStrikes(), a database update and a read, and resetExpiredStrikes() does the same when old strikes have expired. An unknown username does neither, and closing that would mean keeping strike rows for usernames that don't exist. The strike write didn't separate the groups above; the expired-strike reset happens once per expired lock and wasn't measured.

There is a tradeoff here worth arguing with. Locked accounts are no longer cheap to reject, so lockout no longer caps the cost of repeatedly probing an account already known to exist. My reasoning is that an attacker gains nothing by locking an account once every username costs the same, so that cheapness was mostly helping the attacker. Tell me if you see it differently.

What this does not cover

An AuthorizationPlugin returning a non-null LoginStatus returns before any of this code, so OIDC, LDAP and similar plugins are unaffected in both directions: nothing here breaks them, and nothing here protects them. Their messages and timing are their own. A plugin that replaces UserController outright through ExtensionLoader would miss this entirely, and grepping this repo will not reveal one.

Left for their own issues: a failure reason on every failed Login event (the reason is known in the controller, but the Login event is built in the servlet from the same LoginStatus the caller receives), and lock status in the user screens plus a REST unlock.

Tests

DefaultUserControllerAuthorizeUserTest covers what a failed login returns, the work it does and the event it dispatches: a locked account gets the generic failure and is indistinguishable from an unknown username, a wrong password for a real account is indistinguishable from an unknown username, every kind of failed attempt runs one credentials query and one digest, a locked account given its correct password still fails without its strikes changing, a pre-2.2 hash still logs in, and the attempt that locks an account dispatches one User account locked event while the attempts after it dispatch none. Those seven fail against current main and pass with this change. An eighth, a plain successful login, passes on both; it guards the path the others never take.

It binds mock ControllerFactory and SqlConfig through Guice static injection, the way DefaultHttpConfigurationTest already does, so no database is involved. Lockout state is set directly on the User, because LoginRequirementsChecker reads the strike count and last strike time straight off it.

No test asserts the timing itself. The mock digester only counts calls, and a real one makes every call take a quarter of a second or more and turns the suite into a flaky performance test. The numbers above come from a running server.

No smoke test in ci/. The unit tests cover the messages, the live checks above cover a running engine, and a timing assertion would be a poor fit for shared runners.

The test injects its mocks with Guice static injection, like the test classes #519 converts. Whichever of the two merges second converts this one.

Clean build, 689 tests, 0 failures.

Credit

This is @mgaffigan's fix from #241, reworked for the current source layout. That branch predates the June move to src/main/java and no longer applies. The timing half and the lockout event are new.

Test rename

PasswordRequirementsTests becomes PasswordRequirementsTest. The build includes **/*Test.class only, so the plural-named class has never run in any build. Six dormant cases now execute and pass. The wider problem is #449's, which makes the same rename byte for byte, so the two merge cleanly in either order.

How to verify by hand

Defaults are retrylimit = 100 and lockoutperiod = 1. That period is in hours rather than minutes, per Duration.standardHours at LoginRequirementsChecker:99.

1. POST /api/users/_login with a real username and a wrong password, 101 times. The 101st locks the account
2. Send the same request for a nonexistent username, and for the locked account with its correct password. All three responses are byte-identical: FAIL, Login failed.
3. GET /api/events?name=User%20account%20locked shows one event for the account, with username and locked until
4. Time the three cases. They come within about 1% of each other instead of hundreds of times apart

Co-authored with Claude Code.

[github-actions]

Test Results

132 files  + 2  132 suites  +2   3m 15s ⏱️ +4s
803 tests +14  803 ✅ +14  0 💤 ±0  0 ❌ ±0 
929 runs  +14  923 ✅ +14  6 💤 ±0  0 ❌ ±0 

Results for commit 6f6261f. ± Comparison against base commit 4589238.

♻️ This comment has been updated with latest results.

[mgaffigan]

I think this is unlikely to prevent timing side channel analysis, but it's better than nothing.

[pacmano1]

I think this is unlikely to prevent timing side channel analysis, but it's better than nothing.

Far from perfect. Not really sure how to best do this by piling on more stuff, especially given the variation in machines and configs. Maybe change authroizeUser a lot or look into rate limiting, etc. At some threshold the behavior does have a pattern. And a single user name look up make the variation after a number of calls easier to see.

[mgaffigan]

I think this is unlikely to prevent timing side channel analysis, but it's better than nothing.

Far from perfect. Not really sure how to best do this by piling on more stuff, especially given the variation in machines and configs. Maybe change authroizeUser a lot or look into rate limiting, etc. At some threshold the behavior does have a pattern. And a single user name look up make the variation after a number of calls easier to see.

Agreed. When a secure implementation is required, my answer remains "Don't try and do user auth; it's deceptively hard. Use an a separate OIDC IdP."

[tonygermano]

My issues before were

• The system does not log an event to inform an admin that a user has been locked out, and all login failures look the same.
  • For example, if there are 5 attempts allowed, the 5th failure should produce a server event separate from the normal login failure indicating the user has been locked.
  • At a minimum, the 6th login failure should contain a reason that the account was locked if the user used the correct credentials.
• The user panel and user edit screens do not indicate if a user is locked

If those things were taken care of, then I don't think we even need a mirth.properties option to keep the old messages; just change them to a generic authentication error message (i.e., do not say "invalid username or password", because that might not be true). Changing the messages without fixing at least some of the above I think creates an unmanageable environment for something that is really low risk in the first place.

Not a blocker for this PR, but should definitely be addressed:

• There is no way over the REST API (and, therefore, from the admin client) for an admin to unlock a user once locked. These should not be linked, but changing the locked user's password does not even work as a workaround, because it doesn't reset the fields related to the lockout.

[pacmano1]

@tonygermano Would this clear it?

• When the strike that locks an account happens, fire a separate "user locked out" server event with the username and the unlock time.
• A locked account that logs in with the correct password gets told it's locked and when it unlocks. A wrong password still gets the generic failure, so only someone who already has the password learns anything. It's the same one digest the locked path already runs, so timing stays equal.

The property stays for now. Lock status in the user panel and edit screens, plus a REST unlock, would be a separate issue and PR. Disabling a user, which doesn't exist at all today, would be its own issue too.

[tonygermano]

@tonygermano Would this clear it?

• When the strike that locks an account happens, fire a separate "user locked out" server event with the username and the unlock time.
• A locked account that logs in with the correct password gets told it's locked and when it unlocks. A wrong password still gets the generic failure, so only someone who already has the password learns anything. It's the same one digest the locked path already runs, so timing stays equal.

The property stays for now. Lock status in the user panel and edit screens, plus a REST unlock, would be a separate issue and PR. Disabling a user, which doesn't exist at all today, would be its own issue too.

@pacmano1 I think just getting the server events worked out would be enough to drop the property and allow this to merge "always on."

The second suggestion you made should not be implemented, because it leaks other information similar to what this PR is trying to prevent. It allows an attacker to brute force passwords for a suspected account while the account is locked and be informed when they have hit the correct one. They would only need to wait for the unlock to happen and then sign in.

I don't know how difficult this would be to implement with the way things are currently set up, but it would be nice if a login failure event always had a failure reason (invalid credentials, account locked, etc...)

[pacmano1]

@tonygermano Done: the property is gone, every failed login says Login failed., and the attempt that locks an account logs a User account locked event with the username and unlock time. Agreed on the second idea, that's a password oracle while locked, so it's dropped. A failure reason on every Login event is #528, and lock status plus an unlock is #529.

Co-authored with Claude Code.

[mgaffigan]

I don't love the equalizeAuthenticationTime - can we use the normal "branchless" model? Do all the same work in all cases and compare once at the end?

[mgaffigan]

Looks great! Thanks

[pacmano1]

Lotta effort here for arguably low value. Can't drive the car if I can't get to the road. Obligatory Car Analogy

[mgaffigan]

Lotta effort here for arguably low value. Can't drive the car if I can't get to the road. Obligatory Car Analogy

100%. My position remains "remove user/pass auth and use OIDC". Defending it is a losing game.

[tonygermano]

Thanks for working through all of this. The lockout event and the generic message cover what I asked for in my earlier review. With lock status and unlock tracked in #529 and failure reasons in #528, I'm good with this going in "always on" without the property.

Three things I'd like done before merge:

1. Update the PR description to match the final code. Parts of it, such as the ponytail: marker and the extra credentials query, describe the approach from before 75d834f. It should also name the timing difference that is left. A wrong password for a real, unlocked account calls incrementStrikes(), which does a DB update and a read, and resetExpiredStrikes() does the same when old strikes have expired. An unknown username does neither. That's small next to the digest, but probably larger than the query difference the description mentions now. I don't think it can be closed without keeping strike rows for usernames that don't exist, so stating it is enough.

2. Squash into a single commit, with a commit message that describes the final version.

3. Add release note text to the PR description, clearly labeled as the release note. This changes what users see when a login fails and how admins find out about locked accounts. The "Behavior change" section is a good starting point.

Guice / #519: The new test uses Guice static injection, which #519 removes. Whichever merges second will need to update this test.

Claude checked that nothing else references FAIL_LOCKED_OUT (keeping the enum value is right, since plugins can still return it), that the pre-2.2 path still upgrades the hash on success, and that a locked account with the correct password fails without its strikes being reset or added to. Also good catch on the PasswordRequirementsTests rename: that class never ran.

Approving on the understanding that these get done before merge.

---

🤖 Drafted with Claude Code

[pacmano1]

@tonygermano @mgaffigan Done: the description has the release note and the strike difference, and it's squashed to one commit. The code is the same as what you approved apart from one comment, which now says attempts already in flight when an account locks each log the event. The squash cleared your approvals.

Co-authored with Claude Code.

[jonbartels]

This is a bit of an obscure issue, but a timing attack to uncover valid usernames is legit, even without AI. The addition of the test is valuable.