diff --git a/Makefile b/Makefile index 533954d9f..fb411f535 100644 --- a/Makefile +++ b/Makefile @@ -178,11 +178,9 @@ codespell: --skip='*.svg' \ --skip='*.br' \ --skip='./rlm_python/mods-available/eap' \ - --skip='./server/web_ui/static/external' \ - --skip='./server/web_ui/pkg/external' \ - --skip='./server/web_ui/shared/static/external' \ - --skip='./server/web_ui/admin/static/external,./server/web_ui/admin/pkg/external' \ - --skip='./server/lib/src/constants/system_config.rs,./pykanidm/site,./server/lib/src/constants/*.json' + --skip='./server/lib/src/constants/system_config.rs' + --skip='./pykanidm/site' \ + --skip='./server/lib/src/constants/*.json' .PHONY: test/pykanidm/pytest test/pykanidm/pytest: ## python library testing diff --git a/book/src/integrations/oauth2.md b/book/src/integrations/oauth2.md index 34bf202af..7bec976fe 100644 --- a/book/src/integrations/oauth2.md +++ b/book/src/integrations/oauth2.md @@ -45,6 +45,7 @@ introspection. Kanidm will expose its OAuth2 APIs at the following URLs, substituting `:client_id:` with an OAuth2 client ID. +
@@ -59,7 +60,7 @@ URL **(recommended)** `https://idm.example.com/oauth2/openid/:client_id:/.well-known/openid-configuration` This document includes all the URLs and attributes an app needs to be able to -authenticate using OIDC with Kanidm, *except* for the `client_id` and +authenticate using OIDC with Kanidm, _except_ for the `client_id` and `client_secret`. Use this document wherever possible, as it will allow you to easily build and/or @@ -183,6 +184,8 @@ Token signing public key
+ + ## Configuration ### Create the Kanidm Configuration @@ -220,12 +223,12 @@ kanidm system oauth2 update-scope-map nextcloud nextcloud_users email profile op > claims may be added to the authorisation token. It is not guaranteed that all of the associated > claims will be added. > -> - **profile** - name, family_name, given_name, middle_name, nickname, preferred_username, profile, +> * **profile** - name, family_name, given_name, middle_name, nickname, preferred_username, profile, > picture, website, gender, birthdate, zoneinfo, locale, and updated_at -> - **email** - email, email_verified -> - **address** - address -> - **phone** - phone_number, phone_number_verified -> - **groups** - groups +> * **email** - email, email_verified +> * **address** - address +> * **phone** - phone_number, phone_number_verified +> * **groups** - groups @@ -233,6 +236,7 @@ kanidm system oauth2 update-scope-map nextcloud nextcloud_users email profile op > > If you are creating an OpenID Connect (OIDC) client you **MUST** provide a scope map containing > `openid`. Without this, OpenID Connect clients **WILL NOT WORK**! +> > ```bash > kanidm system oauth2 update-scope-map nextcloud nextcloud_users openid > ``` @@ -315,10 +319,7 @@ applications that act as the OAuth2 client and its corresponding webserver is th In this case, the SPA is unable to act as a confidential client since the basic secret would need to be embedded in every client. -Another common example is native applications that use a redirect to localhost. These can't have a -client secret embedded, so must act as public clients. - -Public clients for this reason require PKCE to bind a specific browser session to its OAuth2 +For this reason, public clients require PKCE to bind a specific browser session to its OAuth2 exchange. PKCE can not be disabled for public clients for this reason. To create an OAuth2 public client: @@ -328,7 +329,13 @@ kanidm system oauth2 create-public kanidm system oauth2 create-public mywebapp "My Web App" https://webapp.example.com ``` -To allow localhost redirection +## Native Applications + +Some applications will run a local web server on the user's device which directs users to the IDP for +authentication, then back to the local server. [BCP212](https://www.rfc-editor.org/info/bcp212) +"OAuth 2.0 for Native Apps" specifies the rules for this. + +First allow localhost redirects: ```bash kanidm system oauth2 enable-localhost-redirects @@ -336,6 +343,10 @@ kanidm system oauth2 disable-localhost-redirects kanidm system oauth2 enable-localhost-redirects mywebapp ``` +> [!WARNING] +> +> Kanidm only allows these to be enabled on public clients where PKCE is enforced. + ## Alternate Redirect URLs > [!WARNING] @@ -378,8 +389,10 @@ To indicate your readiness for this transition, all OAuth2 clients must have the `strict-redirect-url` enabled. Once enabled, the client will begin to enforce the 1.4.0 strict validation behaviour. -If you have not enabled `strict-redirect-url` on all OAuth2 clients the upgrade to 1.4.0 will refuse -to proceed. +> [!WARNING] +> +> If you have not enabled `strict-redirect-url` on all OAuth2 clients the upgrade to 1.4.0 will refuse +> to proceed. To enable or disable strict validation: diff --git a/build_rs_cov.profraw b/build_rs_cov.profraw new file mode 100644 index 000000000..75521227e Binary files /dev/null and b/build_rs_cov.profraw differ diff --git a/libs/client/src/oauth.rs b/libs/client/src/oauth.rs index 4603ed952..9b4ce756d 100644 --- a/libs/client/src/oauth.rs +++ b/libs/client/src/oauth.rs @@ -434,6 +434,8 @@ impl KanidmClient { id: &str, origin: &Url, ) -> Result<(), ClientError> { + // TODO: should we normalise loopback origins, so when a user specifies `http://localhost/foo` we store it as `http://[::1]/foo`? + let url_to_add = &[origin.as_str()]; self.perform_post_request( format!("/v1/oauth2/{}/_attr/{}", id, ATTR_OAUTH2_RS_ORIGIN).as_str(), diff --git a/server/lib/src/idm/oauth2.rs b/server/lib/src/idm/oauth2.rs index 4100d1331..c7eac6343 100644 --- a/server/lib/src/idm/oauth2.rs +++ b/server/lib/src/idm/oauth2.rs @@ -46,7 +46,7 @@ use serde_with::{formats, serde_as}; use time::OffsetDateTime; use tracing::trace; use uri::{OAUTH2_TOKEN_INTROSPECT_ENDPOINT, OAUTH2_TOKEN_REVOKE_ENDPOINT}; -use url::{Origin, Url}; +use url::{Host, Origin, Url}; use crate::idm::account::Account; use crate::idm::server::{ @@ -251,9 +251,21 @@ enum OauthRSType { }, } +impl OauthRSType { + /// We only allow localhost redirects if PKCE is enabled/required + fn allow_localhost_redirect(&self) -> bool { + match self { + OauthRSType::Basic { .. } => false, + OauthRSType::Public { + allow_localhost_redirect, + } => *allow_localhost_redirect, + } + } +} + impl std::fmt::Debug for OauthRSType { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - let mut ds = f.debug_struct("Oauth2RSType"); + let mut ds = f.debug_struct("OauthRSType"); match self { OauthRSType::Basic { enable_pkce, .. } => { ds.field("type", &"basic").field("pkce", enable_pkce) @@ -1819,67 +1831,51 @@ impl<'a> IdmServerProxyReadTransaction<'a> { Oauth2Error::InvalidClientId })?; - let allow_localhost_redirect = match &o2rs.type_ { - OauthRSType::Basic { .. } => false, - OauthRSType::Public { - allow_localhost_redirect, - } => *allow_localhost_redirect, - }; + // redirect_uri must be part of the client_id origins, unless the client is public and then it MAY + // be a loopback address exempting it from this check and enforcement and we can carry on safely. + if o2rs.type_.allow_localhost_redirect() && check_is_loopback(&auth_req.redirect_uri) { + debug!("Loopback redirect_uri detected, allowing for localhost"); + } else { + // The legacy origin match is in use. + let origin_uri_matched = + !o2rs.strict_redirect_uri && o2rs.origins.contains(&auth_req.redirect_uri.origin()); + // Strict uri validation is in use. + let strict_redirect_uri_matched = + o2rs.strict_redirect_uri && o2rs.redirect_uris.contains(&auth_req.redirect_uri); + // Allow opaque origins such as app uris. + let opaque_origin_matched = o2rs.opaque_origins.contains(&auth_req.redirect_uri); - let localhost_redirect = auth_req - .redirect_uri - .domain() - .map(|domain| domain == "localhost") - .unwrap_or_default(); - - // Strict uri validation is in use. - let strict_redirect_uri_matched = - o2rs.strict_redirect_uri && o2rs.redirect_uris.contains(&auth_req.redirect_uri); - // The legacy origin match is in use. - let origin_uri_matched = - !o2rs.strict_redirect_uri && o2rs.origins.contains(&auth_req.redirect_uri.origin()); - // Allow opaque origins such as app uris. - let opaque_origin_matched = o2rs.opaque_origins.contains(&auth_req.redirect_uri); - // redirect_uri must be part of the client_id origin, unless the client is public and then it MAY - // be localhost exempting it from this check and enforcement. - let localhost_redirect_matched = allow_localhost_redirect && localhost_redirect; - - // At least one of these conditions must hold true to proceed. - if !(strict_redirect_uri_matched - || origin_uri_matched - || opaque_origin_matched - || localhost_redirect_matched) - { - if o2rs.strict_redirect_uri { - warn!( - "Invalid OAuth2 redirect_uri (must be an exact match to a redirect-url) - got {}", - auth_req.redirect_uri.as_str() - ); - } else { - warn!( - "Invalid OAuth2 redirect_uri (must be related to origin) - got {:?}", - auth_req.redirect_uri.origin() - ); + // At least one of these conditions must hold true to proceed. + if !(strict_redirect_uri_matched || origin_uri_matched || opaque_origin_matched) { + if o2rs.strict_redirect_uri { + warn!( + "Invalid OAuth2 redirect_uri (must be an exact match to a redirect-url) - got {}", + auth_req.redirect_uri.as_str() + ); + } else { + warn!( + "Invalid OAuth2 redirect_uri (must be related to origin) - got {:?}", + auth_req.redirect_uri.origin() + ); + } + return Err(Oauth2Error::InvalidOrigin); + } + // We have to specifically match on http here because non-http origins may be exempt from this + // enforcement. + if (o2rs.origin_https_required && auth_req.redirect_uri.scheme() != "https") + && !opaque_origin_matched + { + admin_warn!( + "Invalid OAuth2 redirect_uri scheme (must be https for secure origin) - got {}", + auth_req.redirect_uri.to_string() + ); + return Err(Oauth2Error::InvalidOrigin); } - return Err(Oauth2Error::InvalidOrigin); - } - - // We have to specifically match on http here because non-http origins may be exempt from this - // enforcement. - if !localhost_redirect - && o2rs.origin_https_required - && auth_req.redirect_uri.scheme() == "http" - { - admin_warn!( - "Invalid OAuth2 redirect_uri (must be https for secure origin) - got {:?}", - auth_req.redirect_uri.scheme() - ); - return Err(Oauth2Error::InvalidOrigin); } let code_challenge = if let Some(pkce_request) = &auth_req.pkce_request { if !o2rs.require_pkce() { - security_info!(?o2rs.name, "Insecure rs configuration - pkce is not enforced, but rs is requesting it!"); + security_info!(?o2rs.name, "Insecure OAuth2 client configuration - PKCE is not enforced, but client is requesting it!"); } // CodeChallengeMethod must be S256 if pkce_request.code_challenge_method != CodeChallengeMethod::S256 { @@ -1891,7 +1887,7 @@ impl<'a> IdmServerProxyReadTransaction<'a> { security_error!(?o2rs.name, "No PKCE code challenge was provided with client in enforced PKCE mode."); return Err(Oauth2Error::InvalidRequest); } else { - security_info!(?o2rs.name, "Insecure client configuration - pkce is not enforced."); + security_info!(?o2rs.name, "Insecure client configuration - PKCE is not enforced."); None }; @@ -1928,7 +1924,7 @@ impl<'a> IdmServerProxyReadTransaction<'a> { }; let Some(account_uuid) = ident.get_uuid() else { - error!("consent request ident does not have a valid uuid, unable to proceed"); + error!("Consent request ident does not have a valid UUID, unable to proceed"); return Err(Oauth2Error::InvalidRequest); }; @@ -2888,6 +2884,23 @@ fn parse_user_code(val: &str) -> Result { }) } +/// Check if a host is local (loopback or localhost) +fn host_is_local(host: &Host<&str>) -> bool { + match host { + Host::Ipv4(ip) => ip.is_loopback(), + Host::Ipv6(ip) => ip.is_loopback(), + Host::Domain(domain) => *domain == "localhost", + } +} + +/// Ensure that the redirect URI is a loopback/localhost address +fn check_is_loopback(redirect_uri: &Url) -> bool { + redirect_uri.host().map_or(false, |host| { + // Check if the host is a loopback/localhost address. + host_is_local(&host) + }) +} + #[cfg(test)] mod tests { use base64::{engine::general_purpose, Engine as _}; @@ -2906,7 +2919,7 @@ mod tests { use openssl::sha; use crate::idm::accountpolicy::ResolvedAccountPolicy; - use crate::idm::oauth2::{AuthoriseResponse, Oauth2Error}; + use crate::idm::oauth2::{host_is_local, AuthoriseResponse, Oauth2Error, OauthRSType}; use crate::idm::server::{IdmServer, IdmServerTransaction}; use crate::prelude::*; use crate::value::{AuthType, OauthClaimMapJoin, SessionState}; @@ -6593,12 +6606,16 @@ mod tests { let ct = Duration::from_secs(TEST_CURRENT_TIME); let (_uat, ident, oauth2_rs_uuid) = setup_oauth2_resource_server_public(idms, ct).await; - let mut idms_prox_write = idms.proxy_write(ct).await.unwrap(); + let mut idms_prox_write: crate::idm::server::IdmServerProxyWriteTransaction<'_> = + idms.proxy_write(ct).await.unwrap(); - let modlist = ModifyList::new_list(vec![Modify::Present( - Attribute::OAuth2AllowLocalhostRedirect, - Value::Bool(true), - )]); + let redirect_uri = Url::parse("http://localhost:8765/oauth2/result") + .expect("Failed to parse redirect URL"); + + let modlist = ModifyList::new_list(vec![ + Modify::Present(Attribute::OAuth2AllowLocalhostRedirect, Value::Bool(true)), + Modify::Present(Attribute::OAuth2RsOrigin, Value::Url(redirect_uri.clone())), + ]); assert!(idms_prox_write .qs_write @@ -6623,7 +6640,7 @@ mod tests { code_challenge, code_challenge_method: CodeChallengeMethod::S256, }), - redirect_uri: Url::parse("http://localhost:8765/oauth2/result").unwrap(), + redirect_uri: redirect_uri.clone(), scope: OAUTH2_SCOPE_OPENID.to_string(), nonce: Some("abcdef".to_string()), oidc_ext: Default::default(), @@ -6655,7 +6672,7 @@ mod tests { let token_req = AccessTokenRequest { grant_type: GrantTypeReq::AuthorizationCode { code: permit_success.code, - redirect_uri: Url::parse("http://localhost:8765/oauth2/result").unwrap(), + redirect_uri, // From the first step. code_verifier, }, @@ -6873,4 +6890,70 @@ mod tests { dbg!(&res); assert!(res.is_err()); } + + #[test] + fn test_url_localhost_domain() { + // ref #2390 - localhost with ports for OAuth2 redirect_uri + + // ensure host_is_local isn't true for a non-local host + let example_is_not_local = "https://example.com/sdfsdf"; + println!("Ensuring that {} is not local", example_is_not_local); + assert!(!host_is_local( + &Url::parse(example_is_not_local) + .expect("Failed to parse example.com as a host?") + .host() + .expect(&format!( + "Couldn't get a host from {}", + example_is_not_local + )) + )); + + let test_urls = [ + ("http://localhost:8080/oauth2/callback", "/oauth2/callback"), + ("https://localhost/foo/bar", "/foo/bar"), + ("http://127.0.0.1:12345/foo", "/foo"), + ("http://[::1]:12345/foo", "/foo"), + ]; + + for (url, path) in test_urls.into_iter() { + println!("Testing URL: {}", url); + let url = Url::parse(url).expect("One of the test values failed!"); + assert!(host_is_local( + &url.host().expect("Didn't parse a host out?") + )); + + assert_eq!(url.path(), path); + } + } + + #[test] + fn test_oauth2_rs_type_allow_localhost_redirect() { + let test_cases = [ + ( + OauthRSType::Public { + allow_localhost_redirect: true, + }, + true, + ), + ( + OauthRSType::Public { + allow_localhost_redirect: false, + }, + false, + ), + ( + OauthRSType::Basic { + authz_secret: "supersecret".to_string(), + enable_pkce: false, + }, + false, + ), + ]; + + assert!(test_cases.iter().all(|(rs_type, expected)| { + let actual = rs_type.allow_localhost_redirect(); + println!("Testing {:?} -> {}", rs_type, expected); + actual == *expected + })); + } }