From 05afae60c2d3dd95e805ea4a2899513493a1e31d Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Thu, 19 Aug 2021 14:21:18 -0700 Subject: [PATCH] Review comments-- - Change list of attributeParsingOverrides to a map - Add unit test for sAMAccountName as group name without the override - Change some comments in the the type definition. --- ...es_activedirectoryidentityprovider.go.tmpl | 9 +- ....dev_activedirectoryidentityproviders.yaml | 9 +- generated/1.17/README.adoc | 4 +- .../types_activedirectoryidentityprovider.go | 9 +- ....dev_activedirectoryidentityproviders.yaml | 9 +- generated/1.18/README.adoc | 4 +- .../types_activedirectoryidentityprovider.go | 9 +- ....dev_activedirectoryidentityproviders.yaml | 9 +- generated/1.19/README.adoc | 4 +- .../types_activedirectoryidentityprovider.go | 9 +- ....dev_activedirectoryidentityproviders.yaml | 9 +- generated/1.20/README.adoc | 4 +- .../types_activedirectoryidentityprovider.go | 9 +- ....dev_activedirectoryidentityproviders.yaml | 9 +- .../types_activedirectoryidentityprovider.go | 9 +- .../active_directory_upstream_watcher.go | 6 +- .../active_directory_upstream_watcher_test.go | 101 ++++++++++++++---- internal/upstreamldap/upstreamldap.go | 38 +++---- internal/upstreamldap/upstreamldap_test.go | 96 +++-------------- 19 files changed, 183 insertions(+), 173 deletions(-) diff --git a/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go.tmpl index e8e9fb26..5889bef6 100644 --- a/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go.tmpl @@ -47,9 +47,9 @@ type ActiveDirectoryIdentityProviderBind struct { type ActiveDirectoryIdentityProviderUserSearchAttributes struct { // Username specifies the name of the attribute in Active Directory entry whose value shall become the username - // of the user after a successful authentication. This would typically be the same attribute name used in - // Optional, when empty this defaults to "userPrincipalName". - // +optional + // of the user after a successful authentication. + // Optional, when empty this defaults to "userPrincipalName". + // +optional Username string `json:"username,omitempty"` // UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely @@ -108,6 +108,9 @@ type ActiveDirectoryIdentityProviderGroupSearch struct { // Optional. When not specified, the default will act as if the filter were specified as // "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". // This searches nested groups by default. + // Note that nested group search can be slow for some Active Directory servers. To disable it, + // you can set the filter to + // "(&(objectClass=group)(member={})" // +optional Filter string `json:"filter,omitempty"` diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml index 83eb8d2a..87f37a02 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml @@ -109,7 +109,9 @@ spec: Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". - This searches nested groups by default. + This searches nested groups by default. Note that nested group + search can be slow for some Active Directory servers. To disable + it, you can set the filter to "(&(objectClass=group)(member={})" type: string type: object host: @@ -145,9 +147,8 @@ spec: username: description: Username specifies the name of the attribute in Active Directory entry whose value shall become the username - of the user after a successful authentication. This would - typically be the same attribute name used in Optional, when - empty this defaults to "userPrincipalName". + of the user after a successful authentication. Optional, + when empty this defaults to "userPrincipalName". type: string type: object base: diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index fbd14cf3..bd077fba 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -799,7 +799,7 @@ ActiveDirectoryIdentityProvider describes the configuration of an upstream Micro |=== | Field | Description | *`base`* __string__ | Base is the dn (distinguished name) that should be used as the search base when searching for groups. E.g. "ou=groups,dc=example,dc=com". Optional, when not specified it will be based on the result of a query for the default naming context. -| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. +| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. Note that nested group search can be slow for some Active Directory servers. To disable it, you can set the filter to "(&(objectClass=group)(member={})" | *`attributes`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-activedirectoryidentityprovidergroupsearchattributes[$$ActiveDirectoryIdentityProviderGroupSearchAttributes$$]__ | Attributes specifies how the group's information should be read from each ActiveDirectory entry which was found as the result of the group search. |=== @@ -894,7 +894,7 @@ Status of an Active Directory identity provider. [cols="25a,75a", options="header"] |=== | Field | Description -| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. This would typically be the same attribute name used in Optional, when empty this defaults to "userPrincipalName". +| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. Optional, when empty this defaults to "userPrincipalName". | *`uid`* __string__ | UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely identify the user within this ActiveDirectory provider after a successful authentication. Optional, when empty this defaults to "objectGUID". |=== diff --git a/generated/1.17/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go index e8e9fb26..5889bef6 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go @@ -47,9 +47,9 @@ type ActiveDirectoryIdentityProviderBind struct { type ActiveDirectoryIdentityProviderUserSearchAttributes struct { // Username specifies the name of the attribute in Active Directory entry whose value shall become the username - // of the user after a successful authentication. This would typically be the same attribute name used in - // Optional, when empty this defaults to "userPrincipalName". - // +optional + // of the user after a successful authentication. + // Optional, when empty this defaults to "userPrincipalName". + // +optional Username string `json:"username,omitempty"` // UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely @@ -108,6 +108,9 @@ type ActiveDirectoryIdentityProviderGroupSearch struct { // Optional. When not specified, the default will act as if the filter were specified as // "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". // This searches nested groups by default. + // Note that nested group search can be slow for some Active Directory servers. To disable it, + // you can set the filter to + // "(&(objectClass=group)(member={})" // +optional Filter string `json:"filter,omitempty"` diff --git a/generated/1.17/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml b/generated/1.17/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml index 83eb8d2a..87f37a02 100644 --- a/generated/1.17/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml +++ b/generated/1.17/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml @@ -109,7 +109,9 @@ spec: Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". - This searches nested groups by default. + This searches nested groups by default. Note that nested group + search can be slow for some Active Directory servers. To disable + it, you can set the filter to "(&(objectClass=group)(member={})" type: string type: object host: @@ -145,9 +147,8 @@ spec: username: description: Username specifies the name of the attribute in Active Directory entry whose value shall become the username - of the user after a successful authentication. This would - typically be the same attribute name used in Optional, when - empty this defaults to "userPrincipalName". + of the user after a successful authentication. Optional, + when empty this defaults to "userPrincipalName". type: string type: object base: diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index b3f341b9..92630ade 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -799,7 +799,7 @@ ActiveDirectoryIdentityProvider describes the configuration of an upstream Micro |=== | Field | Description | *`base`* __string__ | Base is the dn (distinguished name) that should be used as the search base when searching for groups. E.g. "ou=groups,dc=example,dc=com". Optional, when not specified it will be based on the result of a query for the default naming context. -| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. +| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. Note that nested group search can be slow for some Active Directory servers. To disable it, you can set the filter to "(&(objectClass=group)(member={})" | *`attributes`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-activedirectoryidentityprovidergroupsearchattributes[$$ActiveDirectoryIdentityProviderGroupSearchAttributes$$]__ | Attributes specifies how the group's information should be read from each ActiveDirectory entry which was found as the result of the group search. |=== @@ -894,7 +894,7 @@ Status of an Active Directory identity provider. [cols="25a,75a", options="header"] |=== | Field | Description -| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. This would typically be the same attribute name used in Optional, when empty this defaults to "userPrincipalName". +| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. Optional, when empty this defaults to "userPrincipalName". | *`uid`* __string__ | UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely identify the user within this ActiveDirectory provider after a successful authentication. Optional, when empty this defaults to "objectGUID". |=== diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go index e8e9fb26..5889bef6 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go @@ -47,9 +47,9 @@ type ActiveDirectoryIdentityProviderBind struct { type ActiveDirectoryIdentityProviderUserSearchAttributes struct { // Username specifies the name of the attribute in Active Directory entry whose value shall become the username - // of the user after a successful authentication. This would typically be the same attribute name used in - // Optional, when empty this defaults to "userPrincipalName". - // +optional + // of the user after a successful authentication. + // Optional, when empty this defaults to "userPrincipalName". + // +optional Username string `json:"username,omitempty"` // UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely @@ -108,6 +108,9 @@ type ActiveDirectoryIdentityProviderGroupSearch struct { // Optional. When not specified, the default will act as if the filter were specified as // "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". // This searches nested groups by default. + // Note that nested group search can be slow for some Active Directory servers. To disable it, + // you can set the filter to + // "(&(objectClass=group)(member={})" // +optional Filter string `json:"filter,omitempty"` diff --git a/generated/1.18/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml b/generated/1.18/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml index 83eb8d2a..87f37a02 100644 --- a/generated/1.18/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml +++ b/generated/1.18/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml @@ -109,7 +109,9 @@ spec: Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". - This searches nested groups by default. + This searches nested groups by default. Note that nested group + search can be slow for some Active Directory servers. To disable + it, you can set the filter to "(&(objectClass=group)(member={})" type: string type: object host: @@ -145,9 +147,8 @@ spec: username: description: Username specifies the name of the attribute in Active Directory entry whose value shall become the username - of the user after a successful authentication. This would - typically be the same attribute name used in Optional, when - empty this defaults to "userPrincipalName". + of the user after a successful authentication. Optional, + when empty this defaults to "userPrincipalName". type: string type: object base: diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index 3fd892a1..4985760c 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -799,7 +799,7 @@ ActiveDirectoryIdentityProvider describes the configuration of an upstream Micro |=== | Field | Description | *`base`* __string__ | Base is the dn (distinguished name) that should be used as the search base when searching for groups. E.g. "ou=groups,dc=example,dc=com". Optional, when not specified it will be based on the result of a query for the default naming context. -| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. +| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. Note that nested group search can be slow for some Active Directory servers. To disable it, you can set the filter to "(&(objectClass=group)(member={})" | *`attributes`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-activedirectoryidentityprovidergroupsearchattributes[$$ActiveDirectoryIdentityProviderGroupSearchAttributes$$]__ | Attributes specifies how the group's information should be read from each ActiveDirectory entry which was found as the result of the group search. |=== @@ -894,7 +894,7 @@ Status of an Active Directory identity provider. [cols="25a,75a", options="header"] |=== | Field | Description -| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. This would typically be the same attribute name used in Optional, when empty this defaults to "userPrincipalName". +| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. Optional, when empty this defaults to "userPrincipalName". | *`uid`* __string__ | UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely identify the user within this ActiveDirectory provider after a successful authentication. Optional, when empty this defaults to "objectGUID". |=== diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go index e8e9fb26..5889bef6 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go @@ -47,9 +47,9 @@ type ActiveDirectoryIdentityProviderBind struct { type ActiveDirectoryIdentityProviderUserSearchAttributes struct { // Username specifies the name of the attribute in Active Directory entry whose value shall become the username - // of the user after a successful authentication. This would typically be the same attribute name used in - // Optional, when empty this defaults to "userPrincipalName". - // +optional + // of the user after a successful authentication. + // Optional, when empty this defaults to "userPrincipalName". + // +optional Username string `json:"username,omitempty"` // UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely @@ -108,6 +108,9 @@ type ActiveDirectoryIdentityProviderGroupSearch struct { // Optional. When not specified, the default will act as if the filter were specified as // "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". // This searches nested groups by default. + // Note that nested group search can be slow for some Active Directory servers. To disable it, + // you can set the filter to + // "(&(objectClass=group)(member={})" // +optional Filter string `json:"filter,omitempty"` diff --git a/generated/1.19/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml b/generated/1.19/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml index 83eb8d2a..87f37a02 100644 --- a/generated/1.19/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml +++ b/generated/1.19/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml @@ -109,7 +109,9 @@ spec: Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". - This searches nested groups by default. + This searches nested groups by default. Note that nested group + search can be slow for some Active Directory servers. To disable + it, you can set the filter to "(&(objectClass=group)(member={})" type: string type: object host: @@ -145,9 +147,8 @@ spec: username: description: Username specifies the name of the attribute in Active Directory entry whose value shall become the username - of the user after a successful authentication. This would - typically be the same attribute name used in Optional, when - empty this defaults to "userPrincipalName". + of the user after a successful authentication. Optional, + when empty this defaults to "userPrincipalName". type: string type: object base: diff --git a/generated/1.20/README.adoc b/generated/1.20/README.adoc index 1e7060a2..40d1db7c 100644 --- a/generated/1.20/README.adoc +++ b/generated/1.20/README.adoc @@ -799,7 +799,7 @@ ActiveDirectoryIdentityProvider describes the configuration of an upstream Micro |=== | Field | Description | *`base`* __string__ | Base is the dn (distinguished name) that should be used as the search base when searching for groups. E.g. "ou=groups,dc=example,dc=com". Optional, when not specified it will be based on the result of a query for the default naming context. -| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. +| *`filter`* __string__ | Filter is the ActiveDirectory search filter which should be applied when searching for groups for a user. The pattern "{}" must occur in the filter at least once and will be dynamically replaced by the dn (distinguished name) of the user entry found as a result of the user search. E.g. "member={}" or "&(objectClass=groupOfNames)(member={})". For more information about ActiveDirectory filters, see https://ldap.com/ldap-filters. Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". This searches nested groups by default. Note that nested group search can be slow for some Active Directory servers. To disable it, you can set the filter to "(&(objectClass=group)(member={})" | *`attributes`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-activedirectoryidentityprovidergroupsearchattributes[$$ActiveDirectoryIdentityProviderGroupSearchAttributes$$]__ | Attributes specifies how the group's information should be read from each ActiveDirectory entry which was found as the result of the group search. |=== @@ -894,7 +894,7 @@ Status of an Active Directory identity provider. [cols="25a,75a", options="header"] |=== | Field | Description -| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. This would typically be the same attribute name used in Optional, when empty this defaults to "userPrincipalName". +| *`username`* __string__ | Username specifies the name of the attribute in Active Directory entry whose value shall become the username of the user after a successful authentication. Optional, when empty this defaults to "userPrincipalName". | *`uid`* __string__ | UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely identify the user within this ActiveDirectory provider after a successful authentication. Optional, when empty this defaults to "objectGUID". |=== diff --git a/generated/1.20/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go b/generated/1.20/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go index e8e9fb26..5889bef6 100644 --- a/generated/1.20/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go +++ b/generated/1.20/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go @@ -47,9 +47,9 @@ type ActiveDirectoryIdentityProviderBind struct { type ActiveDirectoryIdentityProviderUserSearchAttributes struct { // Username specifies the name of the attribute in Active Directory entry whose value shall become the username - // of the user after a successful authentication. This would typically be the same attribute name used in - // Optional, when empty this defaults to "userPrincipalName". - // +optional + // of the user after a successful authentication. + // Optional, when empty this defaults to "userPrincipalName". + // +optional Username string `json:"username,omitempty"` // UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely @@ -108,6 +108,9 @@ type ActiveDirectoryIdentityProviderGroupSearch struct { // Optional. When not specified, the default will act as if the filter were specified as // "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". // This searches nested groups by default. + // Note that nested group search can be slow for some Active Directory servers. To disable it, + // you can set the filter to + // "(&(objectClass=group)(member={})" // +optional Filter string `json:"filter,omitempty"` diff --git a/generated/1.20/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml b/generated/1.20/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml index 83eb8d2a..87f37a02 100644 --- a/generated/1.20/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml +++ b/generated/1.20/crds/idp.supervisor.pinniped.dev_activedirectoryidentityproviders.yaml @@ -109,7 +109,9 @@ spec: Note that the dn (distinguished name) is not an attribute of an entry, so "dn={}" cannot be used. Optional. When not specified, the default will act as if the filter were specified as "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". - This searches nested groups by default. + This searches nested groups by default. Note that nested group + search can be slow for some Active Directory servers. To disable + it, you can set the filter to "(&(objectClass=group)(member={})" type: string type: object host: @@ -145,9 +147,8 @@ spec: username: description: Username specifies the name of the attribute in Active Directory entry whose value shall become the username - of the user after a successful authentication. This would - typically be the same attribute name used in Optional, when - empty this defaults to "userPrincipalName". + of the user after a successful authentication. Optional, + when empty this defaults to "userPrincipalName". type: string type: object base: diff --git a/generated/latest/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go b/generated/latest/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go index e8e9fb26..5889bef6 100644 --- a/generated/latest/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go +++ b/generated/latest/apis/supervisor/idp/v1alpha1/types_activedirectoryidentityprovider.go @@ -47,9 +47,9 @@ type ActiveDirectoryIdentityProviderBind struct { type ActiveDirectoryIdentityProviderUserSearchAttributes struct { // Username specifies the name of the attribute in Active Directory entry whose value shall become the username - // of the user after a successful authentication. This would typically be the same attribute name used in - // Optional, when empty this defaults to "userPrincipalName". - // +optional + // of the user after a successful authentication. + // Optional, when empty this defaults to "userPrincipalName". + // +optional Username string `json:"username,omitempty"` // UID specifies the name of the attribute in the ActiveDirectory entry which whose value shall be used to uniquely @@ -108,6 +108,9 @@ type ActiveDirectoryIdentityProviderGroupSearch struct { // Optional. When not specified, the default will act as if the filter were specified as // "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={})". // This searches nested groups by default. + // Note that nested group search can be slow for some Active Directory servers. To disable it, + // you can set the filter to + // "(&(objectClass=group)(member={})" // +optional Filter string `json:"filter,omitempty"` diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index abaeda98..c32793d9 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -8,6 +8,8 @@ import ( "context" "fmt" + "github.com/go-ldap/ldap/v3" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -313,11 +315,11 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, GroupNameAttribute: adUpstreamImpl.Spec().GroupSearch().GroupNameAttribute(), }, Dialer: c.ldapDialer, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, } if spec.GroupSearch.Attributes.GroupName == "" { - config.GroupAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{{AttributeName: defaultActiveDirectoryGroupNameAttributeName, OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}} + config.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){defaultActiveDirectoryGroupNameAttributeName: upstreamldap.GroupSAMAccountNameWithDomainSuffix} } conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &adUpstreamImpl, c.secretInformer, c.validatedSecretVersionsCache, config) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go index 2f5cada9..a2326ac2 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -218,7 +218,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, } // Make a copy with targeted changes. @@ -533,7 +533,62 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, + }, + }, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + bindSecretValidTrueCondition(1234), + activeDirectoryConnectionValidTrueCondition(1234, "4242"), + searchBaseFoundInConfigCondition(1234), + { + Type: "TLSConfigurationValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "no TLS configuration provided", + ObservedGeneration: 1234, + }, + }, + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, + }, + { + name: "sAMAccountName explicitly provided as group name attribute does not add an override", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Spec.TLS = nil + upstream.Spec.GroupSearch.Attributes.GroupName = "sAMAccountName" + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + setupMocks: func(conn *mockldapconn.MockConn) { + // Should perform a test dial and bind. + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantResultingCache: []*upstreamldap.ProviderConfig{ + { + Name: testName, + Host: testHost, + ConnectionProtocol: upstreamldap.TLS, + CABundle: nil, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: upstreamldap.UserSearchConfig{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + GroupSearch: upstreamldap.GroupSearchConfig{ + Base: testGroupSearchBase, + Filter: testGroupSearchFilter, + GroupNameAttribute: "sAMAccountName", + }, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -591,7 +646,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -649,7 +704,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantErr: controllerlib.ErrSyntheticRequeue.Error(), @@ -706,7 +761,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -830,7 +885,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -950,7 +1005,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1000,7 +1055,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1190,8 +1245,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))", GroupNameAttribute: "sAMAccountName", }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, - GroupAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "sAMAccountName", OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, + GroupAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"sAMAccountName": upstreamldap.GroupSAMAccountNameWithDomainSuffix}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1241,7 +1296,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1290,7 +1345,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1339,7 +1394,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, + UIDAttributeParsingOverrides: map[string]func(*ldap.Entry) (string, error){"objectGUID": upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1580,25 +1635,25 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { expectedUIDAttributeParsingOverrides := copyOfExpectedValueForResultingCache.UIDAttributeParsingOverrides actualConfig := actualIDP.GetConfig() actualUIDAttributeParsingOverrides := actualConfig.UIDAttributeParsingOverrides - copyOfExpectedValueForResultingCache.UIDAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{} - actualConfig.UIDAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{} + copyOfExpectedValueForResultingCache.UIDAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){} + actualConfig.UIDAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){} require.Equal(t, len(expectedUIDAttributeParsingOverrides), len(actualUIDAttributeParsingOverrides)) - for i := range expectedUIDAttributeParsingOverrides { - require.Equal(t, expectedUIDAttributeParsingOverrides[i].AttributeName, actualUIDAttributeParsingOverrides[i].AttributeName) - require.Equal(t, reflect.ValueOf(expectedUIDAttributeParsingOverrides[i].OverrideFunc).Pointer(), reflect.ValueOf(actualUIDAttributeParsingOverrides[i].OverrideFunc).Pointer()) + for k, v := range expectedUIDAttributeParsingOverrides { + require.NotNil(t, actualUIDAttributeParsingOverrides[k]) + require.Equal(t, reflect.ValueOf(v).Pointer(), reflect.ValueOf(actualUIDAttributeParsingOverrides[k]).Pointer()) } // function equality is awkward. Do the check for equality separately from the rest of the config. expectedGroupAttributeParsingOverrides := copyOfExpectedValueForResultingCache.GroupAttributeParsingOverrides actualGroupAttributeParsingOverrides := actualConfig.GroupAttributeParsingOverrides - copyOfExpectedValueForResultingCache.GroupAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{} - actualConfig.GroupAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{} + copyOfExpectedValueForResultingCache.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){} + actualConfig.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){} require.Equal(t, len(expectedGroupAttributeParsingOverrides), len(actualGroupAttributeParsingOverrides)) - for i := range expectedGroupAttributeParsingOverrides { - require.Equal(t, expectedGroupAttributeParsingOverrides[i].AttributeName, actualGroupAttributeParsingOverrides[i].AttributeName) - require.Equal(t, reflect.ValueOf(expectedGroupAttributeParsingOverrides[i].OverrideFunc).Pointer(), reflect.ValueOf(actualGroupAttributeParsingOverrides[i].OverrideFunc).Pointer()) + for k, v := range expectedGroupAttributeParsingOverrides { + require.NotNil(t, actualGroupAttributeParsingOverrides[k]) + require.Equal(t, reflect.ValueOf(v).Pointer(), reflect.ValueOf(actualGroupAttributeParsingOverrides[k]).Pointer()) } require.Equal(t, copyOfExpectedValueForResultingCache, actualConfig) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 37dab4a8..1a147b43 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -18,9 +18,8 @@ import ( "strings" "time" - "github.com/google/uuid" - "github.com/go-ldap/ldap/v3" + "github.com/google/uuid" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/utils/trace" @@ -38,6 +37,7 @@ const ( groupSearchPageSize = uint32(250) defaultLDAPPort = uint16(389) defaultLDAPSPort = uint16(636) + sAMAccountNameAttribute = "sAMAccountName" ) // Conn abstracts the upstream LDAP communication protocol (mostly for testing). @@ -109,16 +109,11 @@ type ProviderConfig struct { // UIDAttributeParsingOverrides are mappings between an attribute name and a way to parse it as a UID when // it comes out of LDAP. - UIDAttributeParsingOverrides []AttributeParsingOverride + UIDAttributeParsingOverrides map[string]func(*ldap.Entry) (string, error) // GroupNameMappingOverrides are the mappings between an attribute name and a way to parse it as a group // name when it comes out of LDAP. - GroupAttributeParsingOverrides []AttributeParsingOverride -} - -type AttributeParsingOverride struct { - AttributeName string - OverrideFunc func(entry *ldap.Entry) (string, error) + GroupAttributeParsingOverrides map[string]func(*ldap.Entry) (string, error) } // UserSearchConfig contains information about how to search for users in the upstream LDAP IDP. @@ -391,15 +386,13 @@ entries: if len(groupEntry.DN) == 0 { return nil, fmt.Errorf(`searching for group memberships for user with DN %q resulted in search result without DN`, userDN) } - for _, override := range p.c.GroupAttributeParsingOverrides { - if groupAttributeName == override.AttributeName { - overrideGroupName, err := override.OverrideFunc(groupEntry) - if err != nil { - return nil, fmt.Errorf("error finding groups for user %s: %w", userDN, err) - } - groups = append(groups, overrideGroupName) - continue entries + if overrideFunc := p.c.GroupAttributeParsingOverrides[groupAttributeName]; overrideFunc != nil { + overrideGroupName, err := overrideFunc(groupEntry) + if err != nil { + return nil, fmt.Errorf("error finding groups for user %s: %w", userDN, err) } + groups = append(groups, overrideGroupName) + continue entries } // if none of the overrides matched, use the default behavior (no mapping) mappedGroupName, err := p.getSearchResultAttributeValue(groupAttributeName, groupEntry, userDN) @@ -638,10 +631,8 @@ func (p *Provider) getSearchResultAttributeRawValueEncoded(attributeName string, ) } - for _, override := range p.c.UIDAttributeParsingOverrides { - if attributeName == override.AttributeName { - return override.OverrideFunc(entry) - } + if overrideFunc := p.c.UIDAttributeParsingOverrides[attributeName]; overrideFunc != nil { + return overrideFunc(entry) } return base64.RawURLEncoding.EncodeToString(attributeValue), nil @@ -710,7 +701,6 @@ func microsoftUUIDFromBinary(binaryUUID []byte) (string, error) { } func GroupSAMAccountNameWithDomainSuffix(entry *ldap.Entry) (string, error) { - sAMAccountNameAttribute := "sAMAccountName" sAMAccountNameAttributeValues := entry.GetAttributeValues(sAMAccountNameAttribute) if len(sAMAccountNameAttributeValues) != 1 { @@ -734,8 +724,10 @@ func GroupSAMAccountNameWithDomainSuffix(entry *ldap.Entry) (string, error) { return sAMAccountName + "@" + domain, nil } +var domainComponentsRegexp = regexp.MustCompile(",DC=|,dc=") + func getDomainFromDistinguishedName(distinguishedName string) (string, error) { - domainComponents := regexp.MustCompile(",DC=|,dc=").Split(distinguishedName, -1) + domainComponents := domainComponentsRegexp.Split(distinguishedName, -1) if len(domainComponents) == 1 { return "", fmt.Errorf("did not find domain components in group dn: %s", distinguishedName) } diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 59804de3..0cb1f355 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -511,10 +511,9 @@ func TestEndUserAuthentication(t *testing.T) { username: testUpstreamUsername, password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { - p.UIDAttributeParsingOverrides = []AttributeParsingOverride{{ - AttributeName: "objectGUID", - OverrideFunc: MicrosoftUUIDFromBinary("objectGUID"), - }} + p.UIDAttributeParsingOverrides = map[string]func(entry *ldap.Entry) (string, error){ + "objectGUID": MicrosoftUUIDFromBinary("objectGUID"), + } p.UserSearch.UIDAttribute = "objectGUID" }), searchMocks: func(conn *mockldapconn.MockConn) { @@ -547,10 +546,9 @@ func TestEndUserAuthentication(t *testing.T) { username: testUpstreamUsername, password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { - p.UIDAttributeParsingOverrides = []AttributeParsingOverride{{ - AttributeName: "objectGUID", - OverrideFunc: MicrosoftUUIDFromBinary("objectGUID"), - }} + p.UIDAttributeParsingOverrides = map[string]func(entry *ldap.Entry) (string, error){ + "objectGUID": MicrosoftUUIDFromBinary("objectGUID"), + } }), searchMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -570,65 +568,8 @@ func TestEndUserAuthentication(t *testing.T) { password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { p.GroupSearch.GroupNameAttribute = "sAMAccountName" - p.GroupAttributeParsingOverrides = []AttributeParsingOverride{{ - AttributeName: "sAMAccountName", - OverrideFunc: GroupSAMAccountNameWithDomainSuffix, - }} - }), - searchMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) - conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1) - conn.EXPECT().SearchWithPaging(expectedGroupSearch(func(r *ldap.SearchRequest) { - r.Attributes = []string{"sAMAccountName"} - }), expectedGroupSearchPageSize). - Return(&ldap.SearchResult{ - Entries: []*ldap.Entry{ - { - DN: "CN=Mammals,OU=Users,OU=pinniped-ad,DC=activedirectory,DC=mycompany,DC=example,DC=com", - Attributes: []*ldap.EntryAttribute{ - ldap.NewEntryAttribute("sAMAccountName", []string{"Mammals"}), - }, - }, - { - DN: "CN=Animals,OU=Users,OU=pinniped-ad,DC=activedirectory,DC=mycompany,DC=example,DC=com", - Attributes: []*ldap.EntryAttribute{ - ldap.NewEntryAttribute("sAMAccountName", []string{"Animals"}), - }, - }, - }, - Referrals: []string{}, // note that we are not following referrals at this time - Controls: []ldap.Control{}, - }, nil).Times(1) - conn.EXPECT().Close().Times(1) - }, - bindEndUserMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) - }, - wantAuthResponse: expectedAuthResponse(func(r *user.DefaultInfo) { - r.Groups = []string{"Animals@activedirectory.mycompany.example.com", "Mammals@activedirectory.mycompany.example.com"} - }), - }, - { - name: "only the first group override for a given attribute name is applied", - // the choice to only run the first is somewhat arbitrary and likely irrelevant since we only - // have one group override at the moment... - // And as soon as we have starlark attribute mapping it will be obsolete. But this test - // ensures that we - username: testUpstreamUsername, - password: testUpstreamPassword, - providerConfig: providerConfig(func(p *ProviderConfig) { - p.GroupSearch.GroupNameAttribute = "sAMAccountName" - p.GroupAttributeParsingOverrides = []AttributeParsingOverride{ - { - AttributeName: "sAMAccountName", - OverrideFunc: GroupSAMAccountNameWithDomainSuffix, - }, - { - AttributeName: "sAMAccountName", - OverrideFunc: func(entry *ldap.Entry) (string, error) { - return "override-group-name", nil - }, - }, + p.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){ + "sAMAccountName": GroupSAMAccountNameWithDomainSuffix, } }), searchMocks: func(conn *mockldapconn.MockConn) { @@ -670,10 +611,9 @@ func TestEndUserAuthentication(t *testing.T) { password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { p.GroupSearch.GroupNameAttribute = "sAMAccountName" - p.GroupAttributeParsingOverrides = []AttributeParsingOverride{{ - AttributeName: "sAMAccountName", - OverrideFunc: GroupSAMAccountNameWithDomainSuffix, - }} + p.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){ + "sAMAccountName": GroupSAMAccountNameWithDomainSuffix, + } }), searchMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -709,10 +649,9 @@ func TestEndUserAuthentication(t *testing.T) { password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { p.GroupSearch.GroupNameAttribute = "sAMAccountName" - p.GroupAttributeParsingOverrides = []AttributeParsingOverride{{ - AttributeName: "sAMAccountName", - OverrideFunc: GroupSAMAccountNameWithDomainSuffix, - }} + p.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){ + "sAMAccountName": GroupSAMAccountNameWithDomainSuffix, + } }), searchMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -741,10 +680,9 @@ func TestEndUserAuthentication(t *testing.T) { password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { p.GroupSearch.GroupNameAttribute = "sAMAccountName" - p.GroupAttributeParsingOverrides = []AttributeParsingOverride{{ - AttributeName: "sAMAccountName", - OverrideFunc: GroupSAMAccountNameWithDomainSuffix, - }} + p.GroupAttributeParsingOverrides = map[string]func(*ldap.Entry) (string, error){ + "sAMAccountName": GroupSAMAccountNameWithDomainSuffix, + } }), searchMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)