From 1bff3a92d84d892602c20ab86ac4ee9fe62817d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 24 Jul 2020 15:03:20 +0200 Subject: [PATCH] Split LDAP user filters (#996) --- changelog/unreleased/split-ldap-filters.md | 25 ++++ pkg/auth/manager/ldap/ldap.go | 40 ++++-- pkg/user/manager/ldap/ldap.go | 148 +++++++++++++-------- 3 files changed, 151 insertions(+), 62 deletions(-) create mode 100644 changelog/unreleased/split-ldap-filters.md diff --git a/changelog/unreleased/split-ldap-filters.md b/changelog/unreleased/split-ldap-filters.md new file mode 100644 index 0000000000..8d43b65280 --- /dev/null +++ b/changelog/unreleased/split-ldap-filters.md @@ -0,0 +1,25 @@ +Enhancement: Split LDAP user filters + +The current LDAP user and auth filters only allow a single `%s` to be replaced with the relevant string. +The current `userfilter` is used to lookup a single user, search for share recipients and for login. To make each use case more flexible we split this in three and introduced templates. + +For the `userfilter` we moved to filter templates that can use the CS3 user id properties `{{.OpaqueId}}` and `{{.Idp}}`: +``` +userfilter = "(&(objectclass=posixAccount)(|(ownclouduuid={{.OpaqueId}})(cn={{.OpaqueId}})))" +``` + +We introduced a new `findfilter` that is used when searching for users. Use it like this: +``` +findfilter = "(&(objectclass=posixAccount)(|(cn={{query}}*)(displayname={{query}}*)(mail={{query}}*)))" +``` + +Furthermore, we also introduced a dedicated login filter for the LDAP auth manager: +``` +loginfilter = "(&(objectclass=posixAccount)(|(cn={{login}})(mail={{login}})))" +``` + +These filter changes are backward compatible: `findfilter` and `loginfilter` will be derived from the `userfilter` by replacing `%s` with `{{query}}` and `{{login}}` respectively. The `userfilter` replaces `%s` with `{{.OpaqueId}}` + +Finally, we changed the default attribute for the immutable uid of a user to `ms-DS-ConsistencyGuid`. See https://docs.microsoft.com/en-us/azure/active-directory/hybrid/plan-connect-design-concepts for the background. You can fall back to `objectguid` or even `samaccountname` but you will run into trouble when user names change. You have been warned. + +https://github.com/cs3org/reva/pull/996 diff --git a/pkg/auth/manager/ldap/ldap.go b/pkg/auth/manager/ldap/ldap.go index 28cedd9db6..ce742106c3 100644 --- a/pkg/auth/manager/ldap/ldap.go +++ b/pkg/auth/manager/ldap/ldap.go @@ -22,12 +22,14 @@ import ( "context" "crypto/tls" "fmt" + "strings" user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/auth" "github.com/cs3org/reva/pkg/auth/manager/registry" "github.com/cs3org/reva/pkg/errtypes" + "github.com/cs3org/reva/pkg/logger" "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "gopkg.in/ldap.v2" @@ -46,6 +48,7 @@ type config struct { Port int `mapstructure:"port"` BaseDN string `mapstructure:"base_dn"` UserFilter string `mapstructure:"userfilter"` + LoginFilter string `mapstructure:"loginfilter"` BindUsername string `mapstructure:"bind_username"` BindPassword string `mapstructure:"bind_password"` Idp string `mapstructure:"idp"` @@ -53,18 +56,25 @@ type config struct { } type attributes struct { - DN string `mapstructure:"dn"` - UID string `mapstructure:"uid"` - Mail string `mapstructure:"mail"` + // DN is the distinguished name in ldap, e.g. `cn=einstein,ou=users,dc=example,dc=org` + DN string `mapstructure:"dn"` + // UID is an immutable user id, see https://docs.microsoft.com/en-us/azure/active-directory/hybrid/plan-connect-design-concepts + UID string `mapstructure:"uid"` + // CN is the username, typically `cn`, `uid` or `samaccountname` + CN string `mapstructure:"cn"` + // Mail is the email address of a user + Mail string `mapstructure:"mail"` + // Displayname is the Human readable name, e.g. `Albert Einstein` DisplayName string `mapstructure:"displayName"` } // Default attributes (Active Directory) var ldapDefaults = attributes{ + DN: "dn", + UID: "ms-DS-ConsistencyGuid", // you can fall back to objectguid or even samaccountname but you will run into trouble when user names change. You have been warned. + CN: "cn", Mail: "mail", - UID: "objectGUID", DisplayName: "displayName", - DN: "dn", } func parseConfig(m map[string]interface{}) (*config, error) { @@ -85,6 +95,15 @@ func New(m map[string]interface{}) (auth.Manager, error) { return nil, err } + // backwards compatibility + if c.UserFilter != "" { + logger.New().Warn().Msg("userfilter is deprecated, use a loginfilter like `(&(objectclass=posixAccount)(|(cn={{login}}))(mail={{login}}))`") + } + if c.LoginFilter == "" { + c.LoginFilter = c.UserFilter + c.LoginFilter = strings.ReplaceAll(c.LoginFilter, "%s", "{{login}}") + } + return &mgr{ c: c, }, nil @@ -109,9 +128,8 @@ func (am *mgr) Authenticate(ctx context.Context, clientID, clientSecret string) searchRequest := ldap.NewSearchRequest( am.c.BaseDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, - fmt.Sprintf(am.c.UserFilter, clientID), - // TODO(jfd): objectguid, entryuuid etc ... make configurable - []string{am.c.Schema.DN, am.c.Schema.UID, am.c.Schema.Mail, am.c.Schema.DisplayName}, + am.getLoginFilter(clientID), + []string{am.c.Schema.DN, am.c.Schema.UID, am.c.Schema.CN, am.c.Schema.Mail, am.c.Schema.DisplayName}, nil, ) @@ -140,7 +158,7 @@ func (am *mgr) Authenticate(ctx context.Context, clientID, clientSecret string) OpaqueId: sr.Entries[0].GetAttributeValue(am.c.Schema.UID), }, // TODO add more claims from the StandardClaims, eg EmailVerified - Username: sr.Entries[0].GetAttributeValue(am.c.Schema.UID), + Username: sr.Entries[0].GetAttributeValue(am.c.Schema.CN), // TODO groups Groups: []string{}, Mail: sr.Entries[0].GetAttributeValue(am.c.Schema.Mail), @@ -150,3 +168,7 @@ func (am *mgr) Authenticate(ctx context.Context, clientID, clientSecret string) return u, nil } + +func (am *mgr) getLoginFilter(login string) string { + return strings.ReplaceAll(am.c.LoginFilter, "{{login}}", login) +} diff --git a/pkg/user/manager/ldap/ldap.go b/pkg/user/manager/ldap/ldap.go index f6231cb7fe..6bf161f220 100644 --- a/pkg/user/manager/ldap/ldap.go +++ b/pkg/user/manager/ldap/ldap.go @@ -19,10 +19,14 @@ package ldap import ( + "bytes" "context" "crypto/tls" "fmt" + "strings" + "text/template" + "github.com/Masterminds/sprig" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/errtypes" @@ -38,15 +42,9 @@ func init() { } type manager struct { - hostname string - port int - baseDN string - userfilter string - groupfilter string - bindUsername string - bindPassword string - idp string - schema attributes + c *config + userfilter *template.Template + groupfilter *template.Template } type config struct { @@ -54,6 +52,7 @@ type config struct { Port int `mapstructure:"port"` BaseDN string `mapstructure:"base_dn"` UserFilter string `mapstructure:"userfilter"` + FindFilter string `mapstructure:"findfilter"` GroupFilter string `mapstructure:"groupfilter"` BindUsername string `mapstructure:"bind_username"` BindPassword string `mapstructure:"bind_password"` @@ -62,20 +61,25 @@ type config struct { } type attributes struct { - Mail string `mapstructure:"mail"` - UID string `mapstructure:"uid"` + // DN is the distinguished name in ldap, e.g. `cn=einstein,ou=users,dc=example,dc=org` + DN string `mapstructure:"dn"` + // UID is an immutable user id, see https://docs.microsoft.com/en-us/azure/active-directory/hybrid/plan-connect-design-concepts + UID string `mapstructure:"uid"` + // CN is the username, typically `cn`, `uid` or `samaccountname` + CN string `mapstructure:"cn"` + // Mail is the email address of a user + Mail string `mapstructure:"mail"` + // Displayname is the Human readable name, e.g. `Albert Einstein` DisplayName string `mapstructure:"displayName"` - DN string `mapstructure:"dn"` - CN string `mapstructure:"cn"` } // Default attributes (Active Directory) var ldapDefaults = attributes{ - Mail: "mail", - UID: "objectGUID", - DisplayName: "displayName", DN: "dn", + UID: "ms-DS-ConsistencyGuid", // you can fall back to objectguid or even samaccountname but you will run into trouble when user names change. You have been warned. CN: "cn", + Mail: "mail", + DisplayName: "displayName", } func parseConfig(m map[string]interface{}) (*config, error) { @@ -97,39 +101,51 @@ func New(m map[string]interface{}) (user.Manager, error) { return nil, err } - return &manager{ - hostname: c.Hostname, - port: c.Port, - baseDN: c.BaseDN, - userfilter: c.UserFilter, - groupfilter: c.GroupFilter, - bindUsername: c.BindUsername, - bindPassword: c.BindPassword, - idp: c.Idp, - schema: c.Schema, - }, nil + // backwards compatibility + c.UserFilter = strings.ReplaceAll(c.UserFilter, "%s", "{{.OpaqueId}}") + if c.FindFilter == "" { + c.FindFilter = c.UserFilter + } + c.GroupFilter = strings.ReplaceAll(c.GroupFilter, "%s", "{{.OpaqueId}}") + + mgr := &manager{ + c: c, + } + + mgr.userfilter, err = template.New("uf").Funcs(sprig.TxtFuncMap()).Parse(c.UserFilter) + if err != nil { + err := errors.Wrap(err, fmt.Sprintf("error parsing userfilter tpl:%s", c.UserFilter)) + panic(err) + } + mgr.groupfilter, err = template.New("gf").Funcs(sprig.TxtFuncMap()).Parse(c.GroupFilter) + if err != nil { + err := errors.Wrap(err, fmt.Sprintf("error parsing groupfilter tpl:%s", c.GroupFilter)) + panic(err) + } + + return mgr, nil } func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) { log := appctx.GetLogger(ctx) - l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.hostname, m.port), &tls.Config{InsecureSkipVerify: true}) + l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true}) if err != nil { return nil, err } defer l.Close() // First bind with a read only user - err = l.Bind(m.bindUsername, m.bindPassword) + err = l.Bind(m.c.BindUsername, m.c.BindPassword) if err != nil { return nil, err } // Search for the given clientID searchRequest := ldap.NewSearchRequest( - m.baseDN, + m.c.BaseDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, - fmt.Sprintf(m.userfilter, uid.OpaqueId), // TODO this is screaming for errors if filter contains >1 %s - []string{m.schema.DN, m.schema.UID, m.schema.Mail, m.schema.DisplayName}, + m.getUserFilter(uid), + []string{m.c.Schema.DN, m.c.Schema.UID, m.c.Schema.CN, m.c.Schema.Mail, m.c.Schema.DisplayName}, nil, ) @@ -145,8 +161,8 @@ func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User log.Debug().Interface("entries", sr.Entries).Msg("entries") id := &userpb.UserId{ - Idp: m.idp, - OpaqueId: sr.Entries[0].GetAttributeValue(m.schema.UID), + Idp: m.c.Idp, + OpaqueId: sr.Entries[0].GetAttributeValue(m.c.Schema.UID), } groups, err := m.GetUserGroups(ctx, id) if err != nil { @@ -154,34 +170,34 @@ func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User } u := &userpb.User{ Id: id, - Username: sr.Entries[0].GetAttributeValue(m.schema.UID), + Username: sr.Entries[0].GetAttributeValue(m.c.Schema.CN), Groups: groups, - Mail: sr.Entries[0].GetAttributeValue(m.schema.Mail), - DisplayName: sr.Entries[0].GetAttributeValue(m.schema.DisplayName), + Mail: sr.Entries[0].GetAttributeValue(m.c.Schema.Mail), + DisplayName: sr.Entries[0].GetAttributeValue(m.c.Schema.DisplayName), } return u, nil } func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) { - l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.hostname, m.port), &tls.Config{InsecureSkipVerify: true}) + l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true}) if err != nil { return nil, err } defer l.Close() // First bind with a read only user - err = l.Bind(m.bindUsername, m.bindPassword) + err = l.Bind(m.c.BindUsername, m.c.BindPassword) if err != nil { return nil, err } // Search for the given clientID searchRequest := ldap.NewSearchRequest( - m.baseDN, + m.c.BaseDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, - fmt.Sprintf(m.userfilter, query), // TODO this is screaming for errors if filter contains >1 %s - []string{m.schema.DN, m.schema.UID, m.schema.Mail, m.schema.DisplayName}, + m.getFindFilter(query), + []string{m.c.Schema.DN, m.c.Schema.UID, m.c.Schema.CN, m.c.Schema.Mail, m.c.Schema.DisplayName}, nil, ) @@ -194,8 +210,8 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, for _, entry := range sr.Entries { id := &userpb.UserId{ - Idp: m.idp, - OpaqueId: entry.GetAttributeValue(m.schema.UID), + Idp: m.c.Idp, + OpaqueId: entry.GetAttributeValue(m.c.Schema.UID), } groups, err := m.GetUserGroups(ctx, id) if err != nil { @@ -203,10 +219,10 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, } user := &userpb.User{ Id: id, - Username: entry.GetAttributeValue(m.schema.UID), + Username: entry.GetAttributeValue(m.c.Schema.CN), Groups: groups, - Mail: entry.GetAttributeValue(m.schema.Mail), - DisplayName: entry.GetAttributeValue(m.schema.DisplayName), + Mail: entry.GetAttributeValue(m.c.Schema.Mail), + DisplayName: entry.GetAttributeValue(m.c.Schema.DisplayName), } users = append(users, user) } @@ -215,24 +231,24 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, } func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) { - l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.hostname, m.port), &tls.Config{InsecureSkipVerify: true}) + l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true}) if err != nil { return []string{}, err } defer l.Close() // First bind with a read only user - err = l.Bind(m.bindUsername, m.bindPassword) + err = l.Bind(m.c.BindUsername, m.c.BindPassword) if err != nil { return []string{}, err } // Search for the given clientID searchRequest := ldap.NewSearchRequest( - m.baseDN, + m.c.BaseDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, - fmt.Sprintf(m.groupfilter, uid.OpaqueId), // TODO this is screaming for errors if filter contains >1 %s - []string{m.schema.CN}, + m.getGroupFilter(uid), + []string{m.c.Schema.CN}, // TODO use DN to look up group id nil, ) @@ -244,13 +260,17 @@ func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]stri groups := []string{} for _, entry := range sr.Entries { - groups = append(groups, entry.GetAttributeValue(m.schema.CN)) + // FIXME this makes the users groups use the cn, not an immutable id + // FIXME 1. use the memberof or members attribute of a user to get the groups + // FIXME 2. ook up the id for each group + groups = append(groups, entry.GetAttributeValue(m.c.Schema.CN)) } return groups, nil } func (m *manager) IsInGroup(ctx context.Context, uid *userpb.UserId, group string) (bool, error) { + // TODO implement with dedicated ldap query groups, err := m.GetUserGroups(ctx, uid) if err != nil { return false, err @@ -264,3 +284,25 @@ func (m *manager) IsInGroup(ctx context.Context, uid *userpb.UserId, group strin return false, nil } + +func (m *manager) getUserFilter(uid *userpb.UserId) string { + b := bytes.Buffer{} + if err := m.userfilter.Execute(&b, uid); err != nil { + err := errors.Wrap(err, fmt.Sprintf("error executing user template: userid:%+v", uid)) + panic(err) + } + return b.String() +} + +func (m *manager) getFindFilter(query string) string { + return strings.ReplaceAll(m.c.FindFilter, "{{query}}", query) +} + +func (m *manager) getGroupFilter(uid *userpb.UserId) string { + b := bytes.Buffer{} + if err := m.groupfilter.Execute(&b, uid); err != nil { + err := errors.Wrap(err, fmt.Sprintf("error executing group template: userid:%+v", uid)) + panic(err) + } + return b.String() +}