Allow OAuth2 loopback redirects if the path matches (#3252)

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

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.
+<!-- markdownlint-disable MD033 -->
 
 <dl>
 
@@ -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
 
 </dl>
 
+<!-- markdownlint-enable MD037 -->
+
 ## 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
 
 <!-- this is just to split the templates up -->
 
@@ -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 <name> <displayname> <origin>
 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 <name>
@@ -336,6 +343,10 @@ kanidm system oauth2 disable-localhost-redirects <name>
 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:
 

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(),

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<u32, Oauth2Error> {
     })
 }
 
+/// 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
+        }));
+    }
 }