btlqql opened a new pull request, #4714:
URL: https://github.com/apache/rocketmq-dashboard/pull/4714
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. -->
### Related issue
No open issue covers this path; related prior work is #3045 (closed by
#3046), which fixed two other
fail-open paths of the same limiter and stated the invariant this change
restores - "an active lock must
remain in force until its lock duration expires".
### Brief Description
`LoginRateLimiter` protects the login endpoint in two layers. The exact
tracker holds at most
`MAX_TRACKED_USERNAMES` usernames, and the overflow tracker
(`OVERFLOW_BUCKET_COUNT` hash buckets)
takes over for usernames that arrive while that tracker is full. Both of the
limiter's expiry paths
threw the *whole* overflow tracker away:
- `activeExactState` (`LoginRateLimiter.java:159`) - `attempts.remove(key);
clearOverflowAttempts();`
- the lazy sweep in `reclaimExpiredExactAttemptsIfDue`
(`LoginRateLimiter.java:178`) - `if (removed)
clearOverflowAttempts();`
Freeing one exact slot therefore also cancels every lock in the overflow
tracker, and both of those
paths run on the unauthenticated login endpoint: a caller who fills the
exact tracker, waits for the
first decoy's failure window to lapse, and then sends two new usernames has
the tracker saturated
again - with the overflow tracker empty. Any username that had earned a
5-failure lock then answers
`checkAllowed` with "allowed" for the rest of its `LOCK_DURATION`. That is
attacker-triggered, cheap
(one request per freed slot) and needs no credentials, and it contradicts
the invariant the class
documents for itself ("Collisions can share a lock only while the exact
tracker is saturated; they
cannot disable rate limiting.").
The two paths now drop the overflow state of a finished episode only when no
lock in it is still in
force (`clearStaleOverflowAttempts`), so:
- an active lock survives slot churn and stays enforced for its full
`LOCK_DURATION`;
- an episode's leftovers that carry no lock are still dropped, exactly as
before, so stale overflow
state cannot outlive the episode it belongs to;
- the reset on a **successful** login (`recordSuccess`, which requires valid
credentials) is unchanged.
Nothing was weakened to make this pass: the change only ever *keeps* a
rejection the base code already
had, and the locked-state branch of `incrementFailure` is untouched, so a
lock is still never extended
by later failures. The one visible trade-off is stated for the record: a
bucket lock is shared by
hash collisions, so a colliding username stays rejected for the remainder of
a lock whose duration
overlaps - which is what #3045 asked for, since the alternative is a lock
anybody can cancel.
### How Did You Test This Change?
Three new tests in `LoginRateLimiterTest`, all driven by the existing
`MutableClock` so the windows are
exact. The two defect tests build the sequence above (saturate, lock
`operator` in the overflow
tracker while it still has ~4 minutes of `LOCK_DURATION` left, free a slot
by expiry, take the freed
slot again) and then assert the lock is still in force; the third is the
control for the reset that
must stay.
Before the fix (red), on the unmodified tree:
```
$ cd server && mvn -B -ntp test -Dtest=LoginRateLimiterTest
[ERROR] Tests run: 15, Failures: 2, Errors: 0, Skipped: 0
[ERROR]
LoginRateLimiterTest.lookingUpAnExpiredUsernameMustNotReleaseAnotherUsersOverflowLockTest
-- Time elapsed: 0.023 s <<< FAILURE!
Expecting code to raise a throwable.
at
LoginRateLimiterTest.lookingUpAnExpiredUsernameMustNotReleaseAnotherUsersOverflowLockTest(LoginRateLimiterTest.java:277)
[ERROR]
LoginRateLimiterTest.refillingTheExactTrackerMustNotReleaseAnActiveOverflowLockTest
-- Time elapsed: 0.007 s <<< FAILURE!
Expecting code to raise a throwable.
at
LoginRateLimiterTest.refillingTheExactTrackerMustNotReleaseAnActiveOverflowLockTest(LoginRateLimiterTest.java:249)
```
"Expecting code to raise a throwable" is the failure being reported: the
assertion that
`checkAllowed("operator")` still throws `429` did not throw, i.e. the base
code allowed a locked
username mid-lock. The control test
(`freeingSlotStillDropsOverflowStateThatCarriesNoLockTest`) passed
on the base tree, which is the point: it pins the behaviour that must not
regress.
After the fix (green) - the limiter plus the neighbouring login classes that
construct it:
```
$ cd server && mvn -B -ntp test
-Dtest='LoginRateLimiterTest,AuthServiceDatabaseTest,AuthServiceTest,AuthControllerTest'
[INFO] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.auth.AuthControllerTest
[INFO] Tests run: 27, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.auth.AuthServiceDatabaseTest
[INFO] Tests run: 16, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.auth.AuthServiceTest
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 -- in
org.apache.rocketmq.studio.auth.LoginRateLimiterTest
[INFO] Tests run: 67, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
[INFO] You have 0 Checkstyle violations.
```
Note on the full suite: on a clean `rocketmq-studio` checkout `mvn -B -ntp
test` already reports
`Tests run: 3051, Failures: 6, Errors: 25, Skipped: 4`. The 11 red classes
are the MySQL 8 backed
Spring integration tests (`AuthServiceBootstrapIntegrationTest`,
`AuthServiceConcurrencyIntegrationTest`,
`AuthServiceSessionOverviewIntegrationTest`,
`HealthProbeIntegrationTest`, `QueryHistoryServiceIntegrationTest`,
`NativeAlertEvaluationTransactionTest`,
`NotificationOutboxMapperIntegrationTest`,
`RmqAlertStateMapperIntegrationTest`, `StudioApplicationTest`) plus the
external-CLI ones
(`CliAgentProviderTest`, `ClaudeCodeAgentProviderTest`). None of them are
touched by this change; the
tests here are plain unit tests with an injected clock and need no database.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`fix:`)
- [x] Tests added or updated for non-trivial changes, test methods named
`...Test`
- [x] New UI text has both Chinese and English entries under `web/src/i18n/`
(no UI text in this change)
- [x] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks)
- [x] New source files carry the ASF license header (no new files)
- [x] Documentation touched where behaviour changed (no user-visible
contract change: a locked username keeps answering 429 for its full lock
duration)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
