fix(auth): scope lockout key to (account, IP) to prevent DOS by login flood
Closes #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 #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 commit is contained in:
SAY-5
@@ -55,12 +55,28 @@ impl LoginLockoutService {
|
||||
}
|
||||
}
|
||||
|
||||
/// Check whether the account is currently locked.
|
||||
/// Build the cache key from the (lowercased) username and the client IP.
|
||||
///
|
||||
/// The IP is part of the key so that an attacker flooding bad passwords
|
||||
/// from one address cannot lock a legitimate user out of the same account
|
||||
/// from a different address (issue #323). When the caller cannot resolve
|
||||
/// a real IP — e.g. `OXICLOUD_TRUST_PROXY_HEADERS=false` and the peer
|
||||
/// address isn't available — `client_ip` should be a non-empty constant
|
||||
/// like `"unknown"`; in that pathological case we fall back to
|
||||
/// account-scoped lockout, which is no worse than the previous
|
||||
/// behaviour.
|
||||
fn key(username: &str, client_ip: &str) -> String {
|
||||
// `|` is not valid in either a username or an IP literal so it makes
|
||||
// the username/ip boundary unambiguous.
|
||||
format!("{}|{}", username.to_lowercase(), client_ip)
|
||||
}
|
||||
|
||||
/// Check whether the (account, IP) pair is currently locked.
|
||||
///
|
||||
/// Returns `Ok(())` if the user may attempt login, or
|
||||
/// `Err(remaining_secs)` with the *approximate* remaining lockout time.
|
||||
pub fn check(&self, username: &str) -> Result<(), u64> {
|
||||
if let Some(rec) = self.cache.get(&username.to_lowercase())
|
||||
pub fn check(&self, username: &str, client_ip: &str) -> Result<(), u64> {
|
||||
if let Some(rec) = self.cache.get(&Self::key(username, client_ip))
|
||||
&& rec.count >= self.max_failures
|
||||
{
|
||||
// The entry exists and is over the threshold. Because moka
|
||||
@@ -71,8 +87,8 @@ impl LoginLockoutService {
|
||||
}
|
||||
|
||||
/// Record a failed login attempt. Returns the new failure count.
|
||||
pub fn record_failure(&self, username: &str) -> u32 {
|
||||
let key = username.to_lowercase();
|
||||
pub fn record_failure(&self, username: &str, client_ip: &str) -> u32 {
|
||||
let key = Self::key(username, client_ip);
|
||||
let new_count = self.cache.get(&key).map(|r| r.count + 1).unwrap_or(1);
|
||||
self.cache
|
||||
.insert(key.clone(), FailureRecord { count: new_count });
|
||||
@@ -80,18 +96,21 @@ impl LoginLockoutService {
|
||||
if new_count >= self.max_failures {
|
||||
tracing::warn!(
|
||||
username = %username,
|
||||
client_ip = %client_ip,
|
||||
attempts = new_count,
|
||||
lockout_secs = self.lockout_secs,
|
||||
"Account temporarily locked after {} consecutive failed login attempts",
|
||||
"Account temporarily locked after {} consecutive failed login attempts from this IP",
|
||||
new_count,
|
||||
);
|
||||
}
|
||||
new_count
|
||||
}
|
||||
|
||||
/// Record a successful login — resets the failure counter.
|
||||
pub fn record_success(&self, username: &str) {
|
||||
self.cache.invalidate(&username.to_lowercase());
|
||||
/// Record a successful login — resets the failure counter for this
|
||||
/// (account, IP) pair so the user isn't penalised for stray earlier
|
||||
/// failures from the same address.
|
||||
pub fn record_success(&self, username: &str, client_ip: &str) {
|
||||
self.cache.invalidate(&Self::key(username, client_ip));
|
||||
}
|
||||
|
||||
/// Maximum failures before lockout (used to inform callers / error messages).
|
||||
@@ -109,42 +128,88 @@ impl LoginLockoutService {
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
const IP1: &str = "1.1.1.1";
|
||||
const IP2: &str = "2.2.2.2";
|
||||
|
||||
#[test]
|
||||
fn allows_login_under_threshold() {
|
||||
let svc = LoginLockoutService::new(3, 60, 100);
|
||||
assert!(svc.check("alice").is_ok());
|
||||
svc.record_failure("alice");
|
||||
svc.record_failure("alice");
|
||||
assert!(svc.check("alice", IP1).is_ok());
|
||||
svc.record_failure("alice", IP1);
|
||||
svc.record_failure("alice", IP1);
|
||||
// 2 failures — still under threshold
|
||||
assert!(svc.check("alice").is_ok());
|
||||
assert!(svc.check("alice", IP1).is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn locks_after_threshold() {
|
||||
let svc = LoginLockoutService::new(3, 60, 100);
|
||||
svc.record_failure("bob");
|
||||
svc.record_failure("bob");
|
||||
svc.record_failure("bob");
|
||||
assert!(svc.check("bob").is_err());
|
||||
svc.record_failure("bob", IP1);
|
||||
svc.record_failure("bob", IP1);
|
||||
svc.record_failure("bob", IP1);
|
||||
assert!(svc.check("bob", IP1).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resets_on_success() {
|
||||
let svc = LoginLockoutService::new(3, 60, 100);
|
||||
svc.record_failure("carol");
|
||||
svc.record_failure("carol");
|
||||
svc.record_success("carol");
|
||||
svc.record_failure("carol", IP1);
|
||||
svc.record_failure("carol", IP1);
|
||||
svc.record_success("carol", IP1);
|
||||
// Counter reset — should be allowed again
|
||||
assert!(svc.check("carol").is_ok());
|
||||
svc.record_failure("carol"); // starts over at 1
|
||||
assert!(svc.check("carol").is_ok());
|
||||
assert!(svc.check("carol", IP1).is_ok());
|
||||
svc.record_failure("carol", IP1); // starts over at 1
|
||||
assert!(svc.check("carol", IP1).is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn case_insensitive() {
|
||||
let svc = LoginLockoutService::new(2, 60, 100);
|
||||
svc.record_failure("Dave");
|
||||
svc.record_failure("dave");
|
||||
assert!(svc.check("DAVE").is_err());
|
||||
svc.record_failure("Dave", IP1);
|
||||
svc.record_failure("dave", IP1);
|
||||
assert!(svc.check("DAVE", IP1).is_err());
|
||||
}
|
||||
|
||||
/// Regression test for #323: flooding bad passwords from one IP must
|
||||
/// NOT lock the account out for legitimate users coming from a
|
||||
/// different IP.
|
||||
#[test]
|
||||
fn does_not_lock_out_other_ips_for_same_account() {
|
||||
let svc = LoginLockoutService::new(3, 60, 100);
|
||||
|
||||
// Attacker hammers the account from IP1 until it locks for that IP.
|
||||
for _ in 0..3 {
|
||||
svc.record_failure("admin", IP1);
|
||||
}
|
||||
assert!(
|
||||
svc.check("admin", IP1).is_err(),
|
||||
"attacker IP must be locked"
|
||||
);
|
||||
|
||||
// A legitimate user coming from IP2 must still be allowed to try.
|
||||
assert!(
|
||||
svc.check("admin", IP2).is_ok(),
|
||||
"second IP must not inherit the lockout — that's the #323 DOS"
|
||||
);
|
||||
}
|
||||
|
||||
/// A successful login on one IP must clear *that* IP's counter only —
|
||||
/// it should NOT silently absolve a separate, ongoing brute-force from
|
||||
/// a different IP against the same account.
|
||||
#[test]
|
||||
fn success_resets_only_the_acting_ip() {
|
||||
let svc = LoginLockoutService::new(3, 60, 100);
|
||||
|
||||
for _ in 0..3 {
|
||||
svc.record_failure("admin", IP1);
|
||||
}
|
||||
// Genuine login from IP2 succeeds; should reset IP2 counter (which
|
||||
// is already 0 here) but leave IP1's lockout intact.
|
||||
svc.record_success("admin", IP2);
|
||||
|
||||
assert!(
|
||||
svc.check("admin", IP1).is_err(),
|
||||
"IP1 must remain locked after IP2's success"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,10 +1,11 @@
|
||||
use axum::{
|
||||
Router,
|
||||
extract::{Json, Query, State},
|
||||
extract::{ConnectInfo, Json, Query, State},
|
||||
http::{HeaderMap, StatusCode},
|
||||
response::{IntoResponse, Redirect, Response},
|
||||
routing::{get, post, put},
|
||||
};
|
||||
use std::net::SocketAddr;
|
||||
use std::sync::Arc;
|
||||
use utoipa::ToSchema;
|
||||
use uuid::Uuid;
|
||||
@@ -18,6 +19,7 @@ use crate::common::di::AppState;
|
||||
use crate::interfaces::api::cookie_auth;
|
||||
use crate::interfaces::errors::AppError;
|
||||
use crate::interfaces::middleware::auth::CurrentUserId;
|
||||
use crate::interfaces::middleware::trusted_proxy::client_ip_from_parts;
|
||||
use serde::Deserialize;
|
||||
|
||||
/// Public auth routes — no authentication required.
|
||||
@@ -260,6 +262,7 @@ pub async fn register(
|
||||
)]
|
||||
pub async fn login(
|
||||
State(state): State<Arc<AppState>>,
|
||||
ConnectInfo(peer): ConnectInfo<SocketAddr>,
|
||||
headers: HeaderMap,
|
||||
Json(dto): Json<LoginDto>,
|
||||
) -> Result<Response, AppError> {
|
||||
@@ -281,13 +284,24 @@ pub async fn login(
|
||||
};
|
||||
|
||||
// ── Account lockout check ──────────────────────────────────────────
|
||||
// Reject immediately if the account has too many consecutive failures.
|
||||
// This runs BEFORE Argon2 to save CPU under brute-force attacks.
|
||||
if let Err(lockout_secs) = auth_service.login_lockout.check(&dto.username) {
|
||||
// Reject immediately if (this account, this IP) has too many consecutive
|
||||
// failures. The IP is part of the key so an attacker flooding bad
|
||||
// passwords from one address cannot lock a legitimate user out of the
|
||||
// same account from a different address (issue #323). The check runs
|
||||
// BEFORE Argon2 to save CPU under brute-force attacks.
|
||||
let client_ip = client_ip_from_parts(&headers, Some(peer), false);
|
||||
if let Err(lockout_secs) = auth_service
|
||||
.login_lockout
|
||||
.check(&dto.username, &client_ip)
|
||||
{
|
||||
tracing::warn!(
|
||||
target: "audit",
|
||||
event = "auth.login",
|
||||
reason = "account_ip_locked",
|
||||
username = %dto.username,
|
||||
ip = %client_ip,
|
||||
lockout_secs = lockout_secs,
|
||||
"Login rejected — account temporarily locked"
|
||||
"Login rejected: account temporarily locked for this IP"
|
||||
);
|
||||
return Err(AppError::new(
|
||||
StatusCode::TOO_MANY_REQUESTS,
|
||||
@@ -317,7 +331,9 @@ pub async fn login(
|
||||
{
|
||||
Ok(auth_response) => {
|
||||
// ── Successful login — reset lockout counter ──
|
||||
auth_service.login_lockout.record_success(&dto.username);
|
||||
auth_service
|
||||
.login_lockout
|
||||
.record_success(&dto.username, &client_ip);
|
||||
|
||||
tracing::info!("Login successful for user: {}", dto.username);
|
||||
// Log the response structure for debugging
|
||||
@@ -367,7 +383,9 @@ pub async fn login(
|
||||
}
|
||||
Err(err) => {
|
||||
// ── Record failed attempt for lockout tracking ──
|
||||
auth_service.login_lockout.record_failure(&dto.username);
|
||||
auth_service
|
||||
.login_lockout
|
||||
.record_failure(&dto.username, &client_ip);
|
||||
tracing::error!("Login failed for user {}: {}", dto.username, err);
|
||||
Err(err.into())
|
||||
}
|
||||
|
||||
@@ -143,11 +143,22 @@ pub fn client_ip<B>(req: &Request<B>, include_port: bool) -> String {
|
||||
.get::<ConnectInfo<SocketAddr>>()
|
||||
.map(|ci| ci.0);
|
||||
|
||||
client_ip_from_parts(req.headers(), peer, include_port)
|
||||
}
|
||||
|
||||
/// Same as [`client_ip`], but operates on already-extracted parts (headers
|
||||
/// plus an optional TCP peer). Handlers that don't take a full `Request<B>`,
|
||||
/// e.g. those that consume the body via `Json<…>`, can still derive a stable
|
||||
/// client identifier with this entry point.
|
||||
pub fn client_ip_from_parts(
|
||||
headers: &axum::http::HeaderMap,
|
||||
peer: Option<SocketAddr>,
|
||||
include_port: bool,
|
||||
) -> String {
|
||||
if let Some(peer_addr) = peer {
|
||||
if is_trusted_proxy(peer_addr.ip()) {
|
||||
// Try X-Forwarded-For first (leftmost = original client)
|
||||
if let Some(xff) = req
|
||||
.headers()
|
||||
if let Some(xff) = headers
|
||||
.get("x-forwarded-for")
|
||||
.and_then(|v| v.to_str().ok())
|
||||
&& let Some(ip) = xff
|
||||
@@ -160,8 +171,7 @@ pub fn client_ip<B>(req: &Request<B>, include_port: bool) -> String {
|
||||
}
|
||||
|
||||
// Then X-Real-Ip
|
||||
if let Some(xri) = req
|
||||
.headers()
|
||||
if let Some(xri) = headers
|
||||
.get("x-real-ip")
|
||||
.and_then(|v| v.to_str().ok())
|
||||
.map(str::trim)
|
||||
|
||||
@@ -62,14 +62,18 @@ pub async fn basic_auth_middleware(
|
||||
let (username, password) =
|
||||
parse_basic_auth(auth_header).ok_or(NextcloudAuthError::Unauthorized)?;
|
||||
|
||||
// Check account lockout before attempting password verification (saves CPU)
|
||||
// Check account lockout before attempting password verification (saves CPU).
|
||||
// The lockout is per (account, IP) — see #323 for rationale.
|
||||
let client_ip =
|
||||
crate::interfaces::middleware::rate_limit::extract_client_ip(&request);
|
||||
if let Some(auth_svc) = state.auth_service.as_ref()
|
||||
&& let Err(secs) = auth_svc.login_lockout.check(&username)
|
||||
&& let Err(secs) = auth_svc.login_lockout.check(&username, &client_ip)
|
||||
{
|
||||
tracing::warn!(
|
||||
username = %username,
|
||||
client_ip = %client_ip,
|
||||
lockout_remaining_secs = secs,
|
||||
"[NC] Account locked — too many failed attempts"
|
||||
"[NC] Account locked — too many failed attempts from this IP"
|
||||
);
|
||||
return Err(NextcloudAuthError::Unauthorized);
|
||||
}
|
||||
@@ -87,7 +91,7 @@ pub async fn basic_auth_middleware(
|
||||
Ok((user_id, uname, email, role)) => {
|
||||
// Reset lockout counter on success
|
||||
if let Some(auth_svc) = state.auth_service.as_ref() {
|
||||
auth_svc.login_lockout.record_success(&username);
|
||||
auth_svc.login_lockout.record_success(&username, &client_ip);
|
||||
}
|
||||
// External users must never authenticate against the NC
|
||||
// surface — that whole subtree (WebDAV files, uploads,
|
||||
@@ -134,7 +138,7 @@ pub async fn basic_auth_middleware(
|
||||
Err(_) => {
|
||||
// Record failed attempt for lockout tracking
|
||||
if let Some(auth_svc) = state.auth_service.as_ref() {
|
||||
auth_svc.login_lockout.record_failure(&username);
|
||||
auth_svc.login_lockout.record_failure(&username, &client_ip);
|
||||
}
|
||||
Err(NextcloudAuthError::Unauthorized)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user