From 3a4cc6414f6cdc4a7ff883e888a934ad33184c5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Natalie=20Klestrup=20R=C3=B6ijezon?= Date: Tue, 25 Feb 2025 13:56:51 +0100 Subject: [PATCH 1/4] Allow AD integration to filter LDAP groups Fixes #691 --- deploy/helm/opa-operator/crds/crds.yaml | 9 ++++ rust/crd/src/user_info_fetcher.rs | 7 +++ .../src/backend/active_directory.rs | 43 ++++++++++++++++--- rust/user-info-fetcher/src/main.rs | 1 + 4 files changed, 54 insertions(+), 6 deletions(-) diff --git a/deploy/helm/opa-operator/crds/crds.yaml b/deploy/helm/opa-operator/crds/crds.yaml index 59bf7630..411e64fc 100644 --- a/deploy/helm/opa-operator/crds/crds.yaml +++ b/deploy/helm/opa-operator/crds/crds.yaml @@ -77,6 +77,15 @@ spec: default: {} description: Custom attributes, and their LDAP attribute names. type: object + customGroupAttributeFilters: + additionalProperties: + type: string + default: {} + description: |- + Attributes that groups must have to be returned. + + These fields will be spliced into an LDAP Search Query, so wildcards can be used, but characters with a special meaning in LDAP will need to be escaped. + type: object kerberosSecretClassName: description: The name of the Kerberos SecretClass. type: string diff --git a/rust/crd/src/user_info_fetcher.rs b/rust/crd/src/user_info_fetcher.rs index c0f81cb0..a371480e 100644 --- a/rust/crd/src/user_info_fetcher.rs +++ b/rust/crd/src/user_info_fetcher.rs @@ -112,6 +112,13 @@ pub struct ActiveDirectoryBackend { /// Custom attributes, and their LDAP attribute names. #[serde(default)] pub custom_attribute_mappings: BTreeMap, + + /// Attributes that groups must have to be returned. + /// + /// These fields will be spliced into an LDAP Search Query, so wildcards can be used, + /// but characters with a special meaning in LDAP will need to be escaped. + #[serde(default)] + pub custom_group_attribute_filters: BTreeMap, } #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] diff --git a/rust/user-info-fetcher/src/backend/active_directory.rs b/rust/user-info-fetcher/src/backend/active_directory.rs index 8f3f44f1..e44cec27 100644 --- a/rust/user-info-fetcher/src/backend/active_directory.rs +++ b/rust/user-info-fetcher/src/backend/active_directory.rs @@ -1,6 +1,6 @@ use std::{ collections::{BTreeMap, HashMap}, - fmt::Display, + fmt::{Display, Write}, io::{Cursor, Read}, num::ParseIntError, str::FromStr, @@ -90,13 +90,19 @@ const LDAP_FIELD_USER_NAME: &str = "userPrincipalName"; const LDAP_FIELD_USER_PRIMARY_GROUP_RID: &str = "primaryGroupID"; const LDAP_FIELD_GROUP_MEMBER: &str = "member"; -#[tracing::instrument(skip(tls, base_distinguished_name, custom_attribute_mappings))] +#[tracing::instrument(skip( + tls, + base_distinguished_name, + custom_attribute_mappings, + group_attribute_filters, +))] pub(crate) async fn get_user_info( request: &UserInfoRequest, ldap_server: &str, tls: &TlsClientDetails, base_distinguished_name: &str, custom_attribute_mappings: &BTreeMap, + group_attribute_filters: &BTreeMap, ) -> Result { let ldap_tls = utils::tls::configure_native_tls(tls) .await @@ -168,16 +174,27 @@ pub(crate) async fn get_user_info( base_distinguished_name, &user, custom_attribute_mappings, + group_attribute_filters, ) .await } -#[tracing::instrument(skip(ldap, base_dn, user, custom_attribute_mappings), fields(user.dn))] +#[tracing::instrument( + skip( + ldap, + base_dn, + user, + custom_attribute_mappings, + group_attribute_filters, + ), + fields(user.dn), +)] async fn user_attributes( ldap: &mut Ldap, base_dn: &str, user: &SearchEntry, custom_attribute_mappings: &BTreeMap, + group_attribute_filters: &BTreeMap, ) -> Result { let user_sid = user .bin_attrs @@ -242,7 +259,8 @@ async fn user_attributes( }) .collect::>(); let groups = if let Some(user_sid) = &user_sid { - user_group_distinguished_names(ldap, base_dn, user, user_sid).await? + user_group_distinguished_names(ldap, base_dn, user, user_sid, group_attribute_filters) + .await? } else { tracing::debug!(user.dn, "user has no SID, cannot fetch groups..."); Vec::new() @@ -257,12 +275,13 @@ async fn user_attributes( } /// Gets the distinguished names of all of `user`'s groups, both primary and secondary. -#[tracing::instrument(skip(ldap, base_dn, user, user_sid))] +#[tracing::instrument(skip(ldap, base_dn, user, user_sid, group_attribute_filters))] async fn user_group_distinguished_names( ldap: &mut Ldap, base_dn: &str, user: &SearchEntry, user_sid: &SecurityId, + group_attribute_filters: &BTreeMap, ) -> Result, Error> { // User group memberships are tricky, because users have exactly one *primary* and any number of *secondary* groups. // Additionally groups can be members of other groups. @@ -309,10 +328,22 @@ async fn user_group_distinguished_names( "({LDAP_FIELD_GROUP_MEMBER}{LDAP_MATCHING_RULE_IN_CHAIN}=)" ); + // Users can also specify custom filters via `group_attribute_filters` + let custom_group_filter = + group_attribute_filters + .iter() + .fold(String::new(), |mut out, (k, v)| { + // NOTE: This is technically an LDAP injection vuln, but these are provided statically by the OPA administrator, + // who would be able to do plenty of other harm... (like providing their own OPA images that do whatever they want). + // We could base64 the value to "defuse" it entirely, but that would also prevent using wildcards. + write!(out, "({k}={v})").expect("string concatenation is infallible"); + out + }); + // Let's put it all together, and make it go... let groups_filter = format!("(|{primary_group_filter}{primary_group_parents_filter}{secondary_groups_filter})"); - let groups_query_filter = format!("(&(objectClass=group){groups_filter})"); + let groups_query_filter = format!("(&(objectClass=group){custom_group_filter}{groups_filter})"); let requested_group_attrs = [LDAP_FIELD_OBJECT_DISTINGUISHED_NAME]; tracing::debug!( groups_query_filter, diff --git a/rust/user-info-fetcher/src/main.rs b/rust/user-info-fetcher/src/main.rs index aedd197f..63f64b57 100644 --- a/rust/user-info-fetcher/src/main.rs +++ b/rust/user-info-fetcher/src/main.rs @@ -298,6 +298,7 @@ async fn get_user_info( &ad.tls, &ad.base_distinguished_name, &ad.custom_attribute_mappings, + &ad.custom_group_attribute_filters, ) .await .context(get_user_info_error::ActiveDirectorySnafu), From 32af10b5ef5d652c5e5a262578eb1f26dab687ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Natalie=20Klestrup=20R=C3=B6ijezon?= Date: Wed, 26 Feb 2025 15:15:49 +0100 Subject: [PATCH 2/4] Rename `customGroupAttributeFilters` to `additionalGroupAttributeFilters` --- deploy/helm/opa-operator/crds/crds.yaml | 18 ++++++------- rust/crd/src/user_info_fetcher.rs | 2 +- .../src/backend/active_directory.rs | 26 ++++++++++++------- rust/user-info-fetcher/src/main.rs | 2 +- 4 files changed, 27 insertions(+), 21 deletions(-) diff --git a/deploy/helm/opa-operator/crds/crds.yaml b/deploy/helm/opa-operator/crds/crds.yaml index 411e64fc..95b0f804 100644 --- a/deploy/helm/opa-operator/crds/crds.yaml +++ b/deploy/helm/opa-operator/crds/crds.yaml @@ -68,6 +68,15 @@ spec: experimentalActiveDirectory: description: Backend that fetches user information from Active Directory properties: + additionalGroupAttributeFilters: + additionalProperties: + type: string + default: {} + description: |- + Attributes that groups must have to be returned. + + These fields will be spliced into an LDAP Search Query, so wildcards can be used, but characters with a special meaning in LDAP will need to be escaped. + type: object baseDistinguishedName: description: The root Distinguished Name (DN) where users and groups are located. type: string @@ -77,15 +86,6 @@ spec: default: {} description: Custom attributes, and their LDAP attribute names. type: object - customGroupAttributeFilters: - additionalProperties: - type: string - default: {} - description: |- - Attributes that groups must have to be returned. - - These fields will be spliced into an LDAP Search Query, so wildcards can be used, but characters with a special meaning in LDAP will need to be escaped. - type: object kerberosSecretClassName: description: The name of the Kerberos SecretClass. type: string diff --git a/rust/crd/src/user_info_fetcher.rs b/rust/crd/src/user_info_fetcher.rs index a371480e..1b5b92d8 100644 --- a/rust/crd/src/user_info_fetcher.rs +++ b/rust/crd/src/user_info_fetcher.rs @@ -118,7 +118,7 @@ pub struct ActiveDirectoryBackend { /// These fields will be spliced into an LDAP Search Query, so wildcards can be used, /// but characters with a special meaning in LDAP will need to be escaped. #[serde(default)] - pub custom_group_attribute_filters: BTreeMap, + pub additional_group_attribute_filters: BTreeMap, } #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] diff --git a/rust/user-info-fetcher/src/backend/active_directory.rs b/rust/user-info-fetcher/src/backend/active_directory.rs index e44cec27..da5fd280 100644 --- a/rust/user-info-fetcher/src/backend/active_directory.rs +++ b/rust/user-info-fetcher/src/backend/active_directory.rs @@ -94,7 +94,7 @@ const LDAP_FIELD_GROUP_MEMBER: &str = "member"; tls, base_distinguished_name, custom_attribute_mappings, - group_attribute_filters, + additional_group_attribute_filters, ))] pub(crate) async fn get_user_info( request: &UserInfoRequest, @@ -102,7 +102,7 @@ pub(crate) async fn get_user_info( tls: &TlsClientDetails, base_distinguished_name: &str, custom_attribute_mappings: &BTreeMap, - group_attribute_filters: &BTreeMap, + additional_group_attribute_filters: &BTreeMap, ) -> Result { let ldap_tls = utils::tls::configure_native_tls(tls) .await @@ -174,7 +174,7 @@ pub(crate) async fn get_user_info( base_distinguished_name, &user, custom_attribute_mappings, - group_attribute_filters, + additional_group_attribute_filters, ) .await } @@ -185,7 +185,7 @@ pub(crate) async fn get_user_info( base_dn, user, custom_attribute_mappings, - group_attribute_filters, + additional_group_attribute_filters, ), fields(user.dn), )] @@ -194,7 +194,7 @@ async fn user_attributes( base_dn: &str, user: &SearchEntry, custom_attribute_mappings: &BTreeMap, - group_attribute_filters: &BTreeMap, + additional_group_attribute_filters: &BTreeMap, ) -> Result { let user_sid = user .bin_attrs @@ -259,8 +259,14 @@ async fn user_attributes( }) .collect::>(); let groups = if let Some(user_sid) = &user_sid { - user_group_distinguished_names(ldap, base_dn, user, user_sid, group_attribute_filters) - .await? + user_group_distinguished_names( + ldap, + base_dn, + user, + user_sid, + additional_group_attribute_filters, + ) + .await? } else { tracing::debug!(user.dn, "user has no SID, cannot fetch groups..."); Vec::new() @@ -275,13 +281,13 @@ async fn user_attributes( } /// Gets the distinguished names of all of `user`'s groups, both primary and secondary. -#[tracing::instrument(skip(ldap, base_dn, user, user_sid, group_attribute_filters))] +#[tracing::instrument(skip(ldap, base_dn, user, user_sid, additional_group_attribute_filters))] async fn user_group_distinguished_names( ldap: &mut Ldap, base_dn: &str, user: &SearchEntry, user_sid: &SecurityId, - group_attribute_filters: &BTreeMap, + additional_group_attribute_filters: &BTreeMap, ) -> Result, Error> { // User group memberships are tricky, because users have exactly one *primary* and any number of *secondary* groups. // Additionally groups can be members of other groups. @@ -330,7 +336,7 @@ async fn user_group_distinguished_names( // Users can also specify custom filters via `group_attribute_filters` let custom_group_filter = - group_attribute_filters + additional_group_attribute_filters .iter() .fold(String::new(), |mut out, (k, v)| { // NOTE: This is technically an LDAP injection vuln, but these are provided statically by the OPA administrator, diff --git a/rust/user-info-fetcher/src/main.rs b/rust/user-info-fetcher/src/main.rs index 63f64b57..80532bef 100644 --- a/rust/user-info-fetcher/src/main.rs +++ b/rust/user-info-fetcher/src/main.rs @@ -298,7 +298,7 @@ async fn get_user_info( &ad.tls, &ad.base_distinguished_name, &ad.custom_attribute_mappings, - &ad.custom_group_attribute_filters, + &ad.additional_group_attribute_filters, ) .await .context(get_user_info_error::ActiveDirectorySnafu), From 510004a744f74fc7447178aa97474bf3fa586627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Natalie=20Klestrup=20R=C3=B6ijezon?= Date: Wed, 26 Feb 2025 15:20:12 +0100 Subject: [PATCH 3/4] Docs --- .../opa/pages/usage-guide/user-info-fetcher.adoc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/user-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/user-info-fetcher.adoc index 6c9f90ba..101b722f 100644 --- a/docs/modules/opa/pages/usage-guide/user-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/user-info-fetcher.adoc @@ -89,12 +89,14 @@ spec: baseDistinguishedName: DC=sble,DC=test # <3> customAttributeMappings: # <4> country: c # <5> - kerberosSecretClassName: kerberos-ad # <6> + additionalGroupAttributeFilters: # <6> + foo: bar + kerberosSecretClassName: kerberos-ad # <7> tls: verification: server: caCert: - secretClass: tls-ad # <7> + secretClass: tls-ad # <8> cache: # optional, enabled by default entryTimeToLive: 60s # optional, defaults to 60s ---- @@ -103,8 +105,9 @@ spec: <3> The distinguished name to search, users and groups outside of this will not be seen <4> Arbitrary LDAP attributes can be requested to be fetched <5> https://learn.microsoft.com/en-us/windows/win32/ad/address-book-properties[`c`] stores the ISO-3166 country code of the user -<6> The name of the SecretClass that knows how to create Kerberos keytabs trusted by Active Directory -<7> The name of the SecretClass that contains the Active Directory's root CA certificate(s) +<6> Groups can be filtered by LDAP attributes to reduce the load in searching for membership. `*` can be used as a wildcard in these filters. +<7> The name of the SecretClass that knows how to create Kerberos keytabs trusted by Active Directory +<8> The name of the SecretClass that contains the Active Directory's root CA certificate(s) == User info fetcher API From 7240daeccb72a15058634115e181e825c31e90fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Natalie=20Klestrup=20R=C3=B6ijezon?= Date: Wed, 26 Feb 2025 15:21:45 +0100 Subject: [PATCH 4/4] Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbb150bb..766992d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ All notable changes to this project will be documented in this file. - Run a `containerdebug` process in the background of each OPA container to collect debugging information ([#666]). - Added support for OPA `1.0.0` ([#677]). - Aggregate emitted Kubernetes events on the CustomResources ([#675]). +- Added support for filtering groups searched by Active Directory ([#693]). ### Removed @@ -22,6 +23,7 @@ All notable changes to this project will be documented in this file. [#677]: https://github.com/stackabletech/opa-operator/pull/677 [#671]: https://github.com/stackabletech/opa-operator/pull/671 [#675]: https://github.com/stackabletech/opa-operator/pull/675 +[#693]: https://github.com/stackabletech/opa-operator/pull/693 ## [24.11.1] - 2025-01-10