From c7fcdc3e4e796ff7e12ce2a98c0744bb02c0d247 Mon Sep 17 00:00:00 2001 From: Firstyear Date: Sat, 20 Jul 2024 12:09:50 +1000 Subject: [PATCH] Strict redirect URL enforcement (#2917) Add strict OAuth2 URL enforcement per the RFC. This includes a transition process for the next release so that Admins can come into compliance. --- book/src/access_control/intro.md | 4 +- .../src/accounts/posix_accounts_and_groups.md | 3 +- .../developers/designs/authentication_flow.md | 3 +- .../designs/replication_coordinator.md | 4 +- book/src/frequently_asked_questions.md | 9 +- book/src/integrations/ldap.md | 4 +- book/src/integrations/oauth2.md | 75 ++++++-- libs/client/src/oauth.rs | 32 +++- proto/src/constants.rs | 1 + proto/src/internal/error.rs | 2 + proto/src/internal/mod.rs | 3 + server/daemon/src/main.rs | 129 +++++++------ server/lib/src/constants/acp.rs | 106 +++++++++++ server/lib/src/constants/entries.rs | 3 + server/lib/src/constants/mod.rs | 10 +- server/lib/src/constants/schema.rs | 10 + server/lib/src/constants/uuids.rs | 2 + server/lib/src/idm/oauth2.rs | 111 ++++++++--- server/lib/src/idm/server.rs | 10 +- server/lib/src/server/migrations.rs | 175 +++++++++++++++++- tools/cli/src/cli/oauth2.rs | 23 +++ tools/cli/src/opt/kanidm.rs | 33 +++- 22 files changed, 626 insertions(+), 126 deletions(-) diff --git a/book/src/access_control/intro.md b/book/src/access_control/intro.md index 411d587a0..e8f94032e 100644 --- a/book/src/access_control/intro.md +++ b/book/src/access_control/intro.md @@ -20,8 +20,8 @@ possible harm that an attacker may make if they gain access to these roles. Kanidm supports [privilege access mode](../accounts/authentication_and_credentials.md) so that high-level permissions can be assigned to users who must reauthenticate before using those privileges. The privileges then are only accessible for a short period of time. This can allow you -to assign high level permissions to regular person accounts rather than requiring separate -privilege access accounts (PAA) or privileged access workstations (PAW). +to assign high level permissions to regular person accounts rather than requiring separate privilege +access accounts (PAA) or privileged access workstations (PAW). ## Assigning Permissions to Service Accounts diff --git a/book/src/accounts/posix_accounts_and_groups.md b/book/src/accounts/posix_accounts_and_groups.md index bc381d253..6c58f4461 100644 --- a/book/src/accounts/posix_accounts_and_groups.md +++ b/book/src/accounts/posix_accounts_and_groups.md @@ -1,8 +1,7 @@ # POSIX Accounts and Groups Kanidm has features that enable its accounts and groups to be consumed on POSIX-like machines, such -as Linux, FreeBSD or others. Both service accounts and person accounts can be used on POSIX -systems. +as Linux, FreeBSD or others. Both service accounts and person accounts can be used on POSIX systems. ## Notes on POSIX Features diff --git a/book/src/developers/designs/authentication_flow.md b/book/src/developers/designs/authentication_flow.md index 380cd62fe..66bafffc8 100644 --- a/book/src/developers/designs/authentication_flow.md +++ b/book/src/developers/designs/authentication_flow.md @@ -8,7 +8,8 @@ 3. Client requests auth with a method (`AuthStep::Begin(AuthMech)`) 4. Server responds with an acknowledgement (`AuthState::Continue(Vec)`). This is so the challenge can be included in the response, for Passkeys or other challenge-response methods. - - If required, this challenge/response continues in a loop until the requirements are satisfied. For example, TOTP and then Password. + - If required, this challenge/response continues in a loop until the requirements are satisfied. + For example, TOTP and then Password. 5. The result is returned, either: - Success, with the User Auth Token as a `String`. - Denied, with a reason as a `String`. diff --git a/book/src/developers/designs/replication_coordinator.md b/book/src/developers/designs/replication_coordinator.md index 23b72426f..7aa5018d7 100644 --- a/book/src/developers/designs/replication_coordinator.md +++ b/book/src/developers/designs/replication_coordinator.md @@ -241,8 +241,8 @@ Imagine the following example. Here, Node A is acting as the KRC. This would allow Node A to be aware of B, C, D and then create a full mesh. -We wish to decommission Node A and promote Node B to become the new KRC. Imagine at this point we cut -over Node D to point its KRC at Node B. +We wish to decommission Node A and promote Node B to become the new KRC. Imagine at this point we +cut over Node D to point its KRC at Node B. ``` ┌─────────────────┐ ┌─────────────────┐ diff --git a/book/src/frequently_asked_questions.md b/book/src/frequently_asked_questions.md index 7f500fd9e..71f0bf1c7 100644 --- a/book/src/frequently_asked_questions.md +++ b/book/src/frequently_asked_questions.md @@ -54,10 +54,11 @@ has an excellent explanation of the attack. Additionally, this threat is discuss [RFC6819 Section 4.4.1](https://www.rfc-editor.org/rfc/rfc6819#section-4.4.1). As Kanidm aims for "secure by default" design, even with _confidential_ clients, we deem it -important to raise the bar for attackers. For example, an attacker may have access to the `client_id` -and `client_secret` of a confidential client as it was mishandled by a system administrator. While -they may not have direct access to the client/application systems, they could still use this -`client_id+secret` to then carry out the authorisation code interception attack listed. +important to raise the bar for attackers. For example, an attacker may have access to the +`client_id` and `client_secret` of a confidential client as it was mishandled by a system +administrator. While they may not have direct access to the client/application systems, they could +still use this `client_id+secret` to then carry out the authorisation code interception attack +listed. For confidential clients (refered to as a `basic` client in Kanidm due to the use of HTTP Basic for `client_id+secret` presentation) PKCE may optionally be disabled. This can allow authorisation code diff --git a/book/src/integrations/ldap.md b/book/src/integrations/ldap.md index 0939c3f7c..eb85b8ade 100644 --- a/book/src/integrations/ldap.md +++ b/book/src/integrations/ldap.md @@ -31,8 +31,8 @@ key-values on objects which are all UTF8 strings (or subsets thereof) based on v rules. Kanidm internally implements complex structured data types such as tagging on SSH keys, or multi-value credentials. These can not be represented in LDAP. -Many of the structures in Kanidm do not correlate closely to LDAP. For example, Kanidm only has a GID -number, where LDAP's schemas define both a UID number and a GID number. +Many of the structures in Kanidm do not correlate closely to LDAP. For example, Kanidm only has a +GID number, where LDAP's schemas define both a UID number and a GID number. Entries in the database also have a specific name in LDAP, related to their path in the directory tree. Kanidm is a flat model, so we have to emulate some tree-like elements, and ignore others. diff --git a/book/src/integrations/oauth2.md b/book/src/integrations/oauth2.md index 3ea0cfc12..dc0da3439 100644 --- a/book/src/integrations/oauth2.md +++ b/book/src/integrations/oauth2.md @@ -130,13 +130,22 @@ After you have understood your service requirements you first need to configure members of `system_admins` or `idm_hp_oauth2_manage_priv` are able to create or manage OAuth2 client integrations. -You can create a new client with: +You can create a new client by specifying its client name, application display name and the landing +page (home page) of the client. The landing page is where users will be redirected to in order to +begin an OAuth2 flow from the application portal. ```bash -kanidm system oauth2 create +kanidm system oauth2 create kanidm system oauth2 create nextcloud "Nextcloud Production" https://nextcloud.example.com ``` +You must now configure the redirect URL where the application expects OAuth2 requests to be sent. + +```bash +kanidm system oauth2 add-redirect-url +kanidm system oauth2 add-redirect-url nextcloud https://nextcloud.example.com/oauth2/handler +``` + You can create a scope map with: ```bash @@ -286,25 +295,69 @@ kanidm system oauth2 disable-localhost-redirects kanidm system oauth2 enable-localhost-redirects mywebapp ``` -## Alternate Redirect Origins +## Alternate Redirect URLs -Some services may have a website URL as well as native applications. These native applications -require alternate redirection URLs to be configured so that after an OAuth2 exchange, the system can -redirect to the native application. +{{#template ../templates/kani-warning.md imagepath=../images title=WARNING text=SECURITY RISK }} -To support this Kanidm allows supplemental origins to be configured on clients. +> You **MUST NOT** share a single OAuth2 client definition between multiple applications. +> +> The ability to configure multiple redirect URLs is **NOT** to allow you to share a single Kanidm +> client definition between multiple OAuth2 clients. +> +> Sharing OAuth2 client configurations between applications **FUNDAMENTALLY BREAKS** the OAuth2 +> security model and is **NOT SUPPORTED** as a configuration. The Kanidm Project **WILL NOT** +> support you if you attempt this. +> +> Multiple origins are **ONLY** to allow supplemental redirects within the _same_ client +> application. -{{#template ../templates/kani-warning.md imagepath=../images title=WARNING text=The ability to configure multiple origins is NOT intended to allow you to share a single Kanidm client definition between multiple OAuth2 clients. This fundamentally breaks the OAuth2 security model and is NOT SUPPORTED as a configuration. Multiple origins is only to allow supplemental redirects within the _same_ client application. }} +Some services may have a website URL as well as native applications with opaque origins. These +native applications require alternate redirection URLs to be configured so that after an OAuth2 +exchange, the system can redirect to the native application. + +To support this Kanidm allows supplemental opaque origins to be configured on clients. ```bash -kanidm system oauth2 add-origin -kanidm system oauth2 remove-origin +kanidm system oauth2 add-redirect-url +kanidm system oauth2 remove-redirect-url -kanidm system oauth2 add-origin nextcloud app://ios-nextcloud +kanidm system oauth2 add-redirect-url nextcloud app://ios-nextcloud ``` Supplemental URLs are shown in the OAuth2 client configuration in the `oauth2_rs_origin` attribute. +### Strict Redirect URLs + +Kanidm previously enforce that redirection targets only matched by _origin_, not the full URL. In +1.4.0 these URLs will enforce a full URL match instead. + +To indicate your readiness for this transition, all OAuth2 clients must have the field +`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. + +To enable or disable strict validation: + +```bash +kanidm system oauth2 enable-strict-redirect-url +kanidm system oauth2 disable-strict-redirect-url +``` + +### Why Does Sharing a Client Weaken OAuth2? + +By sharing a client ID between multiple applications this implies that all of these applications are +a singular client in Kanidm's view. This means tokens issued to one application can be reused on any +other application. + +This limits your ability to enforce scopes, presents a large compromise blast radius if a token is +stolen, and also increases the damage radius if the client credentials are stolen. + +Generally this also provides a bad user experience since the Kanidm application portal only lists a +single landing URL of the client, so subsequent applications that "piggy back" can not be redirected +to from the application portal meaning that users will not easily be able to access the application. + ## Extended Options for Legacy Clients Not all clients support modern standards like PKCE or ECDSA. In these situations it may be necessary diff --git a/libs/client/src/oauth.rs b/libs/client/src/oauth.rs index 3f29d8adc..e95a9e3f6 100644 --- a/libs/client/src/oauth.rs +++ b/libs/client/src/oauth.rs @@ -4,7 +4,7 @@ use kanidm_proto::constants::{ ATTR_OAUTH2_ALLOW_INSECURE_CLIENT_DISABLE_PKCE, ATTR_OAUTH2_ALLOW_LOCALHOST_REDIRECT, ATTR_OAUTH2_JWT_LEGACY_CRYPTO_ENABLE, ATTR_OAUTH2_PREFER_SHORT_USERNAME, ATTR_OAUTH2_RS_BASIC_SECRET, ATTR_OAUTH2_RS_ORIGIN, ATTR_OAUTH2_RS_ORIGIN_LANDING, - ATTR_OAUTH2_RS_TOKEN_KEY, ATTR_RS256_PRIVATE_KEY_DER, + ATTR_OAUTH2_RS_TOKEN_KEY, ATTR_OAUTH2_STRICT_REDIRECT_URI, ATTR_RS256_PRIVATE_KEY_DER, }; use kanidm_proto::internal::{ImageValue, Oauth2ClaimMapJoin}; use kanidm_proto::v1::Entry; @@ -350,6 +350,36 @@ impl KanidmClient { .await } + pub async fn idm_oauth2_rs_enable_strict_redirect_uri( + &self, + id: &str, + ) -> Result<(), ClientError> { + let mut update_oauth2_rs = Entry { + attrs: BTreeMap::new(), + }; + update_oauth2_rs.attrs.insert( + ATTR_OAUTH2_STRICT_REDIRECT_URI.to_string(), + vec!["true".to_string()], + ); + self.perform_patch_request(format!("/v1/oauth2/{}", id).as_str(), update_oauth2_rs) + .await + } + + pub async fn idm_oauth2_rs_disable_strict_redirect_uri( + &self, + id: &str, + ) -> Result<(), ClientError> { + let mut update_oauth2_rs = Entry { + attrs: BTreeMap::new(), + }; + update_oauth2_rs.attrs.insert( + ATTR_OAUTH2_STRICT_REDIRECT_URI.to_string(), + vec!["false".to_string()], + ); + self.perform_patch_request(format!("/v1/oauth2/{}", id).as_str(), update_oauth2_rs) + .await + } + pub async fn idm_oauth2_rs_update_claim_map( &self, id: &str, diff --git a/proto/src/constants.rs b/proto/src/constants.rs index 07e14d8be..a43fa2ab1 100644 --- a/proto/src/constants.rs +++ b/proto/src/constants.rs @@ -158,6 +158,7 @@ pub const ATTR_OAUTH2_RS_SCOPE_MAP: &str = "oauth2_rs_scope_map"; pub const ATTR_OAUTH2_RS_SUP_SCOPE_MAP: &str = "oauth2_rs_sup_scope_map"; pub const ATTR_OAUTH2_RS_TOKEN_KEY: &str = "oauth2_rs_token_key"; pub const ATTR_OAUTH2_SESSION: &str = "oauth2_session"; +pub const ATTR_OAUTH2_STRICT_REDIRECT_URI: &str = "oauth2_strict_redirect_uri"; pub const ATTR_OBJECTCLASS: &str = "objectclass"; pub const ATTR_OTHER_NO_INDEX: &str = "other-no-index"; pub const ATTR_PASSKEYS: &str = "passkeys"; diff --git a/proto/src/internal/error.rs b/proto/src/internal/error.rs index fd2c2bac1..35b11b01f 100644 --- a/proto/src/internal/error.rs +++ b/proto/src/internal/error.rs @@ -152,6 +152,7 @@ pub enum OperationError { MG0004DomainLevelInDevelopment, MG0005GidConstraintsNotMet, MG0006SKConstraintsNotMet, + MG0007Oauth2StrictConstraintsNotMet, // KP0001KeyProviderNotLoaded, KP0002KeyProviderInvalidClass, @@ -304,6 +305,7 @@ impl OperationError { Self::MG0004DomainLevelInDevelopment => None, Self::MG0005GidConstraintsNotMet => None, Self::MG0006SKConstraintsNotMet => Some("Migration Constraints Not Met - Security Keys should not be present."), + Self::MG0007Oauth2StrictConstraintsNotMet => Some("Migration Constraints Not Met - All OAuth2 clients must have strict-redirect-uri mode enabled."), Self::KP0001KeyProviderNotLoaded => None, Self::KP0002KeyProviderInvalidClass => None, Self::KP0003KeyProviderInvalidType => None, diff --git a/proto/src/internal/mod.rs b/proto/src/internal/mod.rs index 947808dab..0469eb6a1 100644 --- a/proto/src/internal/mod.rs +++ b/proto/src/internal/mod.rs @@ -245,6 +245,9 @@ pub enum DomainUpgradeCheckStatus { Pass7To8SecurityKeys, Fail7To8SecurityKeys, + + Pass7To8Oauth2StrictRedirectUri, + Fail7To8Oauth2StrictRedirectUri, } #[derive(Debug, Serialize, Deserialize, Clone)] diff --git a/server/daemon/src/main.rs b/server/daemon/src/main.rs index 482f7623c..1f1565a61 100644 --- a/server/daemon/src/main.rs +++ b/server/daemon/src/main.rs @@ -180,68 +180,89 @@ async fn submit_admin_req(path: &str, req: AdminTaskRequest, output_mode: Consol } }, - Some(Ok(AdminTaskResponse::DomainUpgradeCheck { report })) => match output_mode { - ConsoleOutputMode::JSON => { - let json_output = serde_json::json!({ - "domain_upgrade_check": report - }); - println!("{}", json_output); - } - ConsoleOutputMode::Text => { - let ProtoDomainUpgradeCheckReport { - name, - uuid, - current_level, - upgrade_level, - report_items, - } = report; + Some(Ok(AdminTaskResponse::DomainUpgradeCheck { report })) => { + match output_mode { + ConsoleOutputMode::JSON => { + let json_output = serde_json::json!({ + "domain_upgrade_check": report + }); + println!("{}", json_output); + } + ConsoleOutputMode::Text => { + let ProtoDomainUpgradeCheckReport { + name, + uuid, + current_level, + upgrade_level, + report_items, + } = report; - info!("domain_name : {}", name); - info!("domain_uuid : {}", uuid); - info!("domain_current_level : {}", current_level); - info!("domain_upgrade_level : {}", upgrade_level); + info!("domain_name : {}", name); + info!("domain_uuid : {}", uuid); + info!("domain_current_level : {}", current_level); + info!("domain_upgrade_level : {}", upgrade_level); - for item in report_items { - info!("------------------------"); - match item.status { - ProtoDomainUpgradeCheckStatus::Pass6To7Gidnumber => { - info!("upgrade_item : gidnumber range validity"); - debug!("from_level : {}", item.from_level); - debug!("to_level : {}", item.to_level); - info!("status : PASS"); - } - ProtoDomainUpgradeCheckStatus::Fail6To7Gidnumber => { - info!("upgrade_item : gidnumber range validity"); - debug!("from_level : {}", item.from_level); - debug!("to_level : {}", item.to_level); - info!("status : FAIL"); - info!("description : The automatically allocated gidnumbers for posix accounts was found to allocate numbers into systemd-reserved ranges. These can no longer be used."); - info!("action : Modify the gidnumber of affected entries so that they are in the range 65536 to 524287 OR reset the gidnumber to cause it to automatically regenerate."); - for entry_id in item.affected_entries { - info!("affected_entry : {}", entry_id); + for item in report_items { + info!("------------------------"); + match item.status { + ProtoDomainUpgradeCheckStatus::Pass6To7Gidnumber => { + info!("upgrade_item : gidnumber range validity"); + debug!("from_level : {}", item.from_level); + debug!("to_level : {}", item.to_level); + info!("status : PASS"); } - } - ProtoDomainUpgradeCheckStatus::Pass7To8SecurityKeys => { - info!("upgrade_item : security key usage"); - debug!("from_level : {}", item.from_level); - debug!("to_level : {}", item.to_level); - info!("status : PASS"); - } - ProtoDomainUpgradeCheckStatus::Fail7To8SecurityKeys => { - info!("upgrade_item : security key usage"); - debug!("from_level : {}", item.from_level); - debug!("to_level : {}", item.to_level); - info!("status : FAIL"); - info!("description : Security keys no longer function as a second factor due to the introduction of CTAP2 and greater forcing PIN interactions."); - info!("action : Modify the accounts in question to remove their security key and add it as a passkey or enable TOTP"); - for entry_id in item.affected_entries { - info!("affected_entry : {}", entry_id); + ProtoDomainUpgradeCheckStatus::Fail6To7Gidnumber => { + info!("upgrade_item : gidnumber range validity"); + debug!("from_level : {}", item.from_level); + debug!("to_level : {}", item.to_level); + info!("status : FAIL"); + info!("description : The automatically allocated gidnumbers for posix accounts was found to allocate numbers into systemd-reserved ranges. These can no longer be used."); + info!("action : Modify the gidnumber of affected entries so that they are in the range 65536 to 524287 OR reset the gidnumber to cause it to automatically regenerate."); + for entry_id in item.affected_entries { + info!("affected_entry : {}", entry_id); + } + } + // =========== + ProtoDomainUpgradeCheckStatus::Pass7To8SecurityKeys => { + info!("upgrade_item : security key usage"); + debug!("from_level : {}", item.from_level); + debug!("to_level : {}", item.to_level); + info!("status : PASS"); + } + ProtoDomainUpgradeCheckStatus::Fail7To8SecurityKeys => { + info!("upgrade_item : security key usage"); + debug!("from_level : {}", item.from_level); + debug!("to_level : {}", item.to_level); + info!("status : FAIL"); + info!("description : Security keys no longer function as a second factor due to the introduction of CTAP2 and greater forcing PIN interactions."); + info!("action : Modify the accounts in question to remove their security key and add it as a passkey or enable TOTP"); + for entry_id in item.affected_entries { + info!("affected_entry : {}", entry_id); + } + } + // =========== + ProtoDomainUpgradeCheckStatus::Pass7To8Oauth2StrictRedirectUri => { + info!("upgrade_item : oauth2 strict redirect uri enforcement"); + debug!("from_level : {}", item.from_level); + debug!("to_level : {}", item.to_level); + info!("status : PASS"); + } + ProtoDomainUpgradeCheckStatus::Fail7To8Oauth2StrictRedirectUri => { + info!("upgrade_item : oauth2 strict redirect uri enforcement"); + debug!("from_level : {}", item.from_level); + debug!("to_level : {}", item.to_level); + info!("status : FAIL"); + info!("description : To harden against possible public client open redirection vulnerabilities, redirect uris must now be registered ahead of time and are validated rather than the former origin verification process."); + info!("action : Verify the redirect uri's for OAuth2 clients and then enable strict-redirect-uri on each client."); + for entry_id in item.affected_entries { + info!("affected_entry : {}", entry_id); + } } } } } } - }, + } Some(Ok(AdminTaskResponse::DomainRaise { level })) => match output_mode { ConsoleOutputMode::JSON => { diff --git a/server/lib/src/constants/acp.rs b/server/lib/src/constants/acp.rs index 0c053d4c9..f3409649d 100644 --- a/server/lib/src/constants/acp.rs +++ b/server/lib/src/constants/acp.rs @@ -881,6 +881,112 @@ lazy_static! { }; } +lazy_static! { + pub static ref IDM_ACP_OAUTH2_MANAGE_DL7: BuiltinAcp = BuiltinAcp { + classes: vec![ + EntryClass::Object, + EntryClass::AccessControlProfile, + EntryClass::AccessControlCreate, + EntryClass::AccessControlDelete, + EntryClass::AccessControlModify, + EntryClass::AccessControlSearch + ], + name: "idm_acp_hp_oauth2_manage_priv", + uuid: UUID_IDM_ACP_OAUTH2_MANAGE_V1, + description: "Builtin IDM Control for managing oauth2 resource server integrations.", + receiver: BuiltinAcpReceiver::Group(vec![UUID_IDM_OAUTH2_ADMINS]), + target: BuiltinAcpTarget::Filter(ProtoFilter::And(vec![ + match_class_filter!(EntryClass::OAuth2ResourceServer), + FILTER_ANDNOT_TOMBSTONE_OR_RECYCLED.clone(), + ])), + search_attrs: vec![ + Attribute::Class, + Attribute::Description, + Attribute::DisplayName, + Attribute::Name, + Attribute::Spn, + Attribute::OAuth2Session, + Attribute::OAuth2RsOrigin, + Attribute::OAuth2RsOriginLanding, + Attribute::OAuth2RsScopeMap, + Attribute::OAuth2RsSupScopeMap, + Attribute::OAuth2RsBasicSecret, + Attribute::OAuth2RsTokenKey, + Attribute::Es256PrivateKeyDer, + Attribute::OAuth2AllowInsecureClientDisablePkce, + Attribute::Rs256PrivateKeyDer, + Attribute::OAuth2JwtLegacyCryptoEnable, + Attribute::OAuth2PreferShortUsername, + Attribute::OAuth2AllowLocalhostRedirect, + Attribute::OAuth2RsClaimMap, + Attribute::Image, + Attribute::OAuth2StrictRedirectUri, + ], + modify_removed_attrs: vec![ + Attribute::Description, + Attribute::DisplayName, + Attribute::Name, + Attribute::OAuth2Session, + Attribute::OAuth2RsOrigin, + Attribute::OAuth2RsOriginLanding, + Attribute::OAuth2RsScopeMap, + Attribute::OAuth2RsSupScopeMap, + Attribute::OAuth2RsBasicSecret, + Attribute::OAuth2RsTokenKey, + Attribute::Es256PrivateKeyDer, + Attribute::OAuth2AllowInsecureClientDisablePkce, + Attribute::Rs256PrivateKeyDer, + Attribute::OAuth2JwtLegacyCryptoEnable, + Attribute::OAuth2PreferShortUsername, + Attribute::OAuth2AllowLocalhostRedirect, + Attribute::OAuth2RsClaimMap, + Attribute::Image, + Attribute::OAuth2StrictRedirectUri, + ], + modify_present_attrs: vec![ + Attribute::Description, + Attribute::DisplayName, + Attribute::Name, + Attribute::OAuth2RsOrigin, + Attribute::OAuth2RsOriginLanding, + Attribute::OAuth2RsSupScopeMap, + Attribute::OAuth2RsScopeMap, + Attribute::OAuth2AllowInsecureClientDisablePkce, + Attribute::OAuth2JwtLegacyCryptoEnable, + Attribute::OAuth2PreferShortUsername, + Attribute::OAuth2AllowLocalhostRedirect, + Attribute::OAuth2RsClaimMap, + Attribute::Image, + Attribute::OAuth2StrictRedirectUri, + ], + create_attrs: vec![ + Attribute::Class, + Attribute::Description, + Attribute::Name, + Attribute::OAuth2RsName, + Attribute::OAuth2RsOrigin, + Attribute::OAuth2RsOriginLanding, + Attribute::OAuth2RsSupScopeMap, + Attribute::OAuth2RsScopeMap, + Attribute::OAuth2AllowInsecureClientDisablePkce, + Attribute::OAuth2JwtLegacyCryptoEnable, + Attribute::OAuth2PreferShortUsername, + Attribute::OAuth2AllowLocalhostRedirect, + Attribute::OAuth2RsClaimMap, + Attribute::Image, + Attribute::OAuth2StrictRedirectUri, + ], + create_classes: vec![ + EntryClass::Object, + EntryClass::Account, + EntryClass::OAuth2ResourceServer, + EntryClass::OAuth2ResourceServerBasic, + EntryClass::OAuth2ResourceServerPublic, + ], + ..Default::default() + }; +} + lazy_static! { pub static ref IDM_ACP_DOMAIN_ADMIN_V1: BuiltinAcp = BuiltinAcp { classes: vec![ diff --git a/server/lib/src/constants/entries.rs b/server/lib/src/constants/entries.rs index 855f02c00..fcee1e427 100644 --- a/server/lib/src/constants/entries.rs +++ b/server/lib/src/constants/entries.rs @@ -143,6 +143,7 @@ pub enum Attribute { OAuth2RsSupScopeMap, OAuth2RsTokenKey, OAuth2Session, + OAuth2StrictRedirectUri, ObjectClass, OtherNoIndex, PassKeys, @@ -335,6 +336,7 @@ impl<'a> TryFrom<&'a str> for Attribute { ATTR_OAUTH2_RS_SUP_SCOPE_MAP => Attribute::OAuth2RsSupScopeMap, ATTR_OAUTH2_RS_TOKEN_KEY => Attribute::OAuth2RsTokenKey, ATTR_OAUTH2_SESSION => Attribute::OAuth2Session, + ATTR_OAUTH2_STRICT_REDIRECT_URI => Attribute::OAuth2StrictRedirectUri, ATTR_OBJECTCLASS => Attribute::ObjectClass, ATTR_OTHER_NO_INDEX => Attribute::OtherNoIndex, ATTR_PASSKEYS => Attribute::PassKeys, @@ -510,6 +512,7 @@ impl From for &'static str { Attribute::OAuth2RsSupScopeMap => ATTR_OAUTH2_RS_SUP_SCOPE_MAP, Attribute::OAuth2RsTokenKey => ATTR_OAUTH2_RS_TOKEN_KEY, Attribute::OAuth2Session => ATTR_OAUTH2_SESSION, + Attribute::OAuth2StrictRedirectUri => ATTR_OAUTH2_STRICT_REDIRECT_URI, Attribute::ObjectClass => ATTR_OBJECTCLASS, Attribute::OtherNoIndex => ATTR_OTHER_NO_INDEX, Attribute::PassKeys => ATTR_PASSKEYS, diff --git a/server/lib/src/constants/mod.rs b/server/lib/src/constants/mod.rs index d9bb71096..b0fdeab66 100644 --- a/server/lib/src/constants/mod.rs +++ b/server/lib/src/constants/mod.rs @@ -84,13 +84,15 @@ pub const DOMAIN_MIN_REMIGRATION_LEVEL: DomainVersion = DOMAIN_LEVEL_5; pub const DOMAIN_MIN_LEVEL: DomainVersion = DOMAIN_TGT_LEVEL; // The previous releases domain functional level pub const DOMAIN_PREVIOUS_TGT_LEVEL: DomainVersion = DOMAIN_LEVEL_6; -// The target supported domain functional level +// The target supported domain functional level. During development this is +// the NEXT level that users will upgrade too. pub const DOMAIN_TGT_LEVEL: DomainVersion = DOMAIN_LEVEL_7; +// The current patch level if any out of band fixes are required. pub const DOMAIN_TGT_PATCH_LEVEL: u32 = PATCH_LEVEL_1; +// The target domain functional level for the SUBSEQUENT release/dev cycle. +pub const DOMAIN_TGT_NEXT_LEVEL: DomainVersion = DOMAIN_LEVEL_8; // The maximum supported domain functional level -pub const DOMAIN_MAX_LEVEL: DomainVersion = DOMAIN_LEVEL_7; -// The next maximum supported domain functional level -pub const DOMAIN_NEXT_LEVEL: DomainVersion = DOMAIN_LEVEL_8; +pub const DOMAIN_MAX_LEVEL: DomainVersion = DOMAIN_LEVEL_8; // On test builds define to 60 seconds #[cfg(test)] diff --git a/server/lib/src/constants/schema.rs b/server/lib/src/constants/schema.rs index d32ddfd83..20df8a28b 100644 --- a/server/lib/src/constants/schema.rs +++ b/server/lib/src/constants/schema.rs @@ -424,6 +424,15 @@ pub static ref SCHEMA_ATTR_OAUTH2_CONSENT_SCOPE_MAP: SchemaAttribute = SchemaAtt ..Default::default() }; +pub static ref SCHEMA_ATTR_OAUTH2_STRICT_REDIRECT_URI_DL7: SchemaAttribute = SchemaAttribute { + uuid: UUID_SCHEMA_ATTR_OAUTH2_STRICT_REDIRECT_URI, + name: Attribute::OAuth2StrictRedirectUri.into(), + description: "Represents if strict redirect uri enforcement is enabled.".to_string(), + + syntax: SyntaxType::Boolean, + ..Default::default() +}; + pub static ref SCHEMA_ATTR_ES256_PRIVATE_KEY_DER: SchemaAttribute = SchemaAttribute { uuid: UUID_SCHEMA_ATTR_ES256_PRIVATE_KEY_DER, name: Attribute::Es256PrivateKeyDer.into(), @@ -1261,6 +1270,7 @@ pub static ref SCHEMA_CLASS_OAUTH2_RS_DL7: SchemaClass = SchemaClass { Attribute::OAuth2RsClaimMap.into(), Attribute::OAuth2Session.into(), Attribute::OAuth2RsOrigin.into(), + Attribute::OAuth2StrictRedirectUri.into(), ], systemmust: vec![ Attribute::OAuth2RsOriginLanding.into(), diff --git a/server/lib/src/constants/uuids.rs b/server/lib/src/constants/uuids.rs index d093a5bba..e804d7891 100644 --- a/server/lib/src/constants/uuids.rs +++ b/server/lib/src/constants/uuids.rs @@ -309,6 +309,8 @@ pub const UUID_SCHEMA_ATTR_REFERS: Uuid = uuid!("00000000-0000-0000-0000-ffff000 pub const UUID_SCHEMA_ATTR_CERTIFICATE: Uuid = uuid!("00000000-0000-0000-0000-ffff00000178"); pub const UUID_SCHEMA_CLASS_CLIENT_CERTIFICATE: Uuid = uuid!("00000000-0000-0000-0000-ffff00000179"); +pub const UUID_SCHEMA_ATTR_OAUTH2_STRICT_REDIRECT_URI: Uuid = + uuid!("00000000-0000-0000-0000-ffff00000180"); // System and domain infos // I'd like to strongly criticise william of the past for making poor choices about these allocations. diff --git a/server/lib/src/idm/oauth2.rs b/server/lib/src/idm/oauth2.rs index 280fc97bb..bbf5f58da 100644 --- a/server/lib/src/idm/oauth2.rs +++ b/server/lib/src/idm/oauth2.rs @@ -273,7 +273,9 @@ pub struct Oauth2RS { origins: HashSet, opaque_origins: HashSet, + redirect_uris: HashSet, origin_https_required: bool, + strict_redirect_uri: bool, claim_map: BTreeMap>, scope_maps: BTreeMap>, @@ -335,11 +337,13 @@ pub struct Oauth2ResourceServersWriteTransaction<'a> { inner: CowCellWriteTxn<'a, Oauth2RSInner>, } -impl TryFrom<(Vec>, Url)> for Oauth2ResourceServers { +impl TryFrom<(Vec>, Url, DomainVersion)> for Oauth2ResourceServers { type Error = OperationError; - fn try_from(value: (Vec>, Url)) -> Result { - let (value, origin) = value; + fn try_from( + value: (Vec>, Url, DomainVersion), + ) -> Result { + let (value, origin, domain_level) = value; let fernet = Fernet::new(&Fernet::generate_key()).ok_or(OperationError::CryptographyError)?; let oauth2rs = Oauth2ResourceServers { @@ -351,7 +355,7 @@ impl TryFrom<(Vec>, Url)> for Oauth2ResourceServers { }; let mut oauth2rs_wr = oauth2rs.write(); - oauth2rs_wr.reload(value)?; + oauth2rs_wr.reload(value, domain_level)?; oauth2rs_wr.commit(); Ok(oauth2rs) } @@ -372,7 +376,11 @@ impl Oauth2ResourceServers { } impl<'a> Oauth2ResourceServersWriteTransaction<'a> { - pub fn reload(&mut self, value: Vec>) -> Result<(), OperationError> { + pub fn reload( + &mut self, + value: Vec>, + domain_level: DomainVersion, + ) -> Result<(), OperationError> { let rs_set: Result, _> = value .into_iter() .map(|ent| { @@ -432,18 +440,24 @@ impl<'a> Oauth2ResourceServersWriteTransaction<'a> { .cloned() .ok_or(OperationError::InvalidValueState)?; - let maybe_extra_origins = ent.get_ava_set(Attribute::OAuth2RsOrigin).and_then(|s| s.as_url_set()); + let maybe_extra_urls = ent.get_ava_set(Attribute::OAuth2RsOrigin).and_then(|s| s.as_url_set()); - let len_uris = maybe_extra_origins.map(|s| s.len() + 1).unwrap_or(1); + let len_uris = maybe_extra_urls.map(|s| s.len() + 1).unwrap_or(1); + + // If we are DL8, then strict enforcement is always required. + let strict_redirect_uri = cfg!(test) || + domain_level >= DOMAIN_LEVEL_8 || + ent.get_ava_single_bool(Attribute::OAuth2StrictRedirectUri) + .unwrap_or(false); // The reason we have to allocate this is that we need to do some processing on these // urls to determine if they are opaque or not. - let mut redirect_uris = Vec::with_capacity(len_uris); + let mut redirect_uris_v = Vec::with_capacity(len_uris); - redirect_uris.push(landing_url); - if let Some(extra_origins) = maybe_extra_origins { + redirect_uris_v.push(landing_url); + if let Some(extra_origins) = maybe_extra_urls { for x_origin in extra_origins { - redirect_uris.push(x_origin.clone()); + redirect_uris_v.push(x_origin.clone()); } } @@ -451,16 +465,22 @@ impl<'a> Oauth2ResourceServersWriteTransaction<'a> { // that may or may not be an opaque origin. We need to split these up now. let mut origins = HashSet::with_capacity(len_uris); + let mut redirect_uris = HashSet::with_capacity(len_uris); let mut opaque_origins = HashSet::with_capacity(len_uris); let mut origin_https_required = false; - for uri in redirect_uris.into_iter() { + for mut uri in redirect_uris_v.into_iter() { + // https://www.rfc-editor.org/rfc/rfc6749#section-3.1.2 + // Must not include a fragment. + uri.set_fragment(None); // Given the presence of a single https url, then all other urls must be https. if uri.scheme() == "https" { origin_https_required = true; origins.insert(uri.origin()); + redirect_uris.insert(uri); } else if uri.scheme() == "http" { origins.insert(uri.origin()); + redirect_uris.insert(uri); } else { opaque_origins.insert(uri); } @@ -640,7 +660,9 @@ impl<'a> Oauth2ResourceServersWriteTransaction<'a> { uuid, origins, opaque_origins, + redirect_uris, origin_https_required, + strict_redirect_uri, scope_maps, sup_scope_maps, client_scopes, @@ -1657,16 +1679,35 @@ impl<'a> IdmServerProxyReadTransaction<'a> { .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. - if !(o2rs.origins.contains(&auth_req.redirect_uri.origin()) - || o2rs.opaque_origins.contains(&auth_req.redirect_uri) - || (allow_localhost_redirect && localhost_redirect)) + 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) { - warn!( - "Invalid OAuth2 redirect_uri (must be related to origin) - got {:?}", - auth_req.redirect_uri.origin() - ); + 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); } @@ -2759,6 +2800,10 @@ mod tests { Value::new_url_s("https://demo.example.com").unwrap() ), // Supplemental origins + ( + Attribute::OAuth2RsOrigin, + Value::new_url_s("https://demo.example.com/oauth2/result").unwrap() + ), ( Attribute::OAuth2RsOrigin, Value::new_url_s("https://portal.example.com").unwrap() @@ -2920,6 +2965,10 @@ mod tests { Attribute::OAuth2RsOriginLanding, Value::new_url_s("https://demo.example.com").unwrap() ), + ( + Attribute::OAuth2RsOrigin, + Value::new_url_s("https://demo.example.com/oauth2/result").unwrap() + ), // System admins ( Attribute::OAuth2RsScopeMap, @@ -3267,6 +3316,26 @@ mod tests { == Oauth2Error::InvalidOrigin ); + // * invalid uri in the redirect + let auth_req = AuthorisationRequest { + response_type: "code".to_string(), + client_id: "test_resource_server".to_string(), + state: "123".to_string(), + pkce_request: pkce_request.clone(), + redirect_uri: Url::parse("https://demo.example.com/oauth2/wrong_place").unwrap(), + scope: OAUTH2_SCOPE_OPENID.to_string(), + nonce: None, + oidc_ext: Default::default(), + unknown_keys: Default::default(), + }; + + assert!( + idms_prox_read + .check_oauth2_authorisation(&ident, &auth_req, ct) + .unwrap_err() + == Oauth2Error::InvalidOrigin + ); + // Requested scope is not available let auth_req = AuthorisationRequest { response_type: "code".to_string(), @@ -3612,7 +3681,7 @@ mod tests { code_challenge: code_challenge.clone().into(), code_challenge_method: CodeChallengeMethod::S256, }), - redirect_uri: Url::parse("https://portal.example.com/oauth2/result").unwrap(), + redirect_uri: Url::parse("https://portal.example.com").unwrap(), scope: OAUTH2_SCOPE_OPENID.to_string(), nonce: Some("abcdef".to_string()), oidc_ext: Default::default(), @@ -3649,7 +3718,7 @@ mod tests { let token_req = AccessTokenRequest { grant_type: GrantTypeReq::AuthorizationCode { code: permit_success.code, - redirect_uri: Url::parse("https://portal.example.com/oauth2/result").unwrap(), + redirect_uri: Url::parse("https://portal.example.com").unwrap(), // From the first step. code_verifier: code_verifier.clone(), }, diff --git a/server/lib/src/idm/server.rs b/server/lib/src/idm/server.rs index 36b226d0e..710e795ff 100644 --- a/server/lib/src/idm/server.rs +++ b/server/lib/src/idm/server.rs @@ -145,11 +145,12 @@ impl IdmServer { let (audit_tx, audit_rx) = unbounded(); // Get the domain name, as the relying party id. - let (rp_id, rp_name, oauth2rs_set) = { + let (rp_id, rp_name, domain_level, oauth2rs_set) = { let mut qs_read = qs.read().await; ( qs_read.get_domain_name().to_string(), qs_read.get_domain_display_name().to_string(), + qs_read.get_domain_version(), // Add a read/reload of all oauth2 configurations. qs_read.get_oauth2rs_set()?, ) @@ -186,8 +187,8 @@ impl IdmServer { OperationError::InvalidState })?; - let oauth2rs = - Oauth2ResourceServers::try_from((oauth2rs_set, origin_url)).map_err(|e| { + let oauth2rs = Oauth2ResourceServers::try_from((oauth2rs_set, origin_url, domain_level)) + .map_err(|e| { admin_error!("Failed to load oauth2 resource servers - {:?}", e); e })?; @@ -2052,9 +2053,10 @@ impl<'a> IdmServerProxyWriteTransaction<'a> { #[instrument(level = "debug", skip_all)] pub fn commit(mut self) -> Result<(), OperationError> { if self.qs_write.get_changed_oauth2() { + let domain_level = self.qs_write.get_domain_version(); self.qs_write .get_oauth2rs_set() - .and_then(|oauth2rs_set| self.oauth2rs.reload(oauth2rs_set))?; + .and_then(|oauth2rs_set| self.oauth2rs.reload(oauth2rs_set, domain_level))?; // Clear the flag to indicate we completed the reload. self.qs_write.clear_changed_oauth2(); } diff --git a/server/lib/src/server/migrations.rs b/server/lib/src/server/migrations.rs index 73a2c0910..7170d2e5a 100644 --- a/server/lib/src/server/migrations.rs +++ b/server/lib/src/server/migrations.rs @@ -717,6 +717,7 @@ impl<'a> QueryServerWriteTransaction<'a> { SCHEMA_ATTR_REFERS_DL7.clone().into(), SCHEMA_ATTR_CERTIFICATE_DL7.clone().into(), SCHEMA_ATTR_OAUTH2_RS_ORIGIN_DL7.clone().into(), + SCHEMA_ATTR_OAUTH2_STRICT_REDIRECT_URI_DL7.clone().into(), SCHEMA_CLASS_DOMAIN_INFO_DL7.clone().into(), SCHEMA_CLASS_SERVICE_ACCOUNT_DL7.clone().into(), SCHEMA_CLASS_SYNC_ACCOUNT_DL7.clone().into(), @@ -760,6 +761,7 @@ impl<'a> QueryServerWriteTransaction<'a> { IDM_ACP_SELF_WRITE_DL7.clone().into(), IDM_ACP_SELF_NAME_WRITE_DL7.clone().into(), IDM_ACP_HP_CLIENT_CERTIFICATE_MANAGER_DL7.clone().into(), + IDM_ACP_OAUTH2_MANAGE_DL7.clone().into(), ]; idm_data @@ -828,6 +830,27 @@ impl<'a> QueryServerWriteTransaction<'a> { return Err(OperationError::MG0006SKConstraintsNotMet); } + // Check oauth2 strict uri + let filter = filter!(f_and!([ + f_eq(Attribute::Class, EntryClass::OAuth2ResourceServer.into()), + f_andnot(f_pres(Attribute::OAuth2StrictRedirectUri)), + ])); + + let results = self.internal_search(filter)?; + + let affected_entries = results + .into_iter() + .map(|entry| entry.get_display_id()) + .collect::>(); + + if !affected_entries.is_empty() { + error!("Unable to proceed. Not all oauth2 clients have strict redirect verification enabled."); + for missing_oauth2_strict_redirect_uri in affected_entries { + error!(%missing_oauth2_strict_redirect_uri); + } + return Err(OperationError::MG0007Oauth2StrictConstraintsNotMet); + } + // =========== Apply changes ============== Ok(()) @@ -1111,7 +1134,7 @@ impl<'a> QueryServerReadTransaction<'a> { let name = d_info.d_name.clone(); let uuid = d_info.d_uuid; let current_level = d_info.d_vers; - let upgrade_level = DOMAIN_NEXT_LEVEL; + let upgrade_level = DOMAIN_TGT_NEXT_LEVEL; let mut report_items = Vec::with_capacity(1); @@ -1139,6 +1162,17 @@ impl<'a> QueryServerReadTransaction<'a> { err })?; report_items.push(item); + + let item = self + .domain_upgrade_check_7_to_8_oauth2_strict_redirect_uri() + .map_err(|err| { + error!( + ?err, + "Failed to perform domain upgrade check 7 to 8 - oauth2-strict-redirect_uri" + ); + err + })?; + report_items.push(item); } Ok(ProtoDomainUpgradeCheckReport { @@ -1276,10 +1310,40 @@ impl<'a> QueryServerReadTransaction<'a> { affected_entries, }) } + + pub(crate) fn domain_upgrade_check_7_to_8_oauth2_strict_redirect_uri( + &mut self, + ) -> Result { + let filter = filter!(f_and!([ + f_eq(Attribute::Class, EntryClass::OAuth2ResourceServer.into()), + f_andnot(f_pres(Attribute::OAuth2StrictRedirectUri)), + ])); + + let results = self.internal_search(filter)?; + + let affected_entries = results + .into_iter() + .map(|entry| entry.get_display_id()) + .collect::>(); + + let status = if affected_entries.is_empty() { + ProtoDomainUpgradeCheckStatus::Pass7To8Oauth2StrictRedirectUri + } else { + ProtoDomainUpgradeCheckStatus::Fail7To8Oauth2StrictRedirectUri + }; + + Ok(ProtoDomainUpgradeCheckItem { + status, + from_level: DOMAIN_LEVEL_7, + to_level: DOMAIN_LEVEL_8, + affected_entries, + }) + } } #[cfg(test)] mod tests { + use super::{ProtoDomainUpgradeCheckItem, ProtoDomainUpgradeCheckStatus}; use crate::prelude::*; #[qs_test] @@ -1391,13 +1455,6 @@ mod tests { .internal_create(vec![ea]) .expect("Unable to create oauth2 client"); - // per migration verification. - let domain_entry = write_txn - .internal_search_uuid(UUID_DOMAIN_INFO) - .expect("Unable to access domain entry"); - - assert!(domain_entry.attribute_pres(Attribute::PrivateCookieKey)); - // Set the version to 7. write_txn .internal_modify_uuid( @@ -1436,4 +1493,106 @@ mod tests { write_txn.commit().expect("Unable to commit"); } + + #[qs_test(domain_level=DOMAIN_LEVEL_7)] + async fn test_migrations_dl7_dl8(server: &QueryServer) { + // Assert our instance was setup to version 7 + let mut write_txn = server.write(duration_from_epoch_now()).await; + + let db_domain_version = write_txn + .internal_search_uuid(UUID_DOMAIN_INFO) + .expect("unable to access domain entry") + .get_ava_single_uint32(Attribute::Version) + .expect("Attribute Version not present"); + + assert_eq!(db_domain_version, DOMAIN_LEVEL_7); + + // Create an oauth2 client that doesn't have a landing url set. + let oauth2_client_uuid = Uuid::new_v4(); + + let ea: Entry = entry_init!( + (Attribute::Class, EntryClass::Object.to_value()), + (Attribute::Class, EntryClass::Account.to_value()), + (Attribute::Uuid, Value::Uuid(oauth2_client_uuid)), + ( + Attribute::Class, + EntryClass::OAuth2ResourceServer.to_value() + ), + ( + Attribute::Class, + EntryClass::OAuth2ResourceServerPublic.to_value() + ), + (Attribute::Name, Value::new_iname("test_resource_server")), + ( + Attribute::DisplayName, + Value::new_utf8s("test_resource_server") + ), + ( + Attribute::OAuth2RsOriginLanding, + Value::new_url_s("https://demo.example.com/oauth2").unwrap() + ), + ( + Attribute::OAuth2RsOrigin, + Value::new_url_s("https://demo.example.com").unwrap() + ) + ); + + write_txn + .internal_create(vec![ea]) + .expect("Unable to create oauth2 client"); + + write_txn.commit().expect("Unable to commit"); + + // pre migration verification. + // check we currently would fail a migration. + + let mut read_txn = server.read().await; + + match read_txn.domain_upgrade_check_7_to_8_oauth2_strict_redirect_uri() { + Ok(ProtoDomainUpgradeCheckItem { + status: ProtoDomainUpgradeCheckStatus::Fail7To8Oauth2StrictRedirectUri, + .. + }) => { + trace!("Failed as expected, very good."); + } + other => { + error!(?other); + unreachable!(); + } + }; + + drop(read_txn); + + // Okay, fix the problem. + + let mut write_txn = server.write(duration_from_epoch_now()).await; + + write_txn + .internal_modify_uuid( + oauth2_client_uuid, + &ModifyList::new_purge_and_set( + Attribute::OAuth2StrictRedirectUri, + Value::Bool(true), + ), + ) + .expect("Unable to enforce strict mode."); + + // Set the version to 8. + write_txn + .internal_modify_uuid( + UUID_DOMAIN_INFO, + &ModifyList::new_purge_and_set( + Attribute::Version, + Value::new_uint32(DOMAIN_LEVEL_8), + ), + ) + .expect("Unable to set domain level to version 8"); + + // Re-load - this applies the migrations. + write_txn.reload().expect("Unable to reload transaction"); + + // post migration verification. + + write_txn.commit().expect("Unable to commit"); + } } diff --git a/tools/cli/src/cli/oauth2.rs b/tools/cli/src/cli/oauth2.rs index 65f0f39c5..150a02c78 100644 --- a/tools/cli/src/cli/oauth2.rs +++ b/tools/cli/src/cli/oauth2.rs @@ -38,6 +38,8 @@ impl Oauth2Opt { | Oauth2Opt::DeleteClaimMap { copt, .. } | Oauth2Opt::EnablePublicLocalhost { copt, .. } | Oauth2Opt::DisablePublicLocalhost { copt, .. } + | Oauth2Opt::EnableStrictRedirectUri { copt, .. } + | Oauth2Opt::DisableStrictRedirectUri { copt, .. } | Oauth2Opt::AddOrigin { copt, .. } | Oauth2Opt::RemoveOrigin { copt, .. } => copt.debug, } @@ -459,6 +461,27 @@ impl Oauth2Opt { Err(e) => handle_client_error(e, copt.output_mode), } } + Oauth2Opt::EnableStrictRedirectUri { copt, name } => { + let client = copt.to_client(OpType::Write).await; + match client + .idm_oauth2_rs_enable_strict_redirect_uri(name.as_str()) + .await + { + Ok(_) => println!("Success"), + Err(e) => handle_client_error(e, copt.output_mode), + } + } + + Oauth2Opt::DisableStrictRedirectUri { copt, name } => { + let client = copt.to_client(OpType::Write).await; + match client + .idm_oauth2_rs_disable_strict_redirect_uri(name.as_str()) + .await + { + Ok(_) => println!("Success"), + Err(e) => handle_client_error(e, copt.output_mode), + } + } } } } diff --git a/tools/cli/src/opt/kanidm.rs b/tools/cli/src/opt/kanidm.rs index 7b2de34e3..d4c90a09f 100644 --- a/tools/cli/src/opt/kanidm.rs +++ b/tools/cli/src/opt/kanidm.rs @@ -1062,23 +1062,23 @@ pub enum Oauth2Opt { #[clap(name="remove-image")] RemoveImage(Named), - /// Add a supplemental origin as a redirection target. For example a phone app + /// Add a supplemental URL as a redirection target. For example a phone app /// may use a redirect URL such as `app://my-cool-app` to trigger a native /// redirection event out of a browser. - #[clap(name = "add-origin")] + #[clap(name = "add-redirect-url")] AddOrigin { name: String, - #[clap(name = "origin-url")] + #[clap(name = "url")] origin: Url, #[clap(flatten)] copt: CommonOpt, }, - /// Remove a supplemental origin from the OAuth2 client configuration. - #[clap(name = "remove-origin")] + /// Remove a supplemental redirect URL from the OAuth2 client configuration. + #[clap(name = "remove-redirect-url")] RemoveOrigin { name: String, - #[clap(name = "origin-url")] + #[clap(name = "url")] origin: Url, #[clap(flatten)] copt: CommonOpt, @@ -1098,8 +1098,21 @@ pub enum Oauth2Opt { /// Disable legacy signing crypto on this oauth2 client. This is the default. #[clap(name = "disable-legacy-crypto")] DisableLegacyCrypto(Named), - #[clap(name = "prefer-short-username")] - + /// Enable strict validation of redirect URLs. Previously redirect URLs only + /// validated the origin of the URL matched. When enabled, redirect URLs must + /// match exactly. + #[clap(name = "enable-strict-redirect-url")] + EnableStrictRedirectUri { + name: String, + #[clap(flatten)] + copt: CommonOpt, + }, + #[clap(name = "disable-strict-redirect-url")] + DisableStrictRedirectUri { + name: String, + #[clap(flatten)] + copt: CommonOpt, + }, #[clap(name = "enable-localhost-redirects")] /// Allow public clients to redirect to localhost. EnablePublicLocalhost { @@ -1114,11 +1127,11 @@ pub enum Oauth2Opt { copt: CommonOpt, name: String, }, - /// Use the 'name' attribute instead of 'spn' for the preferred_username + #[clap(name = "prefer-short-username")] PreferShortUsername(Named), - #[clap(name = "prefer-spn-username")] /// Use the 'spn' attribute instead of 'name' for the preferred_username + #[clap(name = "prefer-spn-username")] PreferSPNUsername(Named), }