From cedbe82bbb5bdfb4fa0bc14587b4ece583e5a868 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 28 May 2021 13:27:11 -0700 Subject: [PATCH] Default `groupSearch.attributes.groupName` to "dn" instead of "cn" - DNs are more unique than CNs, so it feels like a safer default --- .../types_ldapidentityprovider.go.tmpl | 4 +-- ...or.pinniped.dev_ldapidentityproviders.yaml | 8 +++--- generated/1.17/README.adoc | 2 +- .../v1alpha1/types_ldapidentityprovider.go | 4 +-- ...or.pinniped.dev_ldapidentityproviders.yaml | 8 +++--- generated/1.18/README.adoc | 2 +- .../v1alpha1/types_ldapidentityprovider.go | 4 +-- ...or.pinniped.dev_ldapidentityproviders.yaml | 8 +++--- generated/1.19/README.adoc | 2 +- .../v1alpha1/types_ldapidentityprovider.go | 4 +-- ...or.pinniped.dev_ldapidentityproviders.yaml | 8 +++--- generated/1.20/README.adoc | 2 +- .../v1alpha1/types_ldapidentityprovider.go | 4 +-- ...or.pinniped.dev_ldapidentityproviders.yaml | 8 +++--- .../v1alpha1/types_ldapidentityprovider.go | 4 +-- internal/upstreamldap/upstreamldap.go | 5 ++-- internal/upstreamldap/upstreamldap_test.go | 27 +++++++++++++++++-- test/integration/e2e_test.go | 4 +-- test/integration/ldap_client_test.go | 18 +++++++++++-- 19 files changed, 81 insertions(+), 45 deletions(-) diff --git a/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl index 0e28234d..74570a92 100644 --- a/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl @@ -68,8 +68,8 @@ type LDAPIdentityProviderGroupSearchAttributes struct { // GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name // in the user's list of groups after a successful authentication. // The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP - // server in the user's entry. Distinguished names can be used by specifying lower-case "dn". - // Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). + // server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". + // Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). // +optional GroupName string `json:"groupName,omitempty"` } diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 46fbe1d0..ed9780b5 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -86,10 +86,10 @@ spec: in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server - in the user's entry. Distinguished names can be used by - specifying lower-case "dn". Optional. When not specified, - the default will act as if the GroupName were specified - as "cn" (common name). + in the user's entry. E.g. "cn" for common name. Distinguished + names can be used by specifying lower-case "dn". Optional. + When not specified, the default will act as if the GroupName + were specified as "dn" (distinguished name). type: string type: object base: diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index 58890ee6..e9637cbb 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -852,7 +852,7 @@ LDAPIdentityProvider describes the configuration of an upstream Lightweight Dire [cols="25a,75a", options="header"] |=== | Field | Description -| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). +| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). |=== diff --git a/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 0e28234d..74570a92 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -68,8 +68,8 @@ type LDAPIdentityProviderGroupSearchAttributes struct { // GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name // in the user's list of groups after a successful authentication. // The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP - // server in the user's entry. Distinguished names can be used by specifying lower-case "dn". - // Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). + // server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". + // Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). // +optional GroupName string `json:"groupName,omitempty"` } diff --git a/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 46fbe1d0..ed9780b5 100644 --- a/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -86,10 +86,10 @@ spec: in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server - in the user's entry. Distinguished names can be used by - specifying lower-case "dn". Optional. When not specified, - the default will act as if the GroupName were specified - as "cn" (common name). + in the user's entry. E.g. "cn" for common name. Distinguished + names can be used by specifying lower-case "dn". Optional. + When not specified, the default will act as if the GroupName + were specified as "dn" (distinguished name). type: string type: object base: diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index 70b74f90..3fbd9d46 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -852,7 +852,7 @@ LDAPIdentityProvider describes the configuration of an upstream Lightweight Dire [cols="25a,75a", options="header"] |=== | Field | Description -| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). +| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). |=== diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 0e28234d..74570a92 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -68,8 +68,8 @@ type LDAPIdentityProviderGroupSearchAttributes struct { // GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name // in the user's list of groups after a successful authentication. // The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP - // server in the user's entry. Distinguished names can be used by specifying lower-case "dn". - // Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). + // server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". + // Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). // +optional GroupName string `json:"groupName,omitempty"` } diff --git a/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 46fbe1d0..ed9780b5 100644 --- a/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -86,10 +86,10 @@ spec: in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server - in the user's entry. Distinguished names can be used by - specifying lower-case "dn". Optional. When not specified, - the default will act as if the GroupName were specified - as "cn" (common name). + in the user's entry. E.g. "cn" for common name. Distinguished + names can be used by specifying lower-case "dn". Optional. + When not specified, the default will act as if the GroupName + were specified as "dn" (distinguished name). type: string type: object base: diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index 104274ca..48e0b834 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -852,7 +852,7 @@ LDAPIdentityProvider describes the configuration of an upstream Lightweight Dire [cols="25a,75a", options="header"] |=== | Field | Description -| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). +| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). |=== diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 0e28234d..74570a92 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -68,8 +68,8 @@ type LDAPIdentityProviderGroupSearchAttributes struct { // GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name // in the user's list of groups after a successful authentication. // The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP - // server in the user's entry. Distinguished names can be used by specifying lower-case "dn". - // Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). + // server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". + // Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). // +optional GroupName string `json:"groupName,omitempty"` } diff --git a/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 46fbe1d0..ed9780b5 100644 --- a/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -86,10 +86,10 @@ spec: in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server - in the user's entry. Distinguished names can be used by - specifying lower-case "dn". Optional. When not specified, - the default will act as if the GroupName were specified - as "cn" (common name). + in the user's entry. E.g. "cn" for common name. Distinguished + names can be used by specifying lower-case "dn". Optional. + When not specified, the default will act as if the GroupName + were specified as "dn" (distinguished name). type: string type: object base: diff --git a/generated/1.20/README.adoc b/generated/1.20/README.adoc index 005bcc7c..4df9c3d1 100644 --- a/generated/1.20/README.adoc +++ b/generated/1.20/README.adoc @@ -852,7 +852,7 @@ LDAPIdentityProvider describes the configuration of an upstream Lightweight Dire [cols="25a,75a", options="header"] |=== | Field | Description -| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). +| *`groupName`* __string__ | GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). |=== diff --git a/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 0e28234d..74570a92 100644 --- a/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -68,8 +68,8 @@ type LDAPIdentityProviderGroupSearchAttributes struct { // GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name // in the user's list of groups after a successful authentication. // The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP - // server in the user's entry. Distinguished names can be used by specifying lower-case "dn". - // Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). + // server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". + // Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). // +optional GroupName string `json:"groupName,omitempty"` } diff --git a/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 46fbe1d0..ed9780b5 100644 --- a/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -86,10 +86,10 @@ spec: in the user's list of groups after a successful authentication. The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP server - in the user's entry. Distinguished names can be used by - specifying lower-case "dn". Optional. When not specified, - the default will act as if the GroupName were specified - as "cn" (common name). + in the user's entry. E.g. "cn" for common name. Distinguished + names can be used by specifying lower-case "dn". Optional. + When not specified, the default will act as if the GroupName + were specified as "dn" (distinguished name). type: string type: object base: diff --git a/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 0e28234d..74570a92 100644 --- a/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -68,8 +68,8 @@ type LDAPIdentityProviderGroupSearchAttributes struct { // GroupName specifies the name of the attribute in the LDAP entries whose value shall become a group name // in the user's list of groups after a successful authentication. // The value of this field is case-sensitive and must match the case of the attribute name returned by the LDAP - // server in the user's entry. Distinguished names can be used by specifying lower-case "dn". - // Optional. When not specified, the default will act as if the GroupName were specified as "cn" (common name). + // server in the user's entry. E.g. "cn" for common name. Distinguished names can be used by specifying lower-case "dn". + // Optional. When not specified, the default will act as if the GroupName were specified as "dn" (distinguished name). // +optional GroupName string `json:"groupName,omitempty"` } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 11f800eb..95c68915 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -31,7 +31,6 @@ import ( const ( ldapsScheme = "ldaps" distinguishedNameAttributeName = "dn" - commonNameAttributeName = "cn" searchFilterInterpolationLocationMarker = "{}" groupSearchPageSize = uint32(250) defaultLDAPPort = uint16(389) @@ -367,7 +366,7 @@ func (p *Provider) searchGroupsForUserDN(conn Conn, userDN string) ([]string, er groupAttributeName := p.c.GroupSearch.GroupNameAttribute if len(groupAttributeName) == 0 { - groupAttributeName = commonNameAttributeName + groupAttributeName = distinguishedNameAttributeName } groups := []string{} @@ -493,7 +492,7 @@ func (p *Provider) userSearchRequestedAttributes() []string { func (p *Provider) groupSearchRequestedAttributes() []string { switch p.c.GroupSearch.GroupNameAttribute { case "": - return []string{commonNameAttributeName} + return []string{} case distinguishedNameAttributeName: return []string{} default: diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 35609bcf..75f1c718 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -315,6 +315,29 @@ func TestEndUserAuthentication(t *testing.T) { r.UID = base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultDNValue)) }), }, + { + name: "when the GroupNameAttribute is empty then it defaults to dn", + username: testUpstreamUsername, + password: testUpstreamPassword, + providerConfig: providerConfig(func(p *ProviderConfig) { + p.GroupSearch.GroupNameAttribute = "" // blank means to use dn + }), + 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{} + }), expectedGroupSearchPageSize). + Return(exampleGroupSearchResult, 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{testGroupSearchResultDNValue1, testGroupSearchResultDNValue2} + }), + }, { name: "when the GroupNameAttribute is dn", username: testUpstreamUsername, @@ -339,11 +362,11 @@ func TestEndUserAuthentication(t *testing.T) { }), }, { - name: "when the GroupNameAttribute is empty then it defaults to cn", + name: "when the GroupNameAttribute is cn", username: testUpstreamUsername, password: testUpstreamPassword, providerConfig: providerConfig(func(p *ProviderConfig) { - p.GroupSearch.GroupNameAttribute = "" // blank means to use cn + p.GroupSearch.GroupNameAttribute = "cn" }), searchMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 98c72ec0..e2a1be60 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -278,7 +278,7 @@ func TestE2EFullIntegration(t *testing.T) { // Add an LDAP upstream IDP and try using it to authenticate during kubectl commands. t.Run("with Supervisor LDAP upstream IDP", func(t *testing.T) { expectedUsername := env.SupervisorUpstreamLDAP.TestUserMailAttributeValue - expectedGroups := env.SupervisorUpstreamLDAP.TestUserDirectGroupsCNs + expectedGroups := env.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs // Create a ClusterRoleBinding to give our test user from the upstream read-only access to the cluster. library.CreateTestClusterRoleBinding(t, @@ -321,7 +321,7 @@ func TestE2EFullIntegration(t *testing.T) { Base: env.SupervisorUpstreamLDAP.GroupSearchBase, Filter: "", // use the default value of "member={}" Attributes: idpv1alpha1.LDAPIdentityProviderGroupSearchAttributes{ - GroupName: "", // use the default value of "cn" + GroupName: "", // use the default value of "dn" }, }, }, idpv1alpha1.LDAPPhaseReady) diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 5e4735c3..b8a02cdd 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -243,6 +243,20 @@ func TestLDAPSearch(t *testing.T) { }}, }, }, + { + name: "using the default group name attribute, which is dn", + username: "pinny", + password: pinnyPassword, + provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { + p.GroupSearch.GroupNameAttribute = "" + })), + wantAuthResponse: &authenticator.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{ + "cn=ball-game-players,ou=beach-groups,ou=groups,dc=pinniped,dc=dev", + "cn=seals,ou=groups,dc=pinniped,dc=dev", + }}, + }, + }, { name: "using some other custom group name attribute", username: "pinny", @@ -675,8 +689,8 @@ func defaultProviderConfig(env *library.TestEnv, port string) *upstreamldap.Prov }, GroupSearch: upstreamldap.GroupSearchConfig{ Base: "ou=groups,dc=pinniped,dc=dev", - Filter: "", // defaults to member={} - GroupNameAttribute: "", // defaults to cn + Filter: "", // defaults to member={} + GroupNameAttribute: "cn", // defaults to dn, but here we set it to cn }, } }