From a36a36ef6f53d60c4e7490edb170373ebc74937d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 13:36:43 +0000 Subject: [PATCH] Do not count a closed local realm's client passwords towards the lockout A local realm that accepts no passwords from clients holds its provider reject-only in the clients' chain. It refused a password for one of its accounts as Rejected, which BasicAuthSource and the npm legacy login counted as a failed sign-in on the (network, username) lockout that POST /auth/login shares. A client still configured with the password locked its owner out of the browser login. The reject-only provider now answers NotAccepted, folded by the chain into ChainOutcome::NotAccepted, which both paths answer like a wrong password (401, same dummy-hash timing) but release the attempt: nothing was checked, so it is no guess. Wrong passwords on realms that check them still count. Fixes #795 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012e6DJZsSSxZHKzUeJ3TMiG --- crates/core/src/auth/chain.rs | 24 ++++++++- crates/core/src/auth/local_password.rs | 28 +++++++--- crates/core/src/auth/provider.rs | 7 +++ crates/core/src/auth/token.rs | 8 +++ crates/core/tests/local_password.rs | 66 ++++++++++++++++++++++- crates/server/src/auth/mod.rs | 6 +++ crates/server/tests/client_passwords.rs | 72 +++++++++++++++++++++++++ docs/changes/2.2.0/795.fixed.md | 8 +++ 8 files changed, 207 insertions(+), 12 deletions(-) create mode 100644 docs/changes/2.2.0/795.fixed.md diff --git a/crates/core/src/auth/chain.rs b/crates/core/src/auth/chain.rs index 905b4c93..9b1b51f6 100644 --- a/crates/core/src/auth/chain.rs +++ b/crates/core/src/auth/chain.rs @@ -18,6 +18,10 @@ //! user's typed password would leak upstream. A rejection //! from a non-local provider is remembered but does **not** stop the chain //! (a username may exist in a later provider). On its own it never grants. +//! [`AuthOutcome::NotAccepted`] (a local account whose realm takes no +//! password on this chain) is terminal the same way, and folds into +//! [`ChainOutcome::NotAccepted`]: denied like a rejection, but not counted +//! towards the sign-in lockout. //! 4. [`AuthOutcome::Unavailable`] is remembered but does **not** grant. //! **Fail closed:** an unreachable upstream can never open a hole — if no //! provider authenticates and an `Unavailable` was seen, the chain reports @@ -61,6 +65,12 @@ pub enum ChainOutcome { /// No provider authenticated and at least one definitively **rejected** the /// credential. The caller returns 401 (bad credentials), never anonymous. Rejected, + /// A local provider owns the username but takes no password for it on + /// this chain ([`AuthOutcome::NotAccepted`]): the chain stopped there. The + /// caller denies exactly as for [`ChainOutcome::Rejected`] (same status, + /// body and timing), but no password was checked, so the attempt does not + /// count towards the sign-in lockout. + NotAccepted, /// No provider authenticated and at least one was **unavailable** (and none /// rejected). Fail-closed: the caller denies. Surfaced distinctly from /// [`ChainOutcome::Rejected`] so the caller can log/metric upstream health, @@ -145,6 +155,12 @@ impl Chain { AuthOutcome::Rejected if provider.source() == AuthSource::Local => { return (ChainOutcome::Rejected, source); } + // A local account whose realm takes no password here: final + // like a wrong password (never forwarded), but nothing was + // checked. + AuthOutcome::NotAccepted if provider.source() == AuthSource::Local => { + return (ChainOutcome::NotAccepted, source); + } // The local store could not decide (DB error, argon2 overload): // it may still own this username, so do not forward (rule 4). AuthOutcome::Unavailable if provider.source() == AuthSource::Local => { @@ -152,7 +168,9 @@ impl Chain { } // The database failed: no later provider can decide either. AuthOutcome::Failed => return (ChainOutcome::Failed, source), - AuthOutcome::Rejected => saw_rejected = saw_rejected.or(source), + AuthOutcome::Rejected | AuthOutcome::NotAccepted => { + saw_rejected = saw_rejected.or(source) + } AuthOutcome::Unavailable => saw_unavailable = saw_unavailable.or(source), AuthOutcome::Skip => {} } @@ -184,7 +202,9 @@ fn record_sign_in(outcome: &ChainOutcome, decided_by: Option) { }; let outcome = match outcome { ChainOutcome::Authenticated { .. } => SignInOutcome::Success, - ChainOutcome::Rejected | ChainOutcome::NoCredentials => SignInOutcome::Rejected, + ChainOutcome::Rejected | ChainOutcome::NotAccepted | ChainOutcome::NoCredentials => { + SignInOutcome::Rejected + } ChainOutcome::Unavailable => SignInOutcome::Unavailable, ChainOutcome::Failed => SignInOutcome::Error, }; diff --git a/crates/core/src/auth/local_password.rs b/crates/core/src/auth/local_password.rs index 2ceb3fac..962bae25 100644 --- a/crates/core/src/auth/local_password.rs +++ b/crates/core/src/auth/local_password.rs @@ -242,8 +242,9 @@ impl LocalPasswordProvider { } /// Check no password, but keep the local accounts' names: a name the - /// local store owns is refused (after a dummy verification, so it answers - /// like a wrong password), and only a foreign one goes on to the next + /// local store owns is refused as `NotAccepted` (after a dummy + /// verification, so it answers like a wrong password, but does not count + /// towards the sign-in lockout), and only a foreign one goes on to the next /// provider. The chain of a local realm that accepts no passwords from /// clients holds the provider this way, so that such a password is never /// forwarded to a credential-forwarding realm behind it. @@ -252,6 +253,21 @@ impl LocalPasswordProvider { self } + /// How a name the local store owns is refused without checking its + /// password: `Rejected`, or `NotAccepted` when this provider checks no + /// password at all ([`Self::reject_only`]). Nothing is verified then, so + /// the refusal is no guess and does not count towards the sign-in + /// lockout: a client still sending an account's password to a realm that + /// takes none from clients must not lock its owner out of the browser + /// login. It answers the same either way. + fn refused(&self) -> AuthOutcome { + if self.verifies { + AuthOutcome::Rejected + } else { + AuthOutcome::NotAccepted + } + } + /// `Skip` for a name that is not a local password account: at once when /// forwarding follows, after a dummy verification otherwise. async fn skip_foreign(&self, candidate: &str) -> AuthOutcome { @@ -332,9 +348,7 @@ impl AuthProvider for LocalPasswordProvider { // and no upstream realm is asked about it. let Some(stored) = user.password_hash.clone() else { if user.auth_source == "local" || user.is_service_account() { - return self - .after_dummy_verify(cred.password, AuthOutcome::Rejected) - .await; + return self.after_dummy_verify(cred.password, self.refused()).await; } return self.skip_foreign(cred.password).await; }; @@ -345,9 +359,7 @@ impl AuthProvider for LocalPasswordProvider { // (upstream) realm. It costs a (dummy) verification and answers like a // wrong password, so it does not show that the account is deactivated. if !user.active || !self.verifies { - return self - .after_dummy_verify(cred.password, AuthOutcome::Rejected) - .await; + return self.after_dummy_verify(cred.password, self.refused()).await; } match self.limiter.verify(stored, cred.password.to_string()).await { diff --git a/crates/core/src/auth/provider.rs b/crates/core/src/auth/provider.rs index 9ca14f7d..f8eec69a 100644 --- a/crates/core/src/auth/provider.rs +++ b/crates/core/src/auth/provider.rs @@ -179,6 +179,13 @@ pub enum AuthOutcome { /// wrong local password). Never grants; terminal when the provider's source /// is [`AuthSource::Local`], otherwise the chain continues. Rejected, + /// The provider owns this username but takes no password for it here + /// (a local realm that accepts no passwords from clients, see + /// `LocalPasswordProvider::reject_only`): refused like a wrong password, + /// but nothing was checked, so it is no guess and does not count towards + /// the sign-in lockout. Terminal when the provider's source is + /// [`AuthSource::Local`], otherwise treated like [`Self::Rejected`]. + NotAccepted, /// The provider can't decide — typically "this username isn't mine". /// The chain advances to the next provider. Skip, diff --git a/crates/core/src/auth/token.rs b/crates/core/src/auth/token.rs index ddc0960e..d9b9ae1e 100644 --- a/crates/core/src/auth/token.rs +++ b/crates/core/src/auth/token.rs @@ -489,6 +489,14 @@ impl PrincipalSource for BasicAuthSource { attempt.failed(); return SourceOutcome::Invalid; } + // A local account whose realm takes no password from clients: + // denied like a wrong password, but nothing was checked, so it + // does not count towards the lockout (an old client still + // sending it must not lock its owner out of the browser login). + ChainOutcome::NotAccepted => { + attempt.released(); + return SourceOutcome::Invalid; + } // A fail-closed outage (or an argon2 overload) also denies, but // is not the caller's fault, so it does not count towards the // lockout. diff --git a/crates/core/tests/local_password.rs b/crates/core/tests/local_password.rs index 44913f61..e9525f7b 100644 --- a/crates/core/tests/local_password.rs +++ b/crates/core/tests/local_password.rs @@ -174,7 +174,9 @@ async fn a_reject_only_provider_refuses_its_names_and_skips_the_others() { // #647: a local realm that accepts no passwords from clients keeps its // names in the clients' chain: the right password of an active local // account is refused like a wrong one (never Skip, which would forward it - // upstream), and only a name the local store does not own goes on. + // upstream), and only a name the local store does not own goes on. #795: + // nothing is checked, so the refusal is `NotAccepted`, not a `Rejected` + // guess. let pool = common::pool().await; let username = insert_local(&pool, "pw", true).await; let unknown = format!("nobody-{}@example.com", common::unique()); @@ -185,7 +187,7 @@ async fn a_reject_only_provider_refuses_its_names_and_skips_the_others() { for password in ["pw", "a-guess"] { assert_eq!( provider.authenticate(&cred(&username, password)).await, - AuthOutcome::Rejected, + AuthOutcome::NotAccepted, "{password} (forwarding follows: {forwarding_follows})" ); } @@ -204,6 +206,66 @@ async fn a_reject_only_provider_refuses_its_names_and_skips_the_others() { )); } +/// #795: a password a reject-only local provider refuses was never checked, +/// so it is no guess. Over Basic it answers 401 like a wrong password, but it +/// does not count towards the (network, username) lockout the browser login +/// shares; a wrong password through a provider that checks still counts. +#[tokio::test] +async fn a_reject_only_refusal_is_not_counted_towards_the_lockout() { + let pool = common::pool().await; + let username = insert_local(&pool, "pw", true).await; + let (upstream, upstream_calls) = CountingProvider::new( + AuthSource::UpstreamBasic, + AuthOutcome::Rejected, + Duration::ZERO, + ); + let chain = Arc::new(Chain::new(vec![ + Box::new( + LocalPasswordProvider::new(pool.clone()) + .forwarding_follows(true) + .reject_only(), + ), + Box::new(upstream), + ])); + assert_eq!( + chain.authenticate(&cred(&username, "pw")).await, + ChainOutcome::NotAccepted + ); + + let throttle = Arc::new(LoginThrottle::new(5, Duration::from_secs(60))); + let src = + BasicAuthSource::with_chain(pool.clone(), Some(chain)).with_throttle(throttle.clone()); + for password in ["pw", "a-guess"] { + assert_eq!( + src.resolve(&basic_header(&username, password), ip()).await, + SourceOutcome::Invalid, + "{password}" + ); + } + assert_eq!( + throttle.tracked(), + 0, + "a refusal without a check is no failure" + ); + assert_eq!( + upstream_calls.load(Ordering::SeqCst), + 0, + "a local account's password never reaches the upstream" + ); + + // A provider that checks passwords still counts a wrong one. + let checking = Arc::new(Chain::new(vec![ + Box::new(LocalPasswordProvider::new(pool.clone())) as Box, + ])); + let src = + BasicAuthSource::with_chain(pool.clone(), Some(checking)).with_throttle(throttle.clone()); + assert_eq!( + src.resolve(&basic_header(&username, "a-guess"), ip()).await, + SourceOutcome::Invalid + ); + assert_eq!(throttle.tracked(), 1, "a wrong password is counted"); +} + #[tokio::test] async fn local_provider_rejects_an_invited_account() { // #322: an invited local account has no password until its user chooses diff --git a/crates/server/src/auth/mod.rs b/crates/server/src/auth/mod.rs index 4328d2d1..8e42e5da 100644 --- a/crates/server/src/auth/mod.rs +++ b/crates/server/src/auth/mod.rs @@ -1189,6 +1189,12 @@ pub(crate) async fn verify_password_login( attempt.released(); return Err(LoginRefusal::Invalid); } + // A local account whose realm takes no password from clients: no + // password was checked, so it is no guess and does not count either. + ChainOutcome::NotAccepted => { + attempt.released(); + return Err(LoginRefusal::Invalid); + } // Forge's own database failed: not a verdict on the credentials. ChainOutcome::Failed => { attempt.released(); diff --git a/crates/server/tests/client_passwords.rs b/crates/server/tests/client_passwords.rs index 09af87c8..16e5540d 100644 --- a/crates/server/tests/client_passwords.rs +++ b/crates/server/tests/client_passwords.rs @@ -318,6 +318,78 @@ async fn a_forwarding_realm_takes_client_passwords_and_a_closed_local_realm_none srv.login(&setup.forwarded, UPSTREAM_PASSWORD).await; } +/// #795: a closed local realm's password sent by a client was never checked, +/// so it is no guess: refused like a wrong one, but not counted towards the +/// (network, username) lockout the browser login shares. An old client still +/// sending it does not lock its owner out of the sign-in that gets them a +/// token. A wrong password through a realm that checks it still counts. +#[tokio::test] +async fn a_closed_local_realms_password_from_clients_does_not_lock_the_browser_login() { + let setup = Setup::new().await; + let srv = &setup.srv; + let (admin_user, admin_password) = srv.admin_password_basic(); + let settings = srv.state.auth_settings.current().await.settings.clone(); + let attempts = 15; + assert!(attempts > settings.login_max_failures); + + let raw = format!("{}/repository/{}/{RAW_FILE}", srv.base_url, setup.raw); + for _ in 0..attempts { + let resp = srv + .client() + .get(&raw) + .basic_auth(&admin_user, Some(&admin_password)) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 401, "Basic with a closed realm's password"); + } + // `npm login --auth-type=legacy` refuses it the same way. + let npm_login = format!( + "{}/repository/{}/-/user/org.couchdb.user:{}", + srv.base_url, + setup.npm, + admin_user.replace('@', "%40") + ); + for _ in 0..attempts { + let resp = srv + .client() + .put(&npm_login) + .json(&json!({"name": admin_user, "password": admin_password, "type": "user"})) + .send() + .await + .unwrap(); + assert_eq!( + resp.status(), + 401, + "npm login with a closed realm's password" + ); + } + // From the same address, the browser login still takes the password. + srv.login(&admin_user, &admin_password).await; + + // Wrong passwords still count: the browser login checks them, and so + // locks the pair once the budget is spent. + let login = format!("{}/auth/login", srv.base_url); + for _ in 0..settings.login_max_failures { + let resp = srv + .client() + .post(&login) + .json(&json!({"username": admin_user, "password": "not-the-password"})) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 401); + } + let resp = srv + .client() + .post(&login) + .json(&json!({"username": admin_user, "password": admin_password})) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 429, "wrong passwords still lock the pair"); +} + /// Each realm's flag follows its admin edits at once: the local realm opened /// takes its users' passwords from clients, the forwarding realm closed /// takes none (its users then sign their clients in with tokens, which the diff --git a/docs/changes/2.2.0/795.fixed.md b/docs/changes/2.2.0/795.fixed.md new file mode 100644 index 00000000..0edc39c6 --- /dev/null +++ b/docs/changes/2.2.0/795.fixed.md @@ -0,0 +1,8 @@ +A local realm that accepts no passwords from clients counted each password +a client still sent for one of its accounts (HTTP Basic on the feeds and +`/v2/token`, `npm login --auth-type=legacy`) as a failed sign-in. A NuGet or +docker client configured with the password before the upgrade therefore +locked its owner out of the browser login (429) from the same network, the +sign-in that gets them a token. Such a password is still refused like a +wrong one, but no longer counts towards the lockout: nothing is checked on +that path. Wrong passwords for realms that check them count as before.