fix(auth): scope lockout key to (account, IP) to prevent DOS by login flood (#323)#326
Open
SAY-5 wants to merge 1 commit intoAtalayaLabs:mainfrom
Open
fix(auth): scope lockout key to (account, IP) to prevent DOS by login flood (#323)#326SAY-5 wants to merge 1 commit intoAtalayaLabs:mainfrom
SAY-5 wants to merge 1 commit intoAtalayaLabs:mainfrom
Conversation
… flood Closes AtalayaLabs#323. LoginLockoutService cached failed-attempt counters keyed only on the username, so any caller that could reach the auth endpoint and guess (or enumerate) a username could lock that account out for the entire lockout window — the rate limiter happily lets each IP make its share of bad-password attempts before clamping, which is enough to trip the per-account threshold in seconds. The reporter demonstrated a complete DOS by spoofing X-Forwarded-For with OXICLOUD_TRUST_PROXY_HEADERS=true. Fix: change the lockout cache key from `username` to `username|ip`. A flood from one IP locks that IP out of that account, but a legitimate user coming from a different IP is unaffected. Changes: - LoginLockoutService::{check, record_failure, record_success} take client_ip as a second argument; cache key is built via Self::key (`format!("{username}|{ip}")`). - middleware/rate_limit.rs: factor out extract_client_ip_from_parts (HeaderMap + Option<&SocketAddr>) so handlers that don't take a full Request<B> can still derive the same client identifier extract_client_ip uses. extract_client_ip now delegates to it. - auth_handler.rs login: derive client_ip from headers (the only signal available without ConnectInfo) and pass it through to all three lockout calls. - nextcloud/basic_auth_middleware.rs: do the same with the full Request via extract_client_ip. Tests: - Updated existing 4 unit tests to thread an IP arg. - New does_not_lock_out_other_ips_for_same_account: lock from IP1, assert IP2 still allowed (the AtalayaLabs#323 regression). - New success_resets_only_the_acting_ip: a successful login from IP2 must NOT clear an attacker's lockout from IP1. Verification: - `cargo build` ✅ - `cargo test login_lockout` → 6 passed (4 existing thread an IP arg without behaviour change, 2 new pin the per-IP scoping). Signed-off-by: SAY-5 <say.apm35@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #323.
What
Change the
LoginLockoutServicecache key fromusernametousername|ip. A flood of bad passwords from one IP locks that IPout of that account; legitimate users coming from a different IP
are unaffected.
Why
The cache previously keyed on username only, so anyone who could
reach
/api/auth/loginand guess (or enumerate) a username couldlock that account out for the full lockout window. The reporter
demonstrated a complete DOS using
X-Forwarded-Forspoofing withOXICLOUD_TRUST_PROXY_HEADERS=true:Every later request from any other IP for
admin2was rejected forthe next 15 minutes — full DOS for that account. The IP-based rate
limiter doesn't help: it caps requests per IP, but the per-account
counter is incremented on every IP's first few attempts before the
limiter clamps each one.
Changes
LoginLockoutService::{check, record_failure, record_success}now take
client_ipas a second argument; cache key is built viaSelf::key(username, client_ip)→format!("{username_lower}|{client_ip}").middleware/rate_limit.rs: factorextract_client_ipintoextract_client_ip_from_parts(&HeaderMap, Option<&SocketAddr>)so handlers that consume the body via
Json<…>(and thereforecan't take a full
Request<B>) can still derive the same clientidentifier.
extract_client_ipnow delegates to it.auth_handler.rslogin: deriveclient_ipfrom the requestheaders and pass it to
check/record_failure/record_success.nextcloud/basic_auth_middleware.rs: same wiring throughextract_client_ip(&request), which already hadRequestaccess.
Tests
login_lockout_service.rsthreaded with an
IP1constant — behaviour unchanged.does_not_lock_out_other_ips_for_same_account(regression):attacker from
IP1locks the account forIP1, butcheckfor
IP2against the same account staysOk.success_resets_only_the_acting_ip: a successful login fromIP2does NOT clear an in-progress brute-force fromIP1.Verification
cargo build✅cargo test login_lockout→ 6 passed.Notes / non-goals
ops who explicitly want it is out of scope here. If desired, that
could be added later as an opt-in env flag — but the default must
be safe against the issue's repro, hence per-IP scoping.
peer is available is the literal
"unknown"(same string therate limiter uses), which degrades gracefully to per-account
scoping for that one bucket — no worse than today.