From bc04860afbfe00ad6a4d9f977897b79ac8df37f6 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 31 Aug 2023 16:30:08 +0200 Subject: [PATCH] Make the access tokens TTL configurable --- crates/cli/src/commands/server.rs | 10 ++- .../config/src/sections/{csrf.rs => hack.rs} | 63 +++++++------------ crates/config/src/sections/mod.rs | 26 ++++---- crates/handlers/src/app_state.rs | 12 +++- crates/handlers/src/compat/login.rs | 6 +- crates/handlers/src/compat/refresh.rs | 7 ++- crates/handlers/src/lib.rs | 11 +++- crates/handlers/src/oauth2/token.rs | 12 ++-- crates/handlers/src/site_config.rs | 31 +++++++++ crates/handlers/src/test_utils.rs | 11 ++++ docs/config.schema.json | 59 +++++++++-------- 11 files changed, 155 insertions(+), 93 deletions(-) rename crates/config/src/sections/{csrf.rs => hack.rs} (52%) create mode 100644 crates/handlers/src/site_config.rs diff --git a/crates/cli/src/commands/server.rs b/crates/cli/src/commands/server.rs index b0014252..3fd45277 100644 --- a/crates/cli/src/commands/server.rs +++ b/crates/cli/src/commands/server.rs @@ -18,7 +18,9 @@ use anyhow::Context; use clap::Parser; use itertools::Itertools; use mas_config::AppConfig; -use mas_handlers::{AppState, CookieManager, HttpClientFactory, MatrixHomeserver, MetadataCache}; +use mas_handlers::{ + AppState, CookieManager, HttpClientFactory, MatrixHomeserver, MetadataCache, SiteConfig, +}; use mas_listener::{server::Server, shutdown::ShutdownStream}; use mas_matrix_synapse::SynapseConnection; use mas_router::UrlBuilder; @@ -137,6 +139,11 @@ impl Options { http_client_factory.clone(), ); + let site_config = SiteConfig { + access_token_ttl: config.hack.access_token_ttl, + compat_token_ttl: config.hack.compat_token_ttl, + }; + // Explicitly the config to properly zeroize secret keys drop(config); @@ -159,6 +166,7 @@ impl Options { graphql_schema, http_client_factory, password_manager, + site_config, conn_acquisition_histogram: None, }; s.init_metrics()?; diff --git a/crates/config/src/sections/csrf.rs b/crates/config/src/sections/hack.rs similarity index 52% rename from crates/config/src/sections/csrf.rs rename to crates/config/src/sections/hack.rs index e82df2c4..ae72cda8 100644 --- a/crates/config/src/sections/csrf.rs +++ b/crates/config/src/sections/hack.rs @@ -1,4 +1,4 @@ -// Copyright 2021, 2022 The Matrix.org Foundation C.I.C. +// Copyright 2023 The Matrix.org Foundation C.I.C. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -19,33 +19,42 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use serde_with::serde_as; -use super::ConfigurationSection; +use crate::ConfigurationSection; -fn default_ttl() -> Duration { - Duration::hours(1) +fn default_token_ttl() -> Duration { + Duration::minutes(5) } -/// Configuration related to Cross-Site Request Forgery protections +/// Configuration sections for miscellaneous options #[serde_as] -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct CsrfConfig { - /// Time-to-live of a CSRF token in seconds +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] +pub struct HackConfig { + /// Time-to-live of access tokens in seconds #[schemars(with = "u64", range(min = 60, max = 86400))] - #[serde(default = "default_ttl")] + #[serde(default = "default_token_ttl")] #[serde_as(as = "serde_with::DurationSeconds")] - pub ttl: Duration, + pub access_token_ttl: Duration, + + /// Time-to-live of compatibility access tokens in seconds + #[schemars(with = "u64", range(min = 60, max = 86400))] + #[serde(default = "default_token_ttl")] + #[serde_as(as = "serde_with::DurationSeconds")] + pub compat_token_ttl: Duration, } -impl Default for CsrfConfig { +impl Default for HackConfig { fn default() -> Self { - Self { ttl: default_ttl() } + Self { + access_token_ttl: default_token_ttl(), + compat_token_ttl: default_token_ttl(), + } } } #[async_trait] -impl ConfigurationSection for CsrfConfig { +impl ConfigurationSection for HackConfig { fn path() -> &'static str { - "csrf" + "hack" } async fn generate(_rng: R) -> anyhow::Result @@ -59,29 +68,3 @@ impl ConfigurationSection for CsrfConfig { Self::default() } } - -#[cfg(test)] -mod tests { - use figment::Jail; - - use super::*; - - #[test] - fn load_config() { - Jail::expect_with(|jail| { - jail.create_file( - "config.yaml", - r#" - csrf: - ttl: 1800 - "#, - )?; - - let config = CsrfConfig::load_from_file("config.yaml")?; - - assert_eq!(config.ttl, Duration::minutes(30)); - - Ok(()) - }); - } -} diff --git a/crates/config/src/sections/mod.rs b/crates/config/src/sections/mod.rs index 0f1c506b..6a206ba6 100644 --- a/crates/config/src/sections/mod.rs +++ b/crates/config/src/sections/mod.rs @@ -18,9 +18,9 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; mod clients; -mod csrf; mod database; mod email; +mod hack; mod http; mod matrix; mod passwords; @@ -32,9 +32,9 @@ mod upstream_oauth2; pub use self::{ clients::{ClientAuthMethodConfig, ClientConfig, ClientsConfig}, - csrf::CsrfConfig, database::{ConnectConfig as DatabaseConnectConfig, DatabaseConfig}, email::{EmailConfig, EmailSmtpMode, EmailTransportConfig}, + hack::HackConfig, http::{ BindConfig as HttpBindConfig, HttpConfig, ListenerConfig as HttpListenerConfig, Resource as HttpResource, TlsConfig as HttpTlsConfig, UnixOrTcp, @@ -81,10 +81,6 @@ pub struct RootConfig { #[serde(default)] pub templates: TemplatesConfig, - /// Configuration related to Cross-Site Request Forgery protections - #[serde(default)] - pub csrf: CsrfConfig, - /// Configuration related to sending emails #[serde(default)] pub email: EmailConfig, @@ -106,6 +102,10 @@ pub struct RootConfig { /// Configuration related to upstream OAuth providers #[serde(default)] pub upstream_oauth2: UpstreamOAuth2Config, + + /// Miscellaneous configuration options + #[serde(default)] + pub hack: HackConfig, } #[async_trait] @@ -124,13 +124,13 @@ impl ConfigurationSection for RootConfig { database: DatabaseConfig::generate(&mut rng).await?, telemetry: TelemetryConfig::generate(&mut rng).await?, templates: TemplatesConfig::generate(&mut rng).await?, - csrf: CsrfConfig::generate(&mut rng).await?, email: EmailConfig::generate(&mut rng).await?, passwords: PasswordsConfig::generate(&mut rng).await?, secrets: SecretsConfig::generate(&mut rng).await?, matrix: MatrixConfig::generate(&mut rng).await?, policy: PolicyConfig::generate(&mut rng).await?, upstream_oauth2: UpstreamOAuth2Config::generate(&mut rng).await?, + hack: HackConfig::generate(&mut rng).await?, }) } @@ -142,12 +142,12 @@ impl ConfigurationSection for RootConfig { telemetry: TelemetryConfig::test(), templates: TemplatesConfig::test(), passwords: PasswordsConfig::test(), - csrf: CsrfConfig::test(), email: EmailConfig::test(), secrets: SecretsConfig::test(), matrix: MatrixConfig::test(), policy: PolicyConfig::test(), upstream_oauth2: UpstreamOAuth2Config::test(), + hack: HackConfig::test(), } } } @@ -165,9 +165,6 @@ pub struct AppConfig { #[serde(default)] pub templates: TemplatesConfig, - #[serde(default)] - pub csrf: CsrfConfig, - #[serde(default)] pub email: EmailConfig, @@ -180,6 +177,9 @@ pub struct AppConfig { #[serde(default)] pub policy: PolicyConfig, + + #[serde(default)] + pub hack: HackConfig, } #[async_trait] @@ -196,12 +196,12 @@ impl ConfigurationSection for AppConfig { http: HttpConfig::generate(&mut rng).await?, database: DatabaseConfig::generate(&mut rng).await?, templates: TemplatesConfig::generate(&mut rng).await?, - csrf: CsrfConfig::generate(&mut rng).await?, email: EmailConfig::generate(&mut rng).await?, passwords: PasswordsConfig::generate(&mut rng).await?, secrets: SecretsConfig::generate(&mut rng).await?, matrix: MatrixConfig::generate(&mut rng).await?, policy: PolicyConfig::generate(&mut rng).await?, + hack: HackConfig::generate(&mut rng).await?, }) } @@ -211,11 +211,11 @@ impl ConfigurationSection for AppConfig { database: DatabaseConfig::test(), templates: TemplatesConfig::test(), passwords: PasswordsConfig::test(), - csrf: CsrfConfig::test(), email: EmailConfig::test(), secrets: SecretsConfig::test(), matrix: MatrixConfig::test(), policy: PolicyConfig::test(), + hack: HackConfig::test(), } } } diff --git a/crates/handlers/src/app_state.rs b/crates/handlers/src/app_state.rs index cf031d88..b887af94 100644 --- a/crates/handlers/src/app_state.rs +++ b/crates/handlers/src/app_state.rs @@ -34,7 +34,10 @@ use opentelemetry::{ use rand::SeedableRng; use sqlx::PgPool; -use crate::{passwords::PasswordManager, upstream_oauth2::cache::MetadataCache, MatrixHomeserver}; +use crate::{ + passwords::PasswordManager, site_config::SiteConfig, upstream_oauth2::cache::MetadataCache, + MatrixHomeserver, +}; #[derive(Clone)] pub struct AppState { @@ -50,6 +53,7 @@ pub struct AppState { pub http_client_factory: HttpClientFactory, pub password_manager: PasswordManager, pub metadata_cache: MetadataCache, + pub site_config: SiteConfig, pub conn_acquisition_histogram: Option>, } @@ -199,6 +203,12 @@ impl FromRef for MetadataCache { } } +impl FromRef for SiteConfig { + fn from_ref(input: &AppState) -> Self { + input.site_config.clone() + } +} + #[async_trait] impl FromRequestParts for BoxClock { type Rejection = Infallible; diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index cbf365ab..bdf380af 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -32,7 +32,7 @@ use thiserror::Error; use zeroize::Zeroizing; use super::{MatrixError, MatrixHomeserver}; -use crate::{impl_from_error_for_route, passwords::PasswordManager}; +use crate::{impl_from_error_for_route, passwords::PasswordManager, site_config::SiteConfig}; #[derive(Debug, Serialize)] #[serde(tag = "type")] @@ -210,6 +210,7 @@ pub(crate) async fn post( State(password_manager): State, mut repo: BoxRepository, State(homeserver): State, + State(site_config): State, Json(input): Json, ) -> Result { let (session, user) = match (password_manager.is_enabled(), input.credentials) { @@ -242,8 +243,7 @@ pub(crate) async fn post( // If the client asked for a refreshable token, make it expire let expires_in = if input.refresh_token { - // TODO: this should be configurable - Some(Duration::minutes(5)) + Some(site_config.compat_token_ttl) } else { None }; diff --git a/crates/handlers/src/compat/refresh.rs b/crates/handlers/src/compat/refresh.rs index a0ac4f63..3bf06c83 100644 --- a/crates/handlers/src/compat/refresh.rs +++ b/crates/handlers/src/compat/refresh.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use axum::{response::IntoResponse, Json}; +use axum::{extract::State, response::IntoResponse, Json}; use chrono::Duration; use hyper::StatusCode; use mas_data_model::{TokenFormatError, TokenType}; @@ -25,7 +25,7 @@ use serde_with::{serde_as, DurationMilliSeconds}; use thiserror::Error; use super::MatrixError; -use crate::impl_from_error_for_route; +use crate::{impl_from_error_for_route, site_config::SiteConfig}; #[derive(Debug, Deserialize)] pub struct RequestBody { @@ -91,6 +91,7 @@ pub(crate) async fn post( mut rng: BoxRng, clock: BoxClock, mut repo: BoxRepository, + State(site_config): State, Json(input): Json, ) -> Result { let token_type = TokenType::check(&input.refresh_token)?; @@ -128,7 +129,7 @@ pub(crate) async fn post( let new_refresh_token_str = TokenType::CompatRefreshToken.generate(&mut rng); let new_access_token_str = TokenType::CompatAccessToken.generate(&mut rng); - let expires_in = Duration::minutes(5); + let expires_in = site_config.compat_token_ttl; let new_access_token = repo .compat_access_token() .add( diff --git a/crates/handlers/src/lib.rs b/crates/handlers/src/lib.rs index cc6554eb..2857b493 100644 --- a/crates/handlers/src/lib.rs +++ b/crates/handlers/src/lib.rs @@ -68,6 +68,7 @@ pub mod passwords; pub mod upstream_oauth2; mod views; +mod site_config; #[cfg(test)] mod test_utils; @@ -89,8 +90,10 @@ macro_rules! impl_from_error_for_route { pub use mas_axum_utils::{cookies::CookieManager, http_client_factory::HttpClientFactory}; -pub use self::{app_state::AppState, compat::MatrixHomeserver, graphql::schema as graphql_schema}; -pub use crate::upstream_oauth2::cache::MetadataCache; +pub use self::{ + app_state::AppState, compat::MatrixHomeserver, graphql::schema as graphql_schema, + site_config::SiteConfig, upstream_oauth2::cache::MetadataCache, +}; pub fn healthcheck_router() -> Router where @@ -169,6 +172,7 @@ where BoxRepository: FromRequestParts, Encrypter: FromRef, HttpClientFactory: FromRef, + SiteConfig: FromRef, BoxClock: FromRequestParts, BoxRng: FromRequestParts, Policy: FromRequestParts, @@ -225,9 +229,10 @@ where ::Error: std::error::Error + Send + Sync, S: Clone + Send + Sync + 'static, UrlBuilder: FromRef, - BoxRepository: FromRequestParts, + SiteConfig: FromRef, MatrixHomeserver: FromRef, PasswordManager: FromRef, + BoxRepository: FromRequestParts, BoxClock: FromRequestParts, BoxRng: FromRequestParts, { diff --git a/crates/handlers/src/oauth2/token.rs b/crates/handlers/src/oauth2/token.rs index 837840b3..a5fa5e4b 100644 --- a/crates/handlers/src/oauth2/token.rs +++ b/crates/handlers/src/oauth2/token.rs @@ -47,7 +47,7 @@ use tracing::debug; use url::Url; use super::{generate_id_token, generate_token_pair}; -use crate::impl_from_error_for_route; +use crate::{impl_from_error_for_route, site_config::SiteConfig}; #[serde_as] #[skip_serializing_none] @@ -161,6 +161,7 @@ pub(crate) async fn post( State(key_store): State, State(url_builder): State, mut repo: BoxRepository, + State(site_config): State, State(encrypter): State, client_authorization: ClientAuthorization, ) -> Result { @@ -191,12 +192,13 @@ pub(crate) async fn post( &client, &key_store, &url_builder, + &site_config, repo, ) .await? } AccessTokenRequest::RefreshToken(grant) => { - refresh_token_grant(&mut rng, &clock, &grant, &client, repo).await? + refresh_token_grant(&mut rng, &clock, &grant, &client, &site_config, repo).await? } _ => { return Err(RouteError::UnsupportedGrantType); @@ -220,6 +222,7 @@ async fn authorization_code_grant( client: &Client, key_store: &Keystore, url_builder: &UrlBuilder, + site_config: &SiteConfig, mut repo: BoxRepository, ) -> Result<(AccessTokenResponse, BoxRepository), RouteError> { let authz_grant = repo @@ -312,7 +315,7 @@ async fn authorization_code_grant( .get_last_authentication(&browser_session) .await?; - let ttl = Duration::minutes(5); + let ttl = site_config.access_token_ttl; let (access_token, refresh_token) = generate_token_pair(&mut rng, clock, &mut repo, &session, ttl).await?; @@ -367,6 +370,7 @@ async fn refresh_token_grant( clock: &impl Clock, grant: &RefreshTokenGrant, client: &Client, + site_config: &SiteConfig, mut repo: BoxRepository, ) -> Result<(AccessTokenResponse, BoxRepository), RouteError> { let refresh_token = repo @@ -390,7 +394,7 @@ async fn refresh_token_grant( return Err(RouteError::InvalidGrant); } - let ttl = Duration::minutes(5); + let ttl = site_config.access_token_ttl; let (new_access_token, new_refresh_token) = generate_token_pair(rng, clock, &mut repo, &session, ttl).await?; diff --git a/crates/handlers/src/site_config.rs b/crates/handlers/src/site_config.rs new file mode 100644 index 00000000..d24f7ea1 --- /dev/null +++ b/crates/handlers/src/site_config.rs @@ -0,0 +1,31 @@ +// Copyright 2023 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use chrono::Duration; + +/// Random site configuration we don't now where to put yet. +#[derive(Debug, Clone)] +pub struct SiteConfig { + pub access_token_ttl: Duration, + pub compat_token_ttl: Duration, +} + +impl Default for SiteConfig { + fn default() -> Self { + Self { + access_token_ttl: Duration::minutes(5), + compat_token_ttl: Duration::minutes(5), + } + } +} diff --git a/crates/handlers/src/test_utils.rs b/crates/handlers/src/test_utils.rs index 6ed765e1..844913e4 100644 --- a/crates/handlers/src/test_utils.rs +++ b/crates/handlers/src/test_utils.rs @@ -48,6 +48,7 @@ use url::Url; use crate::{ app_state::ErrorWrapper, passwords::{Hasher, PasswordManager}, + site_config::SiteConfig, upstream_oauth2::cache::MetadataCache, MatrixHomeserver, }; @@ -76,6 +77,7 @@ pub(crate) struct TestState { pub graphql_schema: mas_graphql::Schema, pub http_client_factory: HttpClientFactory, pub password_manager: PasswordManager, + pub site_config: SiteConfig, pub clock: Arc, pub rng: Arc>, } @@ -133,6 +135,8 @@ impl TestState { let http_client_factory = HttpClientFactory::new(10); + let site_config = SiteConfig::default(); + let clock = Arc::new(MockClock::default()); let rng = Arc::new(Mutex::new(ChaChaRng::seed_from_u64(42))); @@ -160,6 +164,7 @@ impl TestState { graphql_schema, http_client_factory, password_manager, + site_config, clock, rng, }) @@ -346,6 +351,12 @@ impl FromRef for MetadataCache { } } +impl FromRef for SiteConfig { + fn from_ref(input: &TestState) -> Self { + input.site_config.clone() + } +} + #[async_trait] impl FromRequestParts for BoxClock { type Rejection = Infallible; diff --git a/docs/config.schema.json b/docs/config.schema.json index afad2de4..aee13b1b 100644 --- a/docs/config.schema.json +++ b/docs/config.schema.json @@ -16,17 +16,6 @@ "$ref": "#/definitions/ClientConfig" } }, - "csrf": { - "description": "Configuration related to Cross-Site Request Forgery protections", - "default": { - "ttl": 3600 - }, - "allOf": [ - { - "$ref": "#/definitions/CsrfConfig" - } - ] - }, "database": { "description": "Database connection configuration", "default": { @@ -56,6 +45,18 @@ } ] }, + "hack": { + "description": "Miscellaneous configuration options", + "default": { + "access_token_ttl": 300, + "compat_token_ttl": 300 + }, + "allOf": [ + { + "$ref": "#/definitions/HackConfig" + } + ] + }, "http": { "description": "Configuration of the HTTP server", "default": { @@ -464,20 +465,6 @@ } } }, - "CsrfConfig": { - "description": "Configuration related to Cross-Site Request Forgery protections", - "type": "object", - "properties": { - "ttl": { - "description": "Time-to-live of a CSRF token in seconds", - "default": 3600, - "type": "integer", - "format": "uint64", - "maximum": 86400.0, - "minimum": 60.0 - } - } - }, "DatabaseConfig": { "description": "Database connection configuration", "type": "object", @@ -737,6 +724,28 @@ } ] }, + "HackConfig": { + "description": "Configuration sections for miscellaneous options", + "type": "object", + "properties": { + "access_token_ttl": { + "description": "Time-to-live of access tokens in seconds", + "default": 300, + "type": "integer", + "format": "uint64", + "maximum": 86400.0, + "minimum": 60.0 + }, + "compat_token_ttl": { + "description": "Time-to-live of compatibility access tokens in seconds", + "default": 300, + "type": "integer", + "format": "uint64", + "maximum": 86400.0, + "minimum": 60.0 + } + } + }, "HashingScheme": { "description": "A hashing algorithm", "type": "object",