diff --git a/CHANGELOG.md b/CHANGELOG.md index 52e207f43..4da119e6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,37 @@ # Changelog +## UNRELEASED + +### Changes + +#### Upstream Identity Providers + +To provide additional compatibility for some upstream providers like Active Directory Federation Severices, +some changes have been applied to Rauthy's behavior. + +The first thing is that the HTTP client used for upstream Logins does not force TLS v1.3 anymore, but also allows +TLS v1.2. Both v1.2 and v1.3 are considered being secure by current standards. This is necessary, because some OS'es +like Windows Server 2019 do not support TLS 1.3. + +The second change is for the way upstream providers are configured. The behavior until now was, that Rauthy added the +client credentials as both Basic Authentication in headers, and in the body for maximum compatibility. However, some +IdP'S (like ADFS for instance) complain about this and only expect it in one place. +To make this happen, there are 2 new fields for the upstream IdP configuration: + +- `client_secret_basic: bool` +- `client_secret_post: bool` + +These are available as switches in the Admin UI for each upstream provider. To not introduce breaking changes, all +possibly existing configurations will have both options enabled like it has been up until now. + +[#659](https://github.com/sebadob/rauthy/pull/659) + +#### Note + +Even though this changes the request and response objects on the API, this change is **NOT** being handled as +a breaking change. API clients are forbidden to modify upstream IdPs for security reasons, which means this change +should only affect the Rauthy Admin UI. + ## v0.27.2 ### Changes diff --git a/Cargo.lock b/Cargo.lock index d8b5ebe9b..bc9c1fdd4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4272,7 +4272,7 @@ dependencies = [ [[package]] name = "rauthy" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-service", "actix-web", @@ -4313,7 +4313,7 @@ dependencies = [ [[package]] name = "rauthy-api-types" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-web", "chrono", @@ -4334,7 +4334,7 @@ dependencies = [ [[package]] name = "rauthy-common" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-web", "argon2", @@ -4360,7 +4360,7 @@ dependencies = [ [[package]] name = "rauthy-error" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-multipart", "actix-web", @@ -4396,7 +4396,7 @@ dependencies = [ [[package]] name = "rauthy-handlers" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-multipart", "actix-web", @@ -4426,7 +4426,7 @@ dependencies = [ [[package]] name = "rauthy-middlewares" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-web", "chrono", @@ -4445,7 +4445,7 @@ dependencies = [ [[package]] name = "rauthy-models" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "accept-language", "actix", @@ -4521,7 +4521,7 @@ dependencies = [ [[package]] name = "rauthy-notify" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "async-trait", "chrono", @@ -4541,7 +4541,7 @@ dependencies = [ [[package]] name = "rauthy-schedulers" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-web", "chrono", @@ -4559,7 +4559,7 @@ dependencies = [ [[package]] name = "rauthy-service" -version = "0.27.2" +version = "0.27.3-20241218-1" dependencies = [ "actix-web", "argon2", diff --git a/Cargo.toml b/Cargo.toml index cca776ce8..ee5609d49 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ members = ["src/*"] exclude = ["rauthy-client"] [workspace.package] -version = "0.27.2" +version = "0.27.3-20241218-1" edition = "2021" authors = ["Sebastian Dobe "] license = "Apache-2.0" diff --git a/frontend/src/components/admin/providers/JsonPathDesc.svelte b/frontend/src/components/admin/providers/JsonPathDesc.svelte index dbb375904..615600493 100644 --- a/frontend/src/components/admin/providers/JsonPathDesc.svelte +++ b/frontend/src/components/admin/providers/JsonPathDesc.svelte @@ -1,7 +1,7 @@
-

ID token claim mappings

+

ID token claim mappings

Values from the ID token after a successful upstream login can be mapped automatically.

@@ -25,6 +25,10 @@
diff --git a/migrations/hiqlite/3_client_auth_methods.sql b/migrations/hiqlite/3_client_auth_methods.sql new file mode 100644 index 000000000..37988ccad --- /dev/null +++ b/migrations/hiqlite/3_client_auth_methods.sql @@ -0,0 +1,4 @@ +ALTER TABLE auth_providers + ADD client_secret_basic INTEGER DEFAULT 1 NOT NULL; +ALTER TABLE auth_providers + ADD client_secret_post INTEGER DEFAULT 1 NOT NULL; \ No newline at end of file diff --git a/migrations/postgres/29_client_auth_methods.sql b/migrations/postgres/29_client_auth_methods.sql new file mode 100644 index 000000000..419daaabe --- /dev/null +++ b/migrations/postgres/29_client_auth_methods.sql @@ -0,0 +1,4 @@ +ALTER TABLE auth_providers + ADD client_secret_basic BOOLEAN DEFAULT true NOT NULL; +ALTER TABLE auth_providers + ADD client_secret_post BOOLEAN DEFAULT true NOT NULL; \ No newline at end of file diff --git a/src/api/src/auth_providers.rs b/src/api/src/auth_providers.rs index aa1f257e3..6b89deb7c 100644 --- a/src/api/src/auth_providers.rs +++ b/src/api/src/auth_providers.rs @@ -264,7 +264,15 @@ pub async fn put_provider( if !payload.use_pkce && payload.client_secret.is_none() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - "Must at least be a confidential client or use PKCE".to_string(), + "Must at least be a confidential client or use PKCE", + )); + } + if payload.client_secret.is_some() + && !(payload.client_secret_basic || payload.client_secret_post) + { + return Err(ErrorResponse::new( + ErrorResponseType::BadRequest, + "A confidential client must have a least one of 'client_secret_basic | client_secret_post'", )); } diff --git a/src/api_types/src/auth_providers.rs b/src/api_types/src/auth_providers.rs index 698b4c4db..a703f5a06 100644 --- a/src/api_types/src/auth_providers.rs +++ b/src/api_types/src/auth_providers.rs @@ -39,6 +39,8 @@ pub struct ProviderRequest { pub danger_allow_insecure: Option, pub use_pkce: bool, + pub client_secret_basic: bool, + pub client_secret_post: bool, // This validation is pretty loose, but if we make it too strict, // we will most probably get into compatibility issues. @@ -164,6 +166,8 @@ pub struct ProviderResponse { pub danger_allow_insecure: bool, pub use_pkce: bool, + pub client_secret_basic: bool, + pub client_secret_post: bool, pub root_pem: Option, } @@ -183,5 +187,7 @@ pub struct ProviderLookupResponse<'a> { pub scope: String, pub root_pem: &'a Option, pub use_pkce: bool, + pub client_secret_basic: bool, + pub client_secret_post: bool, pub danger_allow_insecure: bool, } diff --git a/src/models/src/entity/auth_providers.rs b/src/models/src/entity/auth_providers.rs index 636296c00..1aa6ba698 100644 --- a/src/models/src/entity/auth_providers.rs +++ b/src/models/src/entity/auth_providers.rs @@ -133,6 +133,7 @@ pub struct WellKnownLookup { pub userinfo_endpoint: String, pub jwks_uri: String, pub scopes_supported: Vec, + pub token_endpoint_auth_methods_supported: Vec, #[serde(default)] pub code_challenge_methods_supported: Vec, } @@ -190,8 +191,9 @@ pub struct AuthProvider { pub allow_insecure_requests: bool, pub use_pkce: bool, - pub root_pem: Option, + pub client_secret_basic: bool, + pub client_secret_post: bool, } impl<'r> From> for AuthProvider { @@ -218,6 +220,8 @@ impl<'r> From> for AuthProvider { allow_insecure_requests: row.get("allow_insecure_requests"), use_pkce: row.get("use_pkce"), root_pem: row.get("root_pem"), + client_secret_basic: row.get("client_secret_basic"), + client_secret_post: row.get("client_secret_post"), } } } @@ -234,9 +238,10 @@ impl AuthProvider { INSERT INTO auth_providers (id, name, enabled, typ, issuer, authorization_endpoint, token_endpoint, userinfo_endpoint, client_id, secret, scope, admin_claim_path, admin_claim_value, -mfa_claim_path, mfa_claim_value, allow_insecure_requests, use_pkce, root_pem) +mfa_claim_path, mfa_claim_value, allow_insecure_requests, use_pkce, root_pem, client_secret_basic, +client_secret_post) VALUES -($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18) +($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20) RETURNING *"#, params!( slf.id, @@ -256,7 +261,9 @@ RETURNING *"#, slf.mfa_claim_value, slf.allow_insecure_requests, slf.use_pkce, - slf.root_pem + slf.root_pem, + slf.client_secret_basic, + slf.client_secret_post ), ) .await? @@ -266,9 +273,10 @@ RETURNING *"#, INSERT INTO auth_providers (id, name, enabled, typ, issuer, authorization_endpoint, token_endpoint, userinfo_endpoint, client_id, secret, scope, admin_claim_path, admin_claim_value, -mfa_claim_path, mfa_claim_value, allow_insecure_requests, use_pkce, root_pem) +mfa_claim_path, mfa_claim_value, allow_insecure_requests, use_pkce, root_pem, client_secret_basic, +client_secret_post) VALUES -($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18)"#, +($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20)"#, slf.id, slf.name, slf.enabled, @@ -287,6 +295,8 @@ VALUES slf.allow_insecure_requests, slf.use_pkce, slf.root_pem, + slf.client_secret_basic, + slf.client_secret_post ) .execute(DB::conn()) .await?; @@ -405,8 +415,9 @@ UPDATE auth_providers SET name = $1, enabled = $2, issuer = $3, typ = $4, authorization_endpoint = $5, token_endpoint = $6, userinfo_endpoint = $7, client_id = $8, secret = $9, scope = $10, admin_claim_path = $11, admin_claim_value = $12, mfa_claim_path = $13, mfa_claim_value = $14, -allow_insecure_requests = $15, use_pkce = $16, root_pem = $17 -WHERE id = $18"#, +allow_insecure_requests = $15, use_pkce = $16, root_pem = $17, client_secret_basic = $18, +client_secret_post = $19 +WHERE id = $20"#, params!( self.name.clone(), self.enabled, @@ -425,6 +436,8 @@ WHERE id = $18"#, self.allow_insecure_requests, self.use_pkce, self.root_pem.clone(), + self.client_secret_basic, + self.client_secret_post, self.id.clone() ), ) @@ -436,8 +449,9 @@ UPDATE auth_providers SET name = $1, enabled = $2, issuer = $3, typ = $4, authorization_endpoint = $5, token_endpoint = $6, userinfo_endpoint = $7, client_id = $8, secret = $9, scope = $10, admin_claim_path = $11, admin_claim_value = $12, mfa_claim_path = $13, mfa_claim_value = $14, -allow_insecure_requests = $15, use_pkce = $16, root_pem = $17 -WHERE id = $18"#, +allow_insecure_requests = $15, use_pkce = $16, root_pem = $17, client_secret_basic = $18, +client_secret_post = $19 +WHERE id = $20"#, self.name, self.enabled, self.issuer, @@ -455,6 +469,8 @@ WHERE id = $18"#, self.allow_insecure_requests, self.use_pkce, self.root_pem, + self.client_secret_basic, + self.client_secret_post, self.id, ) .execute(DB::conn()) @@ -503,7 +519,7 @@ impl AuthProvider { .timeout(Duration::from_secs(10)) .connect_timeout(Duration::from_secs(10)) .tcp_keepalive(Duration::from_secs(30)) - .min_tls_version(tls::Version::TLS_1_3) + .min_tls_version(tls::Version::TLS_1_2) .tls_built_in_root_certs(true) .user_agent(format!("Rauthy Auth Provider Client v{}", RAUTHY_VERSION)) .https_only(true); @@ -543,6 +559,8 @@ impl AuthProvider { allow_insecure_requests: req.danger_allow_insecure.unwrap_or(false), use_pkce: req.use_pkce, root_pem: req.root_pem, + client_secret_basic: req.client_secret_basic, + client_secret_post: req.client_secret_post, }) } @@ -630,6 +648,15 @@ impl AuthProvider { scope.push_str("email "); } + let client_secret_basic = well_known + .token_endpoint_auth_methods_supported + .iter() + .any(|m| m.as_str() == "client_secret_basic"); + let client_secret_post = well_known + .token_endpoint_auth_methods_supported + .iter() + .any(|m| m.as_str() == "client_secret_post"); + Ok(ProviderLookupResponse { issuer: well_known.issuer, // TODO optimization (and possibly security enhancement): strip issuer url from all of these? @@ -642,6 +669,8 @@ impl AuthProvider { .code_challenge_methods_supported .iter() .any(|c| c == "S256"), + client_secret_basic, + client_secret_post, danger_allow_insecure: payload.danger_allow_insecure.unwrap_or(false), scope, // TODO add `scopes_supported` Vec and make them selectable with checkboxes in the UI @@ -693,6 +722,8 @@ impl TryFrom for ProviderResponse { mfa_claim_value: value.mfa_claim_value, danger_allow_insecure: value.allow_insecure_requests, use_pkce: value.use_pkce, + client_secret_basic: value.client_secret_basic, + client_secret_post: value.client_secret_post, root_pem: value.root_pem, }) } @@ -881,24 +912,36 @@ impl AuthProviderCallback { provider.allow_insecure_requests, provider.root_pem.as_deref(), )?; - let payload = OidcCodeRequestParams { + let mut payload = OidcCodeRequestParams { + // a client MAY add the `client_id`, but it MUST add it when it's public client_id: &provider.client_id, - client_secret: AuthProvider::get_secret_cleartext(&provider.secret)?, + client_secret: None, code: &payload.code, code_verifier: provider.use_pkce.then_some(&payload.pkce_verifier), grant_type: "authorization_code", redirect_uri: &PROVIDER_CALLBACK_URI, }; - let res = client - .post(&provider.token_endpoint) - .header(ACCEPT, APPLICATION_JSON) - .basic_auth( - &provider.client_id, - AuthProvider::get_secret_cleartext(&provider.secret)?, - ) - .form(&payload) - .send() - .await?; + if provider.client_secret_post { + payload.client_secret = AuthProvider::get_secret_cleartext(&provider.secret)?; + } + + let res = { + let mut builder = client + .post(&provider.token_endpoint) + .header(ACCEPT, APPLICATION_JSON); + + if provider.client_secret_basic { + builder = builder.basic_auth( + &provider.client_id, + AuthProvider::get_secret_cleartext(&provider.secret)?, + ) + } + + builder + } + .form(&payload) + .send() + .await?; let status = res.status().as_u16(); debug!("POST /token auth provider status: {}", status);