Skip to content

Commit

Permalink
use config as member of manager, also update auth manager
Browse files Browse the repository at this point in the history 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
  • Loading branch information
@butonic
butonic committed Jul 24, 2020
1 parent ed64c21 commit db04e95
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 62 deletions.
20 changes: 17 additions & 3 deletions changelog/unreleased/split-ldap-filters.md
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,25 @@
Enhancement: Split LDAP user filters

The current LDAP user filters only allow a single `%s` to be replaced with the relevant string. We moved to filter templates that can use the CS3 user id properties `{{.OpaqueId}}` and `{{.Idp}}`. Furthermore,
we introduced a new find filter that is used when searching for users. An example config would be
The current LDAP user and auth filters only allow a single `%s` to be replaced with the relevant string.
The current `userfilter` the 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}}*)))"
```
As you can see the userfilter is used to lookup a single user, whereas the findfilter is for a broader search, e.g. used when searching for share recipients.

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
40 changes: 31 additions & 9 deletions pkg/auth/manager/ldap/ldap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -46,25 +48,33 @@ 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"`
Schema attributes `mapstructure:"schema"`
}

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) {
Expand All @@ -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
Expand All @@ -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,
)

Expand Down Expand Up @@ -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),
Expand All @@ -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)
}
90 changes: 40 additions & 50 deletions pkg/user/manager/ldap/ldap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,9 @@ func init() {
}

type manager struct {
hostname string
port int
baseDN string
userfilter *template.Template
findfilter string
groupfilter *template.Template
bindUsername string
bindPassword string
idp string
schema attributes
c *config
userfilter *template.Template
groupfilter *template.Template
}

type config struct {
Expand All @@ -68,21 +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",
// An immutable user id, see https://docs.microsoft.com/en-us/azure/active-directory/hybrid/plan-connect-design-concepts
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.
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) {
Expand Down Expand Up @@ -112,14 +109,7 @@ func New(m map[string]interface{}) (user.Manager, error) {
c.GroupFilter = strings.ReplaceAll(c.GroupFilter, "%s", "{{.OpaqueId}}")

mgr := &manager{
hostname: c.Hostname,
port: c.Port,
baseDN: c.BaseDN,
findfilter: c.FindFilter,
bindUsername: c.BindUsername,
bindPassword: c.BindPassword,
idp: c.Idp,
schema: c.Schema,
c: c,
}

mgr.userfilter, err = template.New("uf").Funcs(sprig.TxtFuncMap()).Parse(c.UserFilter)
Expand All @@ -138,24 +128,24 @@ func New(m map[string]interface{}) (user.Manager, error) {

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,
m.getUserFilter(uid),
[]string{m.schema.DN, m.schema.CN, m.schema.UID, m.schema.Mail, m.schema.DisplayName},
[]string{m.c.Schema.DN, m.c.Schema.UID, m.c.Schema.CN, m.c.Schema.Mail, m.c.Schema.DisplayName},
nil,
)

Expand All @@ -171,43 +161,43 @@ 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 {
return nil, err
}
u := &userpb.User{
Id: id,
Username: sr.Entries[0].GetAttributeValue(m.schema.CN),
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,
m.getFindFilter(query),
[]string{m.schema.DN, m.schema.CN, m.schema.UID, m.schema.Mail, m.schema.DisplayName},
[]string{m.c.Schema.DN, m.c.Schema.UID, m.c.Schema.CN, m.c.Schema.Mail, m.c.Schema.DisplayName},
nil,
)

Expand All @@ -220,19 +210,19 @@ 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 {
return nil, err
}
user := &userpb.User{
Id: id,
Username: entry.GetAttributeValue(m.schema.CN),
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)
}
Expand All @@ -241,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,
m.getGroupFilter(uid),
[]string{m.schema.CN}, // TODO use DN to look up group id
[]string{m.c.Schema.CN}, // TODO use DN to look up group id
nil,
)

Expand All @@ -273,7 +263,7 @@ func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]stri
// 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.schema.CN))
groups = append(groups, entry.GetAttributeValue(m.c.Schema.CN))
}

return groups, nil
Expand Down Expand Up @@ -305,7 +295,7 @@ func (m *manager) getUserFilter(uid *userpb.UserId) string {
}

func (m *manager) getFindFilter(query string) string {
return strings.ReplaceAll(m.findfilter, "{{query}}", query)
return strings.ReplaceAll(m.c.FindFilter, "{{query}}", query)
}

func (m *manager) getGroupFilter(uid *userpb.UserId) string {
Expand Down

0 comments on commit db04e95

Please sign in to comment.