From c7499ab67db8b297e075edffd57d9586bad5658a Mon Sep 17 00:00:00 2001 From: mdecimus Date: Fri, 27 Dec 2024 14:51:11 +0100 Subject: [PATCH] Removed authentication rate limit (unnecessary since there is fail2ban) --- crates/common/src/config/jmap/settings.rs | 4 -- crates/common/src/lib.rs | 7 +- crates/imap/src/op/authenticate.rs | 7 -- crates/jmap/src/api/http.rs | 76 ++++++++++++++++------ crates/jmap/src/api/management/stores.rs | 7 +- crates/jmap/src/auth/authenticate.rs | 27 +++----- crates/jmap/src/auth/oauth/registration.rs | 3 +- crates/jmap/src/auth/rate_limit.rs | 62 +++++------------- crates/managesieve/src/op/authenticate.rs | 4 -- crates/pop3/src/op/authenticate.rs | 4 -- tests/src/jmap/auth_limits.rs | 54 ++++++--------- tests/src/jmap/mod.rs | 6 +- 12 files changed, 115 insertions(+), 146 deletions(-) diff --git a/crates/common/src/config/jmap/settings.rs b/crates/common/src/config/jmap/settings.rs index cf6b105e..87b97589 100644 --- a/crates/common/src/config/jmap/settings.rs +++ b/crates/common/src/config/jmap/settings.rs @@ -45,7 +45,6 @@ pub struct JmapConfig { pub session_cache_ttl: Duration, pub rate_authenticated: Option, - pub rate_authenticate_req: Option, pub rate_anonymous: Option, pub event_source_throttle: Duration, @@ -305,9 +304,6 @@ impl JmapConfig { rate_authenticated: config .property_or_default::>("jmap.rate-limit.account", "1000/1m") .unwrap_or_default(), - rate_authenticate_req: config - .property_or_default::>("authentication.rate-limit", "10/1m") - .unwrap_or_default(), rate_anonymous: config .property_or_default::>("jmap.rate-limit.anonymous", "100/1m") .unwrap_or_default(), diff --git a/crates/common/src/lib.rs b/crates/common/src/lib.rs index 30018611..71108b4b 100644 --- a/crates/common/src/lib.rs +++ b/crates/common/src/lib.rs @@ -66,10 +66,9 @@ pub const KV_RATE_LIMIT_LOITER: u8 = 4; pub const KV_RATE_LIMIT_AUTH: u8 = 5; pub const KV_RATE_LIMIT_HASH: u8 = 6; pub const KV_RATE_LIMIT_CONTACT: u8 = 7; -pub const KV_RATE_LIMIT_JMAP: u8 = 8; -pub const KV_RATE_LIMIT_JMAP_AUTH: u8 = 9; -pub const KV_RATE_LIMIT_HTTP_ANONYM: u8 = 10; -pub const KV_RATE_LIMIT_IMAP: u8 = 11; +pub const KV_RATE_LIMIT_HTTP_AUTHENTICATED: u8 = 8; +pub const KV_RATE_LIMIT_HTTP_ANONYMOUS: u8 = 9; +pub const KV_RATE_LIMIT_IMAP: u8 = 10; pub const KV_REPUTATION_IP: u8 = 12; pub const KV_REPUTATION_FROM: u8 = 13; pub const KV_REPUTATION_DOMAIN: u8 = 14; diff --git a/crates/imap/src/op/authenticate.rs b/crates/imap/src/op/authenticate.rs index c4140ec8..bbfa13f5 100644 --- a/crates/imap/src/op/authenticate.rs +++ b/crates/imap/src/op/authenticate.rs @@ -17,7 +17,6 @@ use imap_proto::{ receiver::{self, Request}, Command, ResponseCode, StatusResponse, }; -use jmap::auth::rate_limit::RateLimiter; use mail_parser::decoders::base64::base64_decode; use mail_send::Credentials; use std::sync::Arc; @@ -76,12 +75,6 @@ impl Session { credentials: Credentials, tag: String, ) -> trc::Result<()> { - // Throttle authentication requests - self.server - .is_auth_allowed_soft(&self.remote_addr) - .await - .map_err(|err| err.id(tag.clone()))?; - // Authenticate let access_token = self .server diff --git a/crates/jmap/src/api/http.rs b/crates/jmap/src/api/http.rs index b311ddfc..5340794e 100644 --- a/crates/jmap/src/api/http.rs +++ b/crates/jmap/src/api/http.rs @@ -232,13 +232,15 @@ impl ParseHttp for Server { } ("oauth-authorization-server", &Method::GET) => { // Limit anonymous requests - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; return self.handle_oauth_metadata(req, session).await; } ("openid-configuration", &Method::GET) => { // Limit anonymous requests - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; return self.handle_oidc_metadata(req, session).await; } @@ -258,6 +260,10 @@ impl ParseHttp for Server { } } ("mta-sts.txt", &Method::GET) => { + // Limit anonymous requests + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; + return if let Some(policy) = self.build_mta_sts_policy() { Ok(Resource::new("text/plain", policy.to_string().into_bytes()) .into_http_response()) @@ -266,12 +272,20 @@ impl ParseHttp for Server { }; } ("mail-v1.xml", &Method::GET) => { + // Limit anonymous requests + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; + return self.handle_autoconfig_request(&req).await; } ("autoconfig", &Method::GET) => { if path.next().unwrap_or_default() == "mail" && path.next().unwrap_or_default() == "config-v1.1.xml" { + // Limit anonymous requests + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; + return self.handle_autoconfig_request(&req).await; } } @@ -282,12 +296,14 @@ impl ParseHttp for Server { }, "auth" => match (path.next().unwrap_or_default(), req.method()) { ("device", &Method::POST) => { - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; return self.handle_device_auth(&mut req, session).await; } ("token", &Method::POST) => { - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; return self.handle_token_request(&mut req, session).await; } @@ -314,7 +330,8 @@ impl ParseHttp for Server { } ("jwks.json", &Method::GET) => { // Limit anonymous requests - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; return Ok(self.core.oauth.oidc_jwks.clone().into_http_response()); } @@ -408,6 +425,10 @@ impl ParseHttp for Server { if req.method() == Method::GET && path.next().unwrap_or_default() == "config-v1.1.xml" { + // Limit anonymous requests + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; + return self.handle_autoconfig_request(&req).await; } } @@ -415,6 +436,10 @@ impl ParseHttp for Server { if req.method() == Method::POST && path.next().unwrap_or_default() == "autodiscover.xml" { + // Limit anonymous requests + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; + return self .handle_autodiscover_request( fetch_body(&mut req, 8192, session.session_id).await, @@ -423,27 +448,37 @@ impl ParseHttp for Server { } } "robots.txt" => { + // Limit anonymous requests + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; + return Ok( Resource::new("text/plain", b"User-agent: *\nDisallow: /\n".to_vec()) .into_http_response(), ); } - "healthz" => match path.next().unwrap_or_default() { - "live" => { - return Ok(StatusCode::OK.into_http_response()); - } - "ready" => { - return Ok({ - if !self.core.storage.data.is_none() { - StatusCode::OK - } else { - StatusCode::SERVICE_UNAVAILABLE - } + "healthz" => { + // Limit anonymous requests + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; + + match path.next().unwrap_or_default() { + "live" => { + return Ok(StatusCode::OK.into_http_response()); } - .into_http_response()); + "ready" => { + return Ok({ + if !self.core.storage.data.is_none() { + StatusCode::OK + } else { + StatusCode::SERVICE_UNAVAILABLE + } + } + .into_http_response()); + } + _ => (), } - _ => (), - }, + } "metrics" => match path.next().unwrap_or_default() { "prometheus" => { if let Some(prometheus) = &self.core.metrics.prometheus { @@ -508,7 +543,8 @@ impl ParseHttp for Server { if let Some(form) = &self.core.network.contact_form { match *req.method() { Method::POST => { - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; let form_data = FormData::from_request(&mut req, form.max_size, session.session_id) diff --git a/crates/jmap/src/api/management/stores.rs b/crates/jmap/src/api/management/stores.rs index 6224bcb0..3a96a44a 100644 --- a/crates/jmap/src/api/management/stores.rs +++ b/crates/jmap/src/api/management/stores.rs @@ -144,9 +144,10 @@ impl ManageStore for Server { Some("rate-auth") => vec![KV_RATE_LIMIT_AUTH].into(), Some("rate-hash") => vec![KV_RATE_LIMIT_HASH].into(), Some("rate-contact") => vec![KV_RATE_LIMIT_CONTACT].into(), - Some("rate-jmap") => vec![KV_RATE_LIMIT_JMAP].into(), - Some("rate-jmap-auth") => vec![KV_RATE_LIMIT_JMAP_AUTH].into(), - Some("rate-http-anonymous") => vec![KV_RATE_LIMIT_HTTP_ANONYM].into(), + Some("rate-http-authenticated") => { + vec![KV_RATE_LIMIT_HTTP_AUTHENTICATED].into() + } + Some("rate-http-anonymous") => vec![KV_RATE_LIMIT_HTTP_ANONYMOUS].into(), Some("rate-imap") => vec![KV_RATE_LIMIT_IMAP].into(), Some("reputation-ip") => vec![KV_REPUTATION_IP].into(), Some("reputation-from") => vec![KV_REPUTATION_FROM].into(), diff --git a/crates/jmap/src/auth/authenticate.rs b/crates/jmap/src/auth/authenticate.rs index 4e04d9ab..c26f071e 100644 --- a/crates/jmap/src/auth/authenticate.rs +++ b/crates/jmap/src/auth/authenticate.rs @@ -41,9 +41,6 @@ impl Authenticator for Server { self.get_cached_access_token(account_id).await? } else { let credentials = if mechanism.eq_ignore_ascii_case("basic") { - // Throttle authentication requests - self.is_auth_allowed_soft(&session.remote_ip).await?; - // Decode the base64 encoded credentials decode_plain_auth(token).ok_or_else(|| { trc::AuthEvent::Error @@ -54,7 +51,8 @@ impl Authenticator for Server { })? } else if mechanism.eq_ignore_ascii_case("bearer") { // Enforce anonymous rate limit - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; decode_bearer_token(token, allow_api_access).ok_or_else(|| { trc::AuthEvent::Error @@ -65,7 +63,8 @@ impl Authenticator for Server { })? } else { // Enforce anonymous rate limit - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; return Err(trc::AuthEvent::Error .into_err() @@ -75,22 +74,13 @@ impl Authenticator for Server { }; // Authenticate - let access_token = match self + let access_token = self .authenticate(&AuthRequest::from_credentials( credentials, session.session_id, session.remote_ip, )) - .await - { - Ok(access_token) => access_token, - Err(err) => { - if err.matches(trc::EventType::Auth(trc::AuthEvent::Failed)) { - let _ = self.is_auth_allowed_hard(&session.remote_ip).await; - } - return Err(err); - } - }; + .await?; // Cache session self.cache_session(token.to_string(), &access_token); @@ -98,12 +88,13 @@ impl Authenticator for Server { }; // Enforce authenticated rate limit - self.is_account_allowed(&access_token) + self.is_http_authenticated_request_allowed(&access_token) .await .map(|in_flight| (in_flight, access_token)) } else { // Enforce anonymous rate limit - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; Err(trc::AuthEvent::Failed .into_err() diff --git a/crates/jmap/src/auth/oauth/registration.rs b/crates/jmap/src/auth/oauth/registration.rs index c140182f..13c0f9c8 100644 --- a/crates/jmap/src/auth/oauth/registration.rs +++ b/crates/jmap/src/auth/oauth/registration.rs @@ -54,7 +54,8 @@ impl ClientRegistrationHandler for Server { // Validate permissions access_token.assert_has_permission(Permission::OauthClientRegistration)?; } else { - self.is_anonymous_allowed(&session.remote_ip).await?; + self.is_http_anonymous_request_allowed(&session.remote_ip) + .await?; } // Parse request diff --git a/crates/jmap/src/auth/rate_limit.rs b/crates/jmap/src/auth/rate_limit.rs index bfe62981..4859b5bd 100644 --- a/crates/jmap/src/auth/rate_limit.rs +++ b/crates/jmap/src/auth/rate_limit.rs @@ -9,8 +9,7 @@ use std::{net::IpAddr, sync::Arc}; use common::{ ip_to_bytes, listener::limiter::{ConcurrencyLimiter, InFlight}, - ConcurrencyLimiters, Server, KV_RATE_LIMIT_HTTP_ANONYM, KV_RATE_LIMIT_JMAP, - KV_RATE_LIMIT_JMAP_AUTH, + ConcurrencyLimiters, Server, KV_RATE_LIMIT_HTTP_ANONYMOUS, KV_RATE_LIMIT_HTTP_AUTHENTICATED, }; use directory::Permission; use trc::AddContext; @@ -20,14 +19,15 @@ use std::future::Future; pub trait RateLimiter: Sync + Send { fn get_concurrency_limiter(&self, account_id: u32) -> Arc; - fn is_account_allowed( + fn is_http_authenticated_request_allowed( &self, access_token: &AccessToken, ) -> impl Future> + Send; - fn is_anonymous_allowed(&self, addr: &IpAddr) -> impl Future> + Send; + fn is_http_anonymous_request_allowed( + &self, + addr: &IpAddr, + ) -> impl Future> + Send; fn is_upload_allowed(&self, access_token: &AccessToken) -> trc::Result; - fn is_auth_allowed_soft(&self, addr: &IpAddr) -> impl Future> + Send; - fn is_auth_allowed_hard(&self, addr: &IpAddr) -> impl Future> + Send; } impl RateLimiter for Server { @@ -54,14 +54,17 @@ impl RateLimiter for Server { }) } - async fn is_account_allowed(&self, access_token: &AccessToken) -> trc::Result { + async fn is_http_authenticated_request_allowed( + &self, + access_token: &AccessToken, + ) -> trc::Result { let limiter = self.get_concurrency_limiter(access_token.primary_id()); let is_rate_allowed = if let Some(rate) = &self.core.jmap.rate_authenticated { self.core .storage .lookup .is_rate_allowed( - KV_RATE_LIMIT_JMAP, + KV_RATE_LIMIT_HTTP_AUTHENTICATED, &access_token.primary_id.to_be_bytes(), rate, false, @@ -88,13 +91,18 @@ impl RateLimiter for Server { } } - async fn is_anonymous_allowed(&self, addr: &IpAddr) -> trc::Result<()> { + async fn is_http_anonymous_request_allowed(&self, addr: &IpAddr) -> trc::Result<()> { if let Some(rate) = &self.core.jmap.rate_anonymous { if self .core .storage .lookup - .is_rate_allowed(KV_RATE_LIMIT_HTTP_ANONYM, &ip_to_bytes(addr), rate, false) + .is_rate_allowed( + KV_RATE_LIMIT_HTTP_ANONYMOUS, + &ip_to_bytes(addr), + rate, + false, + ) .await .caused_by(trc::location!())? .is_some() @@ -118,38 +126,4 @@ impl RateLimiter for Server { Err(trc::LimitEvent::ConcurrentUpload.into_err()) } } - - async fn is_auth_allowed_soft(&self, addr: &IpAddr) -> trc::Result<()> { - if let Some(rate) = &self.core.jmap.rate_authenticate_req { - if self - .core - .storage - .lookup - .is_rate_allowed(KV_RATE_LIMIT_JMAP_AUTH, &ip_to_bytes(addr), rate, true) - .await - .caused_by(trc::location!())? - .is_some() - { - return Err(trc::AuthEvent::TooManyAttempts.into_err()); - } - } - Ok(()) - } - - async fn is_auth_allowed_hard(&self, addr: &IpAddr) -> trc::Result<()> { - if let Some(rate) = &self.core.jmap.rate_authenticate_req { - if self - .core - .storage - .lookup - .is_rate_allowed(KV_RATE_LIMIT_JMAP_AUTH, &ip_to_bytes(addr), rate, false) - .await - .caused_by(trc::location!())? - .is_some() - { - return Err(trc::AuthEvent::TooManyAttempts.into_err()); - } - } - Ok(()) - } } diff --git a/crates/managesieve/src/op/authenticate.rs b/crates/managesieve/src/op/authenticate.rs index 893a33a1..9093b788 100644 --- a/crates/managesieve/src/op/authenticate.rs +++ b/crates/managesieve/src/op/authenticate.rs @@ -17,7 +17,6 @@ use imap_proto::{ protocol::authenticate::Mechanism, receiver::{self, Request}, }; -use jmap::auth::rate_limit::RateLimiter; use mail_parser::decoders::base64::base64_decode; use std::sync::Arc; @@ -71,9 +70,6 @@ impl Session { } }; - // Throttle authentication requests - self.server.is_auth_allowed_soft(&self.remote_addr).await?; - // Authenticate let access_token = self .server diff --git a/crates/pop3/src/op/authenticate.rs b/crates/pop3/src/op/authenticate.rs index 795043ce..d600cca9 100644 --- a/crates/pop3/src/op/authenticate.rs +++ b/crates/pop3/src/op/authenticate.rs @@ -13,7 +13,6 @@ use common::{ ConcurrencyLimiters, }; use directory::Permission; -use jmap::auth::rate_limit::RateLimiter; use mail_parser::decoders::base64::base64_decode; use mail_send::Credentials; use std::sync::Arc; @@ -68,9 +67,6 @@ impl Session { } pub async fn handle_auth(&mut self, credentials: Credentials) -> trc::Result<()> { - // Throttle authentication requests - self.server.is_auth_allowed_soft(&self.remote_addr).await?; - // Authenticate let access_token = self .server diff --git a/tests/src/jmap/auth_limits.rs b/tests/src/jmap/auth_limits.rs index a5028b15..6640d443 100644 --- a/tests/src/jmap/auth_limits.rs +++ b/tests/src/jmap/auth_limits.rs @@ -68,40 +68,6 @@ pub async fn test(params: &mut JMAPTest) { let range_end = (range_start * LIMIT) + LIMIT; tokio::time::sleep(Duration::from_secs(range_end - now)).await; - // Invalid authentication requests should be rate limited - let mut n_401 = 0; - let mut n_429 = 0; - for n in 0..110 { - if let Err(jmap_client::Error::Problem(problem)) = Client::new() - .credentials(Credentials::basic( - "not_an_account@example.com", - &format!("brute_force{}", n), - )) - .accept_invalid_certs(true) - .connect("https://127.0.0.1:8899") - .await - { - if problem.status().unwrap() == 401 { - n_401 += 1; - if n_401 > 100 { - panic!("Rate limiter failed: 429: {n_429}, 401: {n_401}."); - } - } else if problem.status().unwrap() == 429 { - n_429 += 1; - if n_429 > 11 { - panic!("Rate limiter too restrictive: 429: {n_429}, 401: {n_401}."); - } - } else { - panic!("Unexpected error status {}", problem.status().unwrap()); - } - } else { - panic!("Unexpected response."); - } - } - - // Limit should be restored after 1 second - tokio::time::sleep(Duration::from_millis(1500)).await; - // Test fail2ban assert_eq!( server @@ -113,6 +79,26 @@ pub async fn test(params: &mut JMAPTest) { .unwrap(), None ); + for n in 0..98 { + match Client::new() + .credentials(Credentials::basic( + "not_an_account@example.com", + &format!("brute_force{}", n), + )) + .accept_invalid_certs(true) + .connect("https://127.0.0.1:8899") + .await + { + Err(jmap_client::Error::Problem(_)) => {} + Err(err) => { + panic!("Unexpected response: {:?}", err); + } + Ok(_) => { + panic!("Unexpected success"); + } + } + } + let mut imap = ImapConnection::connect(b"_x ").await; imap.send("AUTHENTICATE PLAIN AGpvaG4AY2hpbWljaGFuZ2Fz") .await; diff --git a/tests/src/jmap/mod.rs b/tests/src/jmap/mod.rs index 9cb2941c..a0765e31 100644 --- a/tests/src/jmap/mod.rs +++ b/tests/src/jmap/mod.rs @@ -120,7 +120,7 @@ implicit = false certificate = "default" [server.fail2ban] -authentication = "101/5s" +authentication = "100/5s" [authentication] rate-limit = "100/2s" @@ -371,7 +371,7 @@ pub async fn jmap_tests() { .await; webhooks::test(&mut params).await; - /*email_query::test(&mut params, delete).await; + email_query::test(&mut params, delete).await; email_get::test(&mut params).await; email_set::test(&mut params).await; email_parse::test(&mut params).await; @@ -381,7 +381,7 @@ pub async fn jmap_tests() { email_copy::test(&mut params).await; thread_get::test(&mut params).await; thread_merge::test(&mut params).await; - mailbox::test(&mut params).await;*/ + mailbox::test(&mut params).await; delivery::test(&mut params).await; auth_acl::test(&mut params).await; auth_limits::test(&mut params).await;