From a02337a07a292a397c6fd6d00edd45f6b33faa2f Mon Sep 17 00:00:00 2001 From: Firstyear Date: Thu, 16 Feb 2023 09:40:16 +1000 Subject: [PATCH] 967 oauth2 implicit search (#1382) --- kanidmd/lib/src/constants/acp.rs | 4 + kanidmd/lib/src/constants/uuids.rs | 2 +- kanidmd/lib/src/idm/applinks.rs | 28 +++--- kanidmd/lib/src/server/access/mod.rs | 120 ++++++++++++++++++++++++ kanidmd/lib/src/server/access/search.rs | 46 +++++++++ kanidmd/lib/src/server/delete.rs | 19 ++++ kanidmd/lib/src/server/migrations.rs | 16 +++- kanidmd_web_ui/src/oauth2.rs | 5 +- 8 files changed, 221 insertions(+), 19 deletions(-) diff --git a/kanidmd/lib/src/constants/acp.rs b/kanidmd/lib/src/constants/acp.rs index 64fd3c1ef..a883f7327 100644 --- a/kanidmd/lib/src/constants/acp.rs +++ b/kanidmd/lib/src/constants/acp.rs @@ -1270,6 +1270,9 @@ pub const JSON_IDM_HP_ACP_SERVICE_ACCOUNT_INTO_PERSON_MIGRATE_V1: &str = r#"{ } }"#; +/* +// Removed in favour of a dynamic check inside of access.rs that is based on membership to an +// oauth2 rs. pub const JSON_IDM_ACP_OAUTH2_READ_PRIV_V1: &str = r#"{ "attrs": { "class": [ @@ -1294,6 +1297,7 @@ pub const JSON_IDM_ACP_OAUTH2_READ_PRIV_V1: &str = r#"{ ] } }"#; +*/ pub const JSON_IDM_HP_ACP_SYNC_ACCOUNT_MANAGE_PRIV_V1: &str = r#"{ "attrs": { diff --git a/kanidmd/lib/src/constants/uuids.rs b/kanidmd/lib/src/constants/uuids.rs index 9249c23a8..1d026c08b 100644 --- a/kanidmd/lib/src/constants/uuids.rs +++ b/kanidmd/lib/src/constants/uuids.rs @@ -300,7 +300,7 @@ pub const _UUID_IDM_PEOPLE_SELF_ACP_WRITE_MAIL_V1: Uuid = uuid!("00000000-0000-0000-0000-ffffff000041"); pub const _UUID_IDM_HP_ACP_SERVICE_ACCOUNT_INTO_PERSON_MIGRATE_V1: Uuid = uuid!("00000000-0000-0000-0000-ffffff000042"); -pub const _UUID_IDM_ACP_OAUTH2_READ_PRIV_V1: Uuid = uuid!("00000000-0000-0000-0000-ffffff000043"); +pub const UUID_IDM_ACP_OAUTH2_READ_PRIV_V1: Uuid = uuid!("00000000-0000-0000-0000-ffffff000043"); pub const _UUID_IDM_HP_ACP_SYNC_ACCOUNT_MANAGE_PRIV_V1: Uuid = uuid!("00000000-0000-0000-0000-ffffff000044"); pub const UUID_IDM_ACP_ACCOUNT_MAIL_READ_PRIV_V1: Uuid = diff --git a/kanidmd/lib/src/idm/applinks.rs b/kanidmd/lib/src/idm/applinks.rs index f1246d535..2742c7a7d 100644 --- a/kanidmd/lib/src/idm/applinks.rs +++ b/kanidmd/lib/src/idm/applinks.rs @@ -13,30 +13,26 @@ impl<'a> IdmServerProxyReadTransaction<'a> { } }; - // Do an internal search - // ⚠️ Safety Notes - We perform an internal search here which bypasses - // access controls. Why? Users normally can't read the oauth2_rs_scope_maps - // since that could (?) disclose access rules. It's probably not a risk, but - // we just don't show them by default. + // Formerly we did an internal search here, but we no longer need to since we have + // the access control module setup so that we can search for and see rs that we + // have access to. // - // This IS safe because we control *all* inputs (the uuids and memberof) and - // they come from the cryptographically verified UAT. we also control all - // outputs and ONLY output data that IS visible by default for an oauth2 - // resource server. - // - // This is kind of a limitation of the kani search system, where the ability to - // compare an attribute, also allows you to read it. In this case we want compare - // without read, but it's not really possible, and it's a silly concept generally - // anyway because publicly allowing that allows retrieval of the values to bruteforce. - let f = filter!(f_or( + // We do this weird looking f_executed/f_intent shenanigans to actually search + // on what we have access to, but we apply access as though we did a search on + // class=oauth2_resource_server instead, and we still apply access here. + let f_executed = filter!(f_or( ident_mo .iter() .copied() .map(|uuid| { f_eq("oauth2_rs_scope_map", PartialValue::Refer(uuid)) }) .collect() )); + let f_intent = filter!(f_eq("class", PVCLASS_OAUTH2_RS.clone())); - let oauth2_related = self.qs_read.internal_search(f)?; + // _ext reduces the entries based on access. + let oauth2_related = self + .qs_read + .impersonate_search_ext(f_executed, f_intent, ident)?; trace!(?oauth2_related); // Aggregate results to a Vec of AppLink diff --git a/kanidmd/lib/src/server/access/mod.rs b/kanidmd/lib/src/server/access/mod.rs index 8181fcea6..452d10bd5 100644 --- a/kanidmd/lib/src/server/access/mod.rs +++ b/kanidmd/lib/src/server/access/mod.rs @@ -2603,4 +2603,124 @@ mod tests { // Test reject purge test_acp_modify!(&me_purge, vec![acp_allow], &r2_set, false); } + + #[test] + fn test_access_ouath2_dyn_search() { + sketching::test_init(); + // Test that an account that is granted a scope to an oauth2 rs is granted + // the ability to search that rs. + let rs_uuid = Uuid::new_v4(); + let ev1 = unsafe { + entry_init!( + ("class", CLASS_OBJECT.clone()), + ("class", Value::new_class("oauth2_resource_server")), + ("class", Value::new_class("oauth2_resource_server_basic")), + ("uuid", Value::Uuid(rs_uuid)), + ("oauth2_rs_name", Value::new_iname("test_resource_server")), + ("displayname", Value::new_utf8s("test_resource_server")), + ( + "oauth2_rs_origin", + Value::new_url_s("https://demo.example.com").unwrap() + ), + ( + "oauth2_rs_scope_map", + Value::new_oauthscopemap(UUID_TEST_GROUP_1, btreeset!["groups".to_string()]) + .expect("invalid oauthscope") + ), + ( + "oauth2_rs_sup_scope_map", + Value::new_oauthscopemap( + UUID_TEST_GROUP_1, + btreeset!["supplement".to_string()] + ) + .expect("invalid oauthscope") + ), + ( + "oauth2_allow_insecure_client_disable_pkce", + Value::new_bool(true) + ), + ("oauth2_jwt_legacy_crypto_enable", Value::new_bool(false)), + ("oauth2_prefer_short_username", Value::new_bool(false)) + ) + .into_sealed_committed() + }; + + let ev1_reduced = unsafe { + entry_init!( + ("class", CLASS_OBJECT.clone()), + ("class", Value::new_class("oauth2_resource_server")), + ("class", Value::new_class("oauth2_resource_server_basic")), + ("uuid", Value::Uuid(rs_uuid)), + ("oauth2_rs_name", Value::new_iname("test_resource_server")), + ("displayname", Value::new_utf8s("test_resource_server")), + ( + "oauth2_rs_origin", + Value::new_url_s("https://demo.example.com").unwrap() + ) + ) + .into_sealed_committed() + }; + + let ev2 = unsafe { + entry_init!( + ("class", CLASS_OBJECT.clone()), + ("class", Value::new_class("oauth2_resource_server")), + ("class", Value::new_class("oauth2_resource_server_basic")), + ("uuid", Value::Uuid(Uuid::new_v4())), + ("oauth2_rs_name", Value::new_iname("second_resource_server")), + ("displayname", Value::new_utf8s("second_resource_server")), + ( + "oauth2_rs_origin", + Value::new_url_s("https://noaccess.example.com").unwrap() + ), + ( + "oauth2_rs_scope_map", + Value::new_oauthscopemap(UUID_SYSTEM_ADMINS, btreeset!["groups".to_string()]) + .expect("invalid oauthscope") + ), + ( + "oauth2_rs_sup_scope_map", + Value::new_oauthscopemap( + // This is NOT the scope map that is access checked! + UUID_TEST_GROUP_1, + btreeset!["supplement".to_string()] + ) + .expect("invalid oauthscope") + ), + ( + "oauth2_allow_insecure_client_disable_pkce", + Value::new_bool(true) + ), + ("oauth2_jwt_legacy_crypto_enable", Value::new_bool(false)), + ("oauth2_prefer_short_username", Value::new_bool(false)) + ) + .into_sealed_committed() + }; + + let r_set = vec![Arc::new(ev1.clone()), Arc::new(ev2)]; + + let se_a = unsafe { + SearchEvent::new_impersonate_entry( + E_TEST_ACCOUNT_1.clone(), + filter_all!(f_pres("oauth2_rs_name")), + ) + }; + let ex_a = vec![Arc::new(ev1)]; + let ex_a_reduced = vec![ev1_reduced]; + + let se_b = unsafe { + SearchEvent::new_impersonate_entry( + E_TEST_ACCOUNT_2.clone(), + filter_all!(f_pres("oauth2_rs_name")), + ) + }; + let ex_b = vec![]; + + // Check the authorisation search event, and that it reduces correctly. + test_acp_search!(&se_a, vec![], r_set.clone(), ex_a); + test_acp_search_reduce!(&se_a, vec![], r_set.clone(), ex_a_reduced); + + // Check the deny case. + test_acp_search!(&se_b, vec![], r_set, ex_b); + } } diff --git a/kanidmd/lib/src/server/access/search.rs b/kanidmd/lib/src/server/access/search.rs index 289c624b5..58487f411 100644 --- a/kanidmd/lib/src/server/access/search.rs +++ b/kanidmd/lib/src/server/access/search.rs @@ -34,6 +34,14 @@ pub(super) fn apply_search_access<'a>( AccessResult::Allow(mut set) => allow.append(&mut set), }; + match search_oauth2_filter_entry(ident, entry) { + AccessResult::Denied => denied = true, + AccessResult::Grant => grant = true, + AccessResult::Ignore => {} + AccessResult::Constrain(mut set) => constrain.append(&mut set), + AccessResult::Allow(mut set) => allow.append(&mut set), + }; + // We'll add more modules later. // Now finalise the decision. @@ -102,3 +110,41 @@ fn search_filter_entry<'a>( AccessResult::Allow(allowed_attrs) } + +fn search_oauth2_filter_entry<'a>( + ident: &Identity, + entry: &'a Arc, +) -> AccessResult<'a> { + match &ident.origin { + IdentType::Internal | IdentType::Synch(_) => AccessResult::Ignore, + IdentType::User(iuser) => { + if entry + .get_ava_as_iutf8("class") + .map(|set| { + trace!(?set); + set.contains("oauth2_resource_server") + }) + .unwrap_or(false) + { + if entry + .get_ava_as_oauthscopemaps("oauth2_rs_scope_map") + .and_then(|maps| ident.get_memberof().map(|mo| (maps, mo))) + .map(|(maps, mo)| maps.keys().any(|k| mo.contains(k))) + .unwrap_or(false) + { + security_access!(entry = ?entry.get_uuid(), ident = ?iuser.entry.get_uuid2rdn(), "ident is a memberof a group granted an oauth2 scope by this entry"); + + return AccessResult::Allow(btreeset!( + "class", + "displayname", + "uuid", + "oauth2_rs_name", + "oauth2_rs_origin", + "oauth2_rs_origin_landing" + )); + } + } + AccessResult::Ignore + } + } +} diff --git a/kanidmd/lib/src/server/delete.rs b/kanidmd/lib/src/server/delete.rs index ada901c12..0b11bb28e 100644 --- a/kanidmd/lib/src/server/delete.rs +++ b/kanidmd/lib/src/server/delete.rs @@ -156,6 +156,25 @@ impl<'a> QueryServerWriteTransaction<'a> { let de = DeleteEvent::new_internal(f_valid); self.delete(&de) } + + #[instrument(level = "debug", skip_all)] + pub fn internal_delete_uuid_if_exists( + &mut self, + target_uuid: Uuid, + ) -> Result<(), OperationError> { + let filter = filter!(f_eq("uuid", PartialValue::Uuid(target_uuid))); + let f_valid = filter + .validate(self.get_schema()) + .map_err(OperationError::SchemaViolation)?; + + let ee = ExistsEvent::new_internal(f_valid.clone()); + // Submit it + if self.exists(&ee)? { + let de = DeleteEvent::new_internal(f_valid); + self.delete(&de)?; + } + Ok(()) + } } #[cfg(test)] diff --git a/kanidmd/lib/src/server/migrations.rs b/kanidmd/lib/src/server/migrations.rs index d815faffa..522940d8d 100644 --- a/kanidmd/lib/src/server/migrations.rs +++ b/kanidmd/lib/src/server/migrations.rs @@ -502,7 +502,7 @@ impl<'a> QueryServerWriteTransaction<'a> { JSON_IDM_ACP_RADIUS_SECRET_READ_PRIV_V1, JSON_IDM_ACP_RADIUS_SECRET_WRITE_PRIV_V1, JSON_IDM_HP_ACP_SERVICE_ACCOUNT_INTO_PERSON_MIGRATE_V1, - JSON_IDM_ACP_OAUTH2_READ_PRIV_V1, + // JSON_IDM_ACP_OAUTH2_READ_PRIV_V1, JSON_IDM_HP_ACP_SYNC_ACCOUNT_MANAGE_PRIV_V1, ]; @@ -534,6 +534,20 @@ impl<'a> QueryServerWriteTransaction<'a> { debug_assert!(res.is_ok()); res?; + // Delete entries that no longer need to exist. + let delete_entries = [UUID_IDM_ACP_OAUTH2_READ_PRIV_V1]; + + let res: Result<(), _> = delete_entries + .into_iter() + .try_for_each(|entry_uuid| self.internal_delete_uuid_if_exists(entry_uuid)); + if res.is_ok() { + admin_debug!("initialise_idm -> result Ok!"); + } else { + admin_error!(?res, "initialise_idm p3 -> result"); + } + debug_assert!(res.is_ok()); + res?; + self.changed_schema = true; self.changed_acp = true; diff --git a/kanidmd_web_ui/src/oauth2.rs b/kanidmd_web_ui/src/oauth2.rs index b1cf2f09d..5613950a7 100644 --- a/kanidmd_web_ui/src/oauth2.rs +++ b/kanidmd_web_ui/src/oauth2.rs @@ -224,7 +224,10 @@ impl Component for Oauth2App { let query: Option = location .query() .map_err(|e| { - let e_msg = format!("failed to decode authorisation request url parameters -> {:?}", e); + let e_msg = format!( + "failed to decode authorisation request url parameters -> {:?}", + e + ); console::error!(e_msg.as_str()); }) .ok()