From 8657b0e3e7aedd1d5a7636cc4a71b800ccdc2572 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 18 Aug 2021 10:11:18 -0700 Subject: [PATCH] Cleanup new group attribute behavior and add test coverage --- .../active_directory_upstream_watcher.go | 13 +- .../active_directory_upstream_watcher_test.go | 75 ++--------- internal/upstreamldap/upstreamldap.go | 31 ++++- internal/upstreamldap/upstreamldap_test.go | 121 +++++++++++++++++- 4 files changed, 160 insertions(+), 80 deletions(-) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index b83b8910..abaeda98 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -32,13 +32,12 @@ const ( defaultActiveDirectoryUsernameAttributeName = "userPrincipalName" defaultActiveDirectoryUIDAttributeName = "objectGUID" - // This is not a real attribute in active directory. - // It represents a combined attribute name, sAMAccountName + "@" + domain. + // By default this group name attribute is the sAMAccountName with special mapping. + // Each group will look like sAMAccountName + "@" + domain. // For example if your group sAMAccountName is "mammals" and your domain is // "activedirectory.example.com", it would be mammals@activedirectory.example.com. // This is because sAMAccountName is only unique within a domain, not a forest. - defaultActiveDirectoryGroupNameAttributeName = "sAMAccountName" - defaultActiveDirectoryGroupNameOverrideAttributeName = "pinniped:sAMAccountName@domain" + defaultActiveDirectoryGroupNameAttributeName = "sAMAccountName" // - is a person. // - is not a computer. @@ -188,10 +187,6 @@ func (g *activeDirectoryUpstreamGenericLDAPGroupSearch) GroupNameAttribute() str if len(g.groupSearch.Attributes.GroupName) == 0 { return defaultActiveDirectoryGroupNameAttributeName } - // you explicitly told us to use the override value - if g.groupSearch.Attributes.GroupName == defaultActiveDirectoryGroupNameOverrideAttributeName { - return defaultActiveDirectoryGroupNameAttributeName - } return g.groupSearch.Attributes.GroupName } @@ -321,7 +316,7 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, } - if spec.GroupSearch.Attributes.GroupName == defaultActiveDirectoryGroupNameOverrideAttributeName || spec.GroupSearch.Attributes.GroupName == "" { + if spec.GroupSearch.Attributes.GroupName == "" { config.GroupAttributeParsingOverrides = []upstreamldap.AttributeParsingOverride{{AttributeName: defaultActiveDirectoryGroupNameAttributeName, OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}} } 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 289e0d2f..2f5cada9 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}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, } // Make a copy with targeted changes. @@ -533,7 +533,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -591,7 +591,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -649,7 +649,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantErr: controllerlib.ErrSyntheticRequeue.Error(), @@ -706,7 +706,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -830,7 +830,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -950,7 +950,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1000,7 +1000,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1190,58 +1190,7 @@ 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}}, - GroupAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "sAMAccountName", OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}}, - }, - }, - wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ - ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, - Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ - Phase: "Ready", - Conditions: allConditionsTrue(1234, "4242"), - }, - }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { - BindSecretResourceVersion: "4242", - LDAPConnectionProtocol: upstreamldap.TLS, - UserSearchBase: testUserSearchBase, - GroupSearchBase: testGroupSearchBase, - }}, - }, - { - name: "when the input activedirectoryidentityprovider group search attributes is the special cased pinniped:sAMAccountName@domain value", - inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { - upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{} - upstream.Spec.UserSearch.Filter = "" - upstream.Spec.GroupSearch.Filter = "" - upstream.Spec.GroupSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderGroupSearchAttributes{GroupName: "pinniped:sAMAccountName@domain"} - })}, - 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: testCABundle, - BindUsername: testBindUsername, - BindPassword: testBindPassword, - UserSearch: upstreamldap.UserSearchConfig{ - Base: testUserSearchBase, - Filter: "(&(objectClass=person)(!(objectClass=computer))(!(showInAdvancedViewOnly=TRUE))(|(sAMAccountName={})(mail={})(userPrincipalName={}))(sAMAccountType=805306368))", - UsernameAttribute: "userPrincipalName", - UIDAttribute: "objectGUID", - }, - GroupSearch: upstreamldap.GroupSearchConfig{ - Base: testGroupSearchBase, - Filter: "(&(objectClass=group)(member:1.2.840.113556.1.4.1941:={}))", - GroupNameAttribute: "sAMAccountName", - }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, GroupAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "sAMAccountName", OverrideFunc: upstreamldap.GroupSAMAccountNameWithDomainSuffix}}, }, }, @@ -1292,7 +1241,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1341,7 +1290,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ @@ -1390,7 +1339,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Filter: testGroupSearchFilter, GroupNameAttribute: testGroupNameAttrName, }, - UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary}}, + UIDAttributeParsingOverrides: []upstreamldap.AttributeParsingOverride{{AttributeName: "objectGUID", OverrideFunc: upstreamldap.MicrosoftUUIDFromBinary("objectGUID")}}, }, }, wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index b6e9571a..37dab4a8 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -386,23 +386,26 @@ func (p *Provider) searchGroupsForUserDN(conn Conn, userDN string) ([]string, er } groups := []string{} +entries: for _, groupEntry := range searchResult.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) } - mappedGroupName, err := p.getSearchResultAttributeValue(groupAttributeName, groupEntry, userDN) - if err != nil { - return nil, fmt.Errorf(`error searching for group memberships for user with DN %q: %w`, userDN, err) - } 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: %w", err) + return nil, fmt.Errorf("error finding groups for user %s: %w", userDN, err) } - mappedGroupName = overrideGroupName + 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) + if err != nil { + return nil, fmt.Errorf(`error searching for group memberships for user with DN %q: %w`, userDN, err) + } groups = append(groups, mappedGroupName) } @@ -708,7 +711,21 @@ func microsoftUUIDFromBinary(binaryUUID []byte) (string, error) { func GroupSAMAccountNameWithDomainSuffix(entry *ldap.Entry) (string, error) { sAMAccountNameAttribute := "sAMAccountName" - sAMAccountName := entry.GetAttributeValue(sAMAccountNameAttribute) + sAMAccountNameAttributeValues := entry.GetAttributeValues(sAMAccountNameAttribute) + + if len(sAMAccountNameAttributeValues) != 1 { + return "", fmt.Errorf(`found %d values for attribute "%s", but expected 1 result`, + len(sAMAccountNameAttributeValues), sAMAccountNameAttribute, + ) + } + + sAMAccountName := sAMAccountNameAttributeValues[0] + if len(sAMAccountName) == 0 { + return "", fmt.Errorf(`found empty value for attribute "%s", but expected value to be non-empty`, + sAMAccountNameAttribute, + ) + } + distinguishedName := entry.DN domain, err := getDomainFromDistinguishedName(distinguishedName) if err != nil { diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 641bee40..59804de3 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -608,6 +608,62 @@ func TestEndUserAuthentication(t *testing.T) { 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 + }, + }, + } + }), + 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: "override group parsing when domain can't be determined from dn", username: testUpstreamUsername, @@ -645,7 +701,70 @@ func TestEndUserAuthentication(t *testing.T) { }, nil).Times(1) conn.EXPECT().Close().Times(1) }, - wantError: "error finding groups: did not find domain components in group dn: no-domain-components", + wantError: "error finding groups for user some-upstream-user-dn: did not find domain components in group dn: no-domain-components", + }, + { + name: "override group parsing when entry has multiple values for attribute", + username: testUpstreamUsername, + 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: "no-domain-components", + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute("sAMAccountName", []string{"Mammals", "Eukaryotes"}), + }, + }, + }, + Referrals: []string{}, // note that we are not following referrals at this time + Controls: []ldap.Control{}, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: "error finding groups for user some-upstream-user-dn: found 2 values for attribute \"sAMAccountName\", but expected 1 result", + }, { + name: "override group parsing when entry has no values for attribute", + username: testUpstreamUsername, + 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: "no-domain-components", + Attributes: []*ldap.EntryAttribute{}, + }, + }, + Referrals: []string{}, // note that we are not following referrals at this time + Controls: []ldap.Control{}, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantError: "error finding groups for user some-upstream-user-dn: found 0 values for attribute \"sAMAccountName\", but expected 1 result", }, { name: "when dial fails",