From 51c6a1478663bcf98031e0608a14fc8a9361e92b Mon Sep 17 00:00:00 2001 From: sdimovv <36302090+sdimovv@users.noreply.github.com> Date: Wed, 22 Feb 2023 15:18:22 +0200 Subject: [PATCH] providers/ldap: Improve compatibility with LDAP clients (#4750) * Fixed invalid LDAP attributes by replacing '.'s and '/'s with '-' * Leave old fields for now for backward compatibility * Add forgotten depreceated field * Fix tests * Fix tests * use shorter attribute names Signed-off-by: Jens Langhammer * sanitize attributes Signed-off-by: Jens Langhammer * keep both sanitized and unsanitized user fields Signed-off-by: Jens Langhammer * add sanitized fields to test Signed-off-by: Jens Langhammer --------- Signed-off-by: Jens Langhammer Co-authored-by: Jens Langhammer --- internal/outpost/ldap/entries.go | 28 +++++++++++------- internal/outpost/ldap/group/group.go | 19 +++++++----- internal/outpost/ldap/utils/utils.go | 13 +++++++- internal/outpost/ldap/utils/utils_test.go | 36 +++++++++++------------ tests/e2e/test_provider_ldap.py | 16 ++++++++++ website/docs/providers/ldap/index.md | 13 ++++++-- 6 files changed, 86 insertions(+), 39 deletions(-) diff --git a/internal/outpost/ldap/entries.go b/internal/outpost/ldap/entries.go index 7b9ca96de..44b18cbd6 100644 --- a/internal/outpost/ldap/entries.go +++ b/internal/outpost/ldap/entries.go @@ -11,7 +11,9 @@ import ( func (pi *ProviderInstance) UserEntry(u api.User) *ldap.Entry { dn := pi.GetUserDN(u.Username) - attrs := utils.AKAttrsToLDAP(u.Attributes) + attrs := utils.AttributesToLDAP(u.Attributes, false) + sanitizedAttrs := utils.AttributesToLDAP(u.Attributes, true) + attrs = append(attrs, sanitizedAttrs...) if u.IsActive == nil { u.IsActive = api.PtrBool(false) @@ -20,18 +22,22 @@ func (pi *ProviderInstance) UserEntry(u api.User) *ldap.Entry { u.Email = api.PtrString("") } attrs = utils.EnsureAttributes(attrs, map[string][]string{ - "memberOf": pi.GroupsForUser(u), + // Old fields for backwards compatibility "goauthentik.io/ldap/active": {strconv.FormatBool(*u.IsActive)}, "goauthentik.io/ldap/superuser": {strconv.FormatBool(u.IsSuperuser)}, - "cn": {u.Username}, - "sAMAccountName": {u.Username}, - "uid": {u.Uid}, - "name": {u.Name}, - "displayName": {u.Name}, - "mail": {*u.Email}, - "objectClass": {constants.OCUser, constants.OCOrgPerson, constants.OCInetOrgPerson, constants.OCAKUser}, - "uidNumber": {pi.GetUidNumber(u)}, - "gidNumber": {pi.GetUidNumber(u)}, + // End old fields + "ak-active": {strconv.FormatBool(*u.IsActive)}, + "ak-superuser": {strconv.FormatBool(u.IsSuperuser)}, + "memberOf": pi.GroupsForUser(u), + "cn": {u.Username}, + "sAMAccountName": {u.Username}, + "uid": {u.Uid}, + "name": {u.Name}, + "displayName": {u.Name}, + "mail": {*u.Email}, + "objectClass": {constants.OCUser, constants.OCOrgPerson, constants.OCInetOrgPerson, constants.OCAKUser}, + "uidNumber": {pi.GetUidNumber(u)}, + "gidNumber": {pi.GetUidNumber(u)}, }) return &ldap.Entry{DN: dn, Attributes: attrs} } diff --git a/internal/outpost/ldap/group/group.go b/internal/outpost/ldap/group/group.go index 1e942e57e..cef60056f 100644 --- a/internal/outpost/ldap/group/group.go +++ b/internal/outpost/ldap/group/group.go @@ -22,7 +22,9 @@ type LDAPGroup struct { } func (lg *LDAPGroup) Entry() *ldap.Entry { - attrs := utils.AKAttrsToLDAP(lg.AKAttributes) + attrs := utils.AttributesToLDAP(lg.AKAttributes, false) + sanitizedAttrs := utils.AttributesToLDAP(lg.AKAttributes, true) + attrs = append(attrs, sanitizedAttrs...) objectClass := []string{constants.OCGroup, constants.OCGroupOfUniqueNames, constants.OCGroupOfNames, constants.OCAKGroup} if lg.IsVirtualGroup { @@ -30,13 +32,16 @@ func (lg *LDAPGroup) Entry() *ldap.Entry { } attrs = utils.EnsureAttributes(attrs, map[string][]string{ - "objectClass": objectClass, - "member": lg.Member, + // Old fields for backwards compatibility "goauthentik.io/ldap/superuser": {strconv.FormatBool(lg.IsSuperuser)}, - "cn": {lg.CN}, - "uid": {lg.Uid}, - "sAMAccountName": {lg.CN}, - "gidNumber": {lg.GidNumber}, + // End old fields + "ak-superuser": {strconv.FormatBool(lg.IsSuperuser)}, + "objectClass": objectClass, + "member": lg.Member, + "cn": {lg.CN}, + "uid": {lg.Uid}, + "sAMAccountName": {lg.CN}, + "gidNumber": {lg.GidNumber}, }) return &ldap.Entry{DN: lg.DN, Attributes: attrs} } diff --git a/internal/outpost/ldap/utils/utils.go b/internal/outpost/ldap/utils/utils.go index 4c1c4db66..bb44a1381 100644 --- a/internal/outpost/ldap/utils/utils.go +++ b/internal/outpost/ldap/utils/utils.go @@ -9,6 +9,14 @@ import ( ldapConstants "goauthentik.io/internal/outpost/ldap/constants" ) +func AttributeKeySanitize(key string) string { + return strings.ReplaceAll( + strings.ReplaceAll(key, "/", "-"), + ".", + "", + ) +} + func stringify(in interface{}) *string { switch t := in.(type) { case string: @@ -36,13 +44,16 @@ func stringify(in interface{}) *string { } } -func AKAttrsToLDAP(attrs map[string]interface{}) []*ldap.EntryAttribute { +func AttributesToLDAP(attrs map[string]interface{}, sanitize bool) []*ldap.EntryAttribute { attrList := []*ldap.EntryAttribute{} if attrs == nil { return attrList } for attrKey, attrValue := range attrs { entry := &ldap.EntryAttribute{Name: attrKey} + if sanitize { + entry.Name = AttributeKeySanitize(attrKey) + } switch t := attrValue.(type) { case []string: entry.Values = t diff --git a/internal/outpost/ldap/utils/utils_test.go b/internal/outpost/ldap/utils/utils_test.go index 6131eee08..13b4cdc86 100644 --- a/internal/outpost/ldap/utils/utils_test.go +++ b/internal/outpost/ldap/utils/utils_test.go @@ -19,16 +19,16 @@ func TestAKAttrsToLDAP_String(t *testing.T) { u.Attributes = map[string]interface{}{ "foo": "bar", } - assert.Equal(t, 1, len(AKAttrsToLDAP(u.Attributes))) - assert.Equal(t, "foo", AKAttrsToLDAP(u.Attributes)[0].Name) - assert.Equal(t, []string{"bar"}, AKAttrsToLDAP(u.Attributes)[0].Values) + assert.Equal(t, 1, len(AttributesToLDAP(u.Attributes, true))) + assert.Equal(t, "foo", AttributesToLDAP(u.Attributes, true)[0].Name) + assert.Equal(t, []string{"bar"}, AttributesToLDAP(u.Attributes, true)[0].Values) // pointer string u.Attributes = map[string]interface{}{ "foo": api.PtrString("bar"), } - assert.Equal(t, 1, len(AKAttrsToLDAP(u.Attributes))) - assert.Equal(t, "foo", AKAttrsToLDAP(u.Attributes)[0].Name) - assert.Equal(t, []string{"bar"}, AKAttrsToLDAP(u.Attributes)[0].Values) + assert.Equal(t, 1, len(AttributesToLDAP(u.Attributes, true))) + assert.Equal(t, "foo", AttributesToLDAP(u.Attributes, true)[0].Name) + assert.Equal(t, []string{"bar"}, AttributesToLDAP(u.Attributes, true)[0].Values) } func TestAKAttrsToLDAP_String_List(t *testing.T) { @@ -37,16 +37,16 @@ func TestAKAttrsToLDAP_String_List(t *testing.T) { u.Attributes = map[string]interface{}{ "foo": []string{"bar"}, } - assert.Equal(t, 1, len(AKAttrsToLDAP(u.Attributes))) - assert.Equal(t, "foo", AKAttrsToLDAP(u.Attributes)[0].Name) - assert.Equal(t, []string{"bar"}, AKAttrsToLDAP(u.Attributes)[0].Values) + assert.Equal(t, 1, len(AttributesToLDAP(u.Attributes, true))) + assert.Equal(t, "foo", AttributesToLDAP(u.Attributes, true)[0].Name) + assert.Equal(t, []string{"bar"}, AttributesToLDAP(u.Attributes, true)[0].Values) // pointer string list u.Attributes = map[string]interface{}{ "foo": &[]string{"bar"}, } - assert.Equal(t, 1, len(AKAttrsToLDAP(u.Attributes))) - assert.Equal(t, "foo", AKAttrsToLDAP(u.Attributes)[0].Name) - assert.Equal(t, []string{"bar"}, AKAttrsToLDAP(u.Attributes)[0].Values) + assert.Equal(t, 1, len(AttributesToLDAP(u.Attributes, true))) + assert.Equal(t, "foo", AttributesToLDAP(u.Attributes, true)[0].Name) + assert.Equal(t, []string{"bar"}, AttributesToLDAP(u.Attributes, true)[0].Values) } func TestAKAttrsToLDAP_Dict(t *testing.T) { @@ -56,9 +56,9 @@ func TestAKAttrsToLDAP_Dict(t *testing.T) { "foo": "bar", }, } - assert.Equal(t, 1, len(AKAttrsToLDAP(d))) - assert.Equal(t, "foo", AKAttrsToLDAP(d)[0].Name) - assert.Equal(t, []string{"map[foo:bar]"}, AKAttrsToLDAP(d)[0].Values) + assert.Equal(t, 1, len(AttributesToLDAP(d, true))) + assert.Equal(t, "foo", AttributesToLDAP(d, true)[0].Name) + assert.Equal(t, []string{"map[foo:bar]"}, AttributesToLDAP(d, true)[0].Values) } func TestAKAttrsToLDAP_Mixed(t *testing.T) { @@ -69,7 +69,7 @@ func TestAKAttrsToLDAP_Mixed(t *testing.T) { 6, }, } - assert.Equal(t, 1, len(AKAttrsToLDAP(d))) - assert.Equal(t, "foo", AKAttrsToLDAP(d)[0].Name) - assert.Equal(t, []string{"foo", "6"}, AKAttrsToLDAP(d)[0].Values) + assert.Equal(t, 1, len(AttributesToLDAP(d, true))) + assert.Equal(t, "foo", AttributesToLDAP(d, true)[0].Name) + assert.Equal(t, []string{"foo", "6"}, AttributesToLDAP(d, true)[0].Values) } diff --git a/tests/e2e/test_provider_ldap.py b/tests/e2e/test_provider_ldap.py index 49f45cd90..b5dc1551e 100644 --- a/tests/e2e/test_provider_ldap.py +++ b/tests/e2e/test_provider_ldap.py @@ -225,10 +225,16 @@ class TestProviderLDAP(SeleniumTestCase): "uidNumber": [str(2000 + o_user.pk)], "gidNumber": [str(2000 + o_user.pk)], "memberOf": [], + # Old fields for backwards compatibility "goauthentik.io/ldap/active": ["true"], "goauthentik.io/ldap/superuser": ["false"], "goauthentik.io/user/override-ips": ["true"], "goauthentik.io/user/service-account": ["true"], + # End old fields + "ak-active": ["true"], + "ak-superuser": ["false"], + "goauthentikio-user-override-ips": ["true"], + "goauthentikio-user-service-account": ["true"], }, "type": "searchResEntry", }, @@ -250,10 +256,16 @@ class TestProviderLDAP(SeleniumTestCase): "uidNumber": [str(2000 + embedded_account.pk)], "gidNumber": [str(2000 + embedded_account.pk)], "memberOf": [], + # Old fields for backwards compatibility "goauthentik.io/ldap/active": ["true"], "goauthentik.io/ldap/superuser": ["false"], "goauthentik.io/user/override-ips": ["true"], "goauthentik.io/user/service-account": ["true"], + # End old fields + "ak-active": ["true"], + "ak-superuser": ["false"], + "goauthentikio-user-override-ips": ["true"], + "goauthentikio-user-service-account": ["true"], }, "type": "searchResEntry", }, @@ -278,8 +290,12 @@ class TestProviderLDAP(SeleniumTestCase): f"cn={group.name},ou=groups,dc=ldap,dc=goauthentik,dc=io" for group in self.user.ak_groups.all() ], + # Old fields for backwards compatibility "goauthentik.io/ldap/active": ["true"], "goauthentik.io/ldap/superuser": ["true"], + # End old fields + "ak-active": ["true"], + "ak-superuser": ["true"], "extraAttribute": ["bar"], }, "type": "searchResEntry", diff --git a/website/docs/providers/ldap/index.md b/website/docs/providers/ldap/index.md index b9313979c..419352d3a 100644 --- a/website/docs/providers/ldap/index.md +++ b/website/docs/providers/ldap/index.md @@ -29,8 +29,13 @@ The following fields are currently sent for users: - "organizationalPerson" - "goauthentik.io/ldap/user" - `memberOf`: A list of all DNs that the user is a member of -- `goauthentik.io/ldap/active`: "true" if the account is active, otherwise "false" -- `goauthentik.io/ldap/superuser`: "true" if the account is part of a group with superuser permissions, otherwise "false" +- `ak-active`: "true" if the account is active, otherwise "false" +- `ak-superuser`: "true" if the account is part of a group with superuser permissions, otherwise "false" + +:::warning +The use of the `goauthentik.io/ldap/active` and `goauthentik.io/ldap/superuser` attributes is deprecated as of authentik 2023.3. They will be removed completely in a future release. +Use the replacements fields above instead. +::: The following fields are current set for groups: @@ -51,6 +56,10 @@ The virtual groups gidNumber is equal to the uidNumber of the user. Starting with 2021.9.1, custom attributes will override the inbuilt attributes. ::: +:::info +Starting with 2023.3, periods and slashes in custom attributes will be sanitized. +::: + ## SSL You can also configure SSL for your LDAP Providers by selecting a certificate and a server name in the provider settings.