From f5ba4796f59c52b4172f534844623bf87c021be1 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 3 Apr 2018 09:52:43 -0400 Subject: [PATCH] Case insensitive behavior for LDAP (#4238) --- CHANGELOG.md | 5 + builtin/credential/ldap/backend.go | 19 +- builtin/credential/ldap/backend_test.go | 218 +++++++++++++++++++-- builtin/credential/ldap/path_config.go | 111 ++++++++--- builtin/credential/ldap/path_groups.go | 31 ++- builtin/credential/ldap/path_users.go | 35 +++- website/source/api/auth/ldap/index.html.md | 5 + 7 files changed, 365 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23419865b..fa7432f69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ DEPRECATIONS/CHANGES: accommodate this as best as possible, and users of other tools may have to make adjustments, but in the end we felt that the ends did not justify the means and we needed to prioritize security over operational convenience. + * LDAP auth method case sensitivity: We now treat usernames and groups + configured locally for policy assignment in a case insensitive fashion by + default. Existing configurations will continue to work as they do now; + however, the next time a configuration is written `case_sensitive_names` + will need to be explicitly set to `true`. FEATURES: diff --git a/builtin/credential/ldap/backend.go b/builtin/credential/ldap/backend.go index 61ef01d46..8d3e46b31 100644 --- a/builtin/credential/ldap/backend.go +++ b/builtin/credential/ldap/backend.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "math" + "strings" "text/template" "github.com/go-ldap/ldap" @@ -173,8 +174,13 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri } var allGroups []string + canonicalUsername := username + cs := *cfg.CaseSensitiveNames + if !cs { + canonicalUsername = strings.ToLower(username) + } // Import the custom added groups from ldap backend - user, err := b.User(ctx, req.Storage, username) + user, err := b.User(ctx, req.Storage, canonicalUsername) if err == nil && user != nil && user.Groups != nil { if b.Logger().IsDebug() { b.Logger().Debug("adding local groups", "num_local_groups", len(user.Groups), "local_groups", user.Groups) @@ -184,9 +190,18 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri // Merge local and LDAP groups allGroups = append(allGroups, ldapGroups...) + canonicalGroups := allGroups + // If not case sensitive, lowercase all + if !cs { + canonicalGroups = make([]string, len(allGroups)) + for i, v := range allGroups { + canonicalGroups[i] = strings.ToLower(v) + } + } + // Retrieve policies var policies []string - for _, groupName := range allGroups { + for _, groupName := range canonicalGroups { group, err := b.Group(ctx, req.Storage, groupName) if err == nil && group != nil { policies = append(policies, group.Policies...) diff --git a/builtin/credential/ldap/backend_test.go b/builtin/credential/ldap/backend_test.go index be4c1e6db..ccffd4ee2 100644 --- a/builtin/credential/ldap/backend_test.go +++ b/builtin/credential/ldap/backend_test.go @@ -31,6 +31,182 @@ func createBackendWithStorage(t *testing.T) (*backend, logical.Storage) { return b, config.StorageView } +func TestLdapAuthBackend_CaseSensitivity(t *testing.T) { + var resp *logical.Response + var err error + b, storage := createBackendWithStorage(t) + + ctx := context.Background() + + testVals := func(caseSensitive bool) { + // Clear storage + userList, err := storage.List(ctx, "user/") + if err != nil { + t.Fatal(err) + } + for _, user := range userList { + err = storage.Delete(ctx, "user/"+user) + if err != nil { + t.Fatal(err) + } + } + groupList, err := storage.List(ctx, "group/") + if err != nil { + t.Fatal(err) + } + for _, group := range groupList { + err = storage.Delete(ctx, "group/"+group) + if err != nil { + t.Fatal(err) + } + } + + configReq := &logical.Request{ + Path: "config", + Operation: logical.ReadOperation, + Storage: storage, + } + resp, err = b.HandleRequest(ctx, configReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + if resp == nil { + t.Fatal("nil response") + } + if resp.Data["case_sensitive_names"].(bool) != caseSensitive { + t.Fatalf("expected case sensitivity %t, got %t", caseSensitive, resp.Data["case_sensitive_names"].(bool)) + } + + groupReq := &logical.Request{ + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "policies": "grouppolicy", + }, + Path: "groups/EngineerS", + Storage: storage, + } + resp, err = b.HandleRequest(ctx, groupReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + keys, err := storage.List(ctx, "group/") + if err != nil { + t.Fatal(err) + } + switch caseSensitive { + case true: + if keys[0] != "EngineerS" { + t.Fatalf("bad: %s", keys[0]) + } + default: + if keys[0] != "engineers" { + t.Fatalf("bad: %s", keys[0]) + } + } + + userReq := &logical.Request{ + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "groups": "EngineerS", + "policies": "userpolicy", + }, + Path: "users/teSlA", + Storage: storage, + } + resp, err = b.HandleRequest(ctx, userReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + keys, err = storage.List(ctx, "user/") + if err != nil { + t.Fatal(err) + } + switch caseSensitive { + case true: + if keys[0] != "teSlA" { + t.Fatalf("bad: %s", keys[0]) + } + default: + if keys[0] != "tesla" { + t.Fatalf("bad: %s", keys[0]) + } + } + + if caseSensitive { + // The online test server is actually case sensitive so we need to + // write again so it works + userReq = &logical.Request{ + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "groups": "EngineerS", + "policies": "userpolicy", + }, + Path: "users/tesla", + Storage: storage, + } + resp, err = b.HandleRequest(ctx, userReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + } + + loginReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "login/tesla", + Data: map[string]interface{}{ + "password": "password", + }, + Storage: storage, + } + resp, err = b.HandleRequest(ctx, loginReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + expected := []string{"grouppolicy", "userpolicy"} + if !reflect.DeepEqual(expected, resp.Auth.Policies) { + t.Fatalf("bad: policies: expected: %q, actual: %q", expected, resp.Auth.Policies) + } + } + + configReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "config", + Data: map[string]interface{}{ + // Online LDAP test server + // http://www.forumsys.com/tutorials/integration-how-to/ldap/online-ldap-test-server/ + "url": "ldap://ldap.forumsys.com", + "userattr": "uid", + "userdn": "dc=example,dc=com", + "groupdn": "dc=example,dc=com", + "binddn": "cn=read-only-admin,dc=example,dc=com", + }, + Storage: storage, + } + resp, err = b.HandleRequest(ctx, configReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + + testVals(false) + + // Check that if the value is nil, on read it is case sensitive + configEntry, err := b.Config(ctx, configReq) + if err != nil { + t.Fatal(err) + } + configEntry.CaseSensitiveNames = nil + entry, err := logical.StorageEntryJSON("config", configEntry) + if err != nil { + t.Fatal(err) + } + err = configReq.Storage.Put(ctx, entry) + if err != nil { + t.Fatal(err) + } + + testVals(true) +} + func TestLdapAuthBackend_UserPolicies(t *testing.T) { var resp *logical.Response var err error @@ -278,10 +454,11 @@ func testAccStepConfigUrl(t *testing.T) logicaltest.TestStep { Data: map[string]interface{}{ // Online LDAP test server // http://www.forumsys.com/tutorials/integration-how-to/ldap/online-ldap-test-server/ - "url": "ldap://ldap.forumsys.com", - "userattr": "uid", - "userdn": "dc=example,dc=com", - "groupdn": "dc=example,dc=com", + "url": "ldap://ldap.forumsys.com", + "userattr": "uid", + "userdn": "dc=example,dc=com", + "groupdn": "dc=example,dc=com", + "case_sensitive_names": true, }, } } @@ -294,12 +471,13 @@ func testAccStepConfigUrlWithAuthBind(t *testing.T) logicaltest.TestStep { // Online LDAP test server // http://www.forumsys.com/tutorials/integration-how-to/ldap/online-ldap-test-server/ // In this test we also exercise multiple URL support - "url": "foobar://ldap.example.com,ldap://ldap.forumsys.com", - "userattr": "uid", - "userdn": "dc=example,dc=com", - "groupdn": "dc=example,dc=com", - "binddn": "cn=read-only-admin,dc=example,dc=com", - "bindpass": "password", + "url": "foobar://ldap.example.com,ldap://ldap.forumsys.com", + "userattr": "uid", + "userdn": "dc=example,dc=com", + "groupdn": "dc=example,dc=com", + "binddn": "cn=read-only-admin,dc=example,dc=com", + "bindpass": "password", + "case_sensitive_names": true, }, } } @@ -311,11 +489,12 @@ func testAccStepConfigUrlWithDiscover(t *testing.T) logicaltest.TestStep { Data: map[string]interface{}{ // Online LDAP test server // http://www.forumsys.com/tutorials/integration-how-to/ldap/online-ldap-test-server/ - "url": "ldap://ldap.forumsys.com", - "userattr": "uid", - "userdn": "dc=example,dc=com", - "groupdn": "dc=example,dc=com", - "discoverdn": true, + "url": "ldap://ldap.forumsys.com", + "userattr": "uid", + "userdn": "dc=example,dc=com", + "groupdn": "dc=example,dc=com", + "discoverdn": true, + "case_sensitive_names": true, }, } } @@ -327,10 +506,11 @@ func testAccStepConfigUrlNoGroupDN(t *testing.T) logicaltest.TestStep { Data: map[string]interface{}{ // Online LDAP test server // http://www.forumsys.com/tutorials/integration-how-to/ldap/online-ldap-test-server/ - "url": "ldap://ldap.forumsys.com", - "userattr": "uid", - "userdn": "dc=example,dc=com", - "discoverdn": true, + "url": "ldap://ldap.forumsys.com", + "userattr": "uid", + "userdn": "dc=example,dc=com", + "discoverdn": true, + "case_sensitive_names": true, }, } } diff --git a/builtin/credential/ldap/path_config.go b/builtin/credential/ldap/path_config.go index 6cad1e55b..401510c18 100644 --- a/builtin/credential/ldap/path_config.go +++ b/builtin/credential/ldap/path_config.go @@ -14,6 +14,7 @@ import ( "github.com/go-ldap/ldap" log "github.com/hashicorp/go-hclog" multierror "github.com/hashicorp/go-multierror" + "github.com/hashicorp/vault/helper/consts" "github.com/hashicorp/vault/helper/tlsutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -109,11 +110,17 @@ Default: cn`, Default: "tls12", Description: "Maximum TLS version to use. Accepted values are 'tls10', 'tls11' or 'tls12'. Defaults to 'tls12'", }, + "deny_null_bind": &framework.FieldSchema{ Type: framework.TypeBool, Default: true, Description: "Denies an unauthenticated LDAP bind request if the user's password is empty; defaults to true", }, + + "case_sensitive_names": &framework.FieldSchema{ + Type: framework.TypeBool, + Description: "If true, case sensitivity will be used when comparing usernames and groups for matching policies.", + }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -149,6 +156,9 @@ func (b *backend) Config(ctx context.Context, req *logical.Request) (*ConfigEntr if storedConfig == nil { // No user overrides, return default configuration + result.CaseSensitiveNames = new(bool) + *result.CaseSensitiveNames = false + return result, nil } @@ -158,6 +168,24 @@ func (b *backend) Config(ctx context.Context, req *logical.Request) (*ConfigEntr return nil, err } + var persistNeeded bool + if result.CaseSensitiveNames == nil { + // Upgrade from before switching to case-insensitive + result.CaseSensitiveNames = new(bool) + *result.CaseSensitiveNames = true + persistNeeded = true + } + + if persistNeeded && (b.System().LocalMount() || !b.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary)) { + entry, err := logical.StorageEntryJSON("config", result) + if err != nil { + return nil, err + } + if err := req.Storage.Put(ctx, entry); err != nil { + return nil, err + } + } + result.logger = b.Logger() return result, nil @@ -174,21 +202,22 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, d *f resp := &logical.Response{ Data: map[string]interface{}{ - "url": cfg.Url, - "userdn": cfg.UserDN, - "groupdn": cfg.GroupDN, - "groupfilter": cfg.GroupFilter, - "groupattr": cfg.GroupAttr, - "upndomain": cfg.UPNDomain, - "userattr": cfg.UserAttr, - "certificate": cfg.Certificate, - "insecure_tls": cfg.InsecureTLS, - "starttls": cfg.StartTLS, - "binddn": cfg.BindDN, - "deny_null_bind": cfg.DenyNullBind, - "discoverdn": cfg.DiscoverDN, - "tls_min_version": cfg.TLSMinVersion, - "tls_max_version": cfg.TLSMaxVersion, + "url": cfg.Url, + "userdn": cfg.UserDN, + "groupdn": cfg.GroupDN, + "groupfilter": cfg.GroupFilter, + "groupattr": cfg.GroupAttr, + "upndomain": cfg.UPNDomain, + "userattr": cfg.UserAttr, + "certificate": cfg.Certificate, + "insecure_tls": cfg.InsecureTLS, + "starttls": cfg.StartTLS, + "binddn": cfg.BindDN, + "deny_null_bind": cfg.DenyNullBind, + "discoverdn": cfg.DiscoverDN, + "tls_min_version": cfg.TLSMinVersion, + "tls_max_version": cfg.TLSMaxVersion, + "case_sensitive_names": *cfg.CaseSensitiveNames, }, } return resp, nil @@ -282,23 +311,33 @@ func (b *backend) newConfigEntry(d *framework.FieldData) (*ConfigEntry, error) { if startTLS { cfg.StartTLS = startTLS } + bindDN := d.Get("binddn").(string) if bindDN != "" { cfg.BindDN = bindDN } + bindPass := d.Get("bindpass").(string) if bindPass != "" { cfg.BindPassword = bindPass } + denyNullBind := d.Get("deny_null_bind").(bool) if denyNullBind { cfg.DenyNullBind = denyNullBind } + discoverDN := d.Get("discoverdn").(bool) if discoverDN { cfg.DiscoverDN = discoverDN } + caseSensitiveNames, ok := d.GetOk("case_sensitive_names") + if ok { + cfg.CaseSensitiveNames = new(bool) + *cfg.CaseSensitiveNames = caseSensitiveNames.(bool) + } + return cfg, nil } @@ -309,6 +348,13 @@ func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d * return logical.ErrorResponse(err.Error()), nil } + // On write, if not specified, use false. We do this here so upgrade logic + // works since it calls the same newConfigEntry function + if cfg.CaseSensitiveNames == nil { + cfg.CaseSensitiveNames = new(bool) + *cfg.CaseSensitiveNames = false + } + entry, err := logical.StorageEntryJSON("config", cfg) if err != nil { return nil, err @@ -321,23 +367,24 @@ func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, d * } type ConfigEntry struct { - logger log.Logger - Url string `json:"url" structs:"url" mapstructure:"url"` - UserDN string `json:"userdn" structs:"userdn" mapstructure:"userdn"` - GroupDN string `json:"groupdn" structs:"groupdn" mapstructure:"groupdn"` - GroupFilter string `json:"groupfilter" structs:"groupfilter" mapstructure:"groupfilter"` - GroupAttr string `json:"groupattr" structs:"groupattr" mapstructure:"groupattr"` - UPNDomain string `json:"upndomain" structs:"upndomain" mapstructure:"upndomain"` - UserAttr string `json:"userattr" structs:"userattr" mapstructure:"userattr"` - Certificate string `json:"certificate" structs:"certificate" mapstructure:"certificate"` - InsecureTLS bool `json:"insecure_tls" structs:"insecure_tls" mapstructure:"insecure_tls"` - StartTLS bool `json:"starttls" structs:"starttls" mapstructure:"starttls"` - BindDN string `json:"binddn" structs:"binddn" mapstructure:"binddn"` - BindPassword string `json:"bindpass" structs:"bindpass" mapstructure:"bindpass"` - DenyNullBind bool `json:"deny_null_bind" structs:"deny_null_bind" mapstructure:"deny_null_bind"` - DiscoverDN bool `json:"discoverdn" structs:"discoverdn" mapstructure:"discoverdn"` - TLSMinVersion string `json:"tls_min_version" structs:"tls_min_version" mapstructure:"tls_min_version"` - TLSMaxVersion string `json:"tls_max_version" structs:"tls_max_version" mapstructure:"tls_max_version"` + logger log.Logger + Url string `json:"url"` + UserDN string `json:"userdn"` + GroupDN string `json:"groupdn"` + GroupFilter string `json:"groupfilter"` + GroupAttr string `json:"groupattr"` + UPNDomain string `json:"upndomain"` + UserAttr string `json:"userattr"` + Certificate string `json:"certificate"` + InsecureTLS bool `json:"insecure_tls"` + StartTLS bool `json:"starttls"` + BindDN string `json:"binddn"` + BindPassword string `json:"bindpass"` + DenyNullBind bool `json:"deny_null_bind"` + DiscoverDN bool `json:"discoverdn"` + TLSMinVersion string `json:"tls_min_version"` + TLSMaxVersion string `json:"tls_max_version"` + CaseSensitiveNames *bool `json:"case_sensitive_names,omitempty` } func (c *ConfigEntry) GetTLSConfig(host string) (*tls.Config, error) { diff --git a/builtin/credential/ldap/path_groups.go b/builtin/credential/ldap/path_groups.go index dfd7b6329..9d402faa9 100644 --- a/builtin/credential/ldap/path_groups.go +++ b/builtin/credential/ldap/path_groups.go @@ -2,6 +2,7 @@ package ldap import ( "context" + "strings" "github.com/hashicorp/vault/helper/policyutil" "github.com/hashicorp/vault/logical" @@ -74,7 +75,20 @@ func (b *backend) pathGroupDelete(ctx context.Context, req *logical.Request, d * } func (b *backend) pathGroupRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - group, err := b.Group(ctx, req.Storage, d.Get("name").(string)) + groupname := d.Get("name").(string) + + cfg, err := b.Config(ctx, req) + if err != nil { + return nil, err + } + if cfg == nil { + return logical.ErrorResponse("ldap backend not configured"), nil + } + if !*cfg.CaseSensitiveNames { + groupname = strings.ToLower(groupname) + } + + group, err := b.Group(ctx, req.Storage, groupname) if err != nil { return nil, err } @@ -90,8 +104,21 @@ func (b *backend) pathGroupRead(ctx context.Context, req *logical.Request, d *fr } func (b *backend) pathGroupWrite(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + groupname := d.Get("name").(string) + + cfg, err := b.Config(ctx, req) + if err != nil { + return nil, err + } + if cfg == nil { + return logical.ErrorResponse("ldap backend not configured"), nil + } + if !*cfg.CaseSensitiveNames { + groupname = strings.ToLower(groupname) + } + // Store it - entry, err := logical.StorageEntryJSON("group/"+d.Get("name").(string), &GroupEntry{ + entry, err := logical.StorageEntryJSON("group/"+groupname, &GroupEntry{ Policies: policyutil.ParsePolicies(d.Get("policies")), }) if err != nil { diff --git a/builtin/credential/ldap/path_users.go b/builtin/credential/ldap/path_users.go index 038f76870..b19004597 100644 --- a/builtin/credential/ldap/path_users.go +++ b/builtin/credential/ldap/path_users.go @@ -81,7 +81,20 @@ func (b *backend) pathUserDelete(ctx context.Context, req *logical.Request, d *f } func (b *backend) pathUserRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - user, err := b.User(ctx, req.Storage, d.Get("name").(string)) + username := d.Get("name").(string) + + cfg, err := b.Config(ctx, req) + if err != nil { + return nil, err + } + if cfg == nil { + return logical.ErrorResponse("ldap backend not configured"), nil + } + if !*cfg.CaseSensitiveNames { + username = strings.ToLower(username) + } + + user, err := b.User(ctx, req.Storage, username) if err != nil { return nil, err } @@ -98,15 +111,29 @@ func (b *backend) pathUserRead(ctx context.Context, req *logical.Request, d *fra } func (b *backend) pathUserWrite(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - name := d.Get("name").(string) - groups := strutil.RemoveDuplicates(strutil.ParseStringSlice(d.Get("groups").(string), ","), false) + lowercaseGroups := false + username := d.Get("name").(string) + + cfg, err := b.Config(ctx, req) + if err != nil { + return nil, err + } + if cfg == nil { + return logical.ErrorResponse("ldap backend not configured"), nil + } + if !*cfg.CaseSensitiveNames { + username = strings.ToLower(username) + lowercaseGroups = true + } + + groups := strutil.RemoveDuplicates(strutil.ParseStringSlice(d.Get("groups").(string), ","), lowercaseGroups) policies := policyutil.ParsePolicies(d.Get("policies")) for i, g := range groups { groups[i] = strings.TrimSpace(g) } // Store it - entry, err := logical.StorageEntryJSON("user/"+name, &UserEntry{ + entry, err := logical.StorageEntryJSON("user/"+username, &UserEntry{ Groups: groups, Policies: policies, }) diff --git a/website/source/api/auth/ldap/index.html.md b/website/source/api/auth/ldap/index.html.md index ee0a908f3..b7e396e43 100644 --- a/website/source/api/auth/ldap/index.html.md +++ b/website/source/api/auth/ldap/index.html.md @@ -28,6 +28,11 @@ This endpoint configures the LDAP auth method. - `url` `(string: )` – The LDAP server to connect to. Examples: `ldap://ldap.myorg.com`, `ldaps://ldap.myorg.com:636` +- `case_sensitive_names` `(bool: false)` – If set, user and group names + assigned to policies within the backend will be case sensitive. Otherwise, + names will be normalized to lower case. Case will still be preserved when + sending the username to the LDAP server at login time; this is only for + matching local user/group definitions. - `starttls` `(bool: false)` – If true, issues a `StartTLS` command after establishing an unencrypted connection. - `tls_min_version` `(string: tls12)` – Minimum TLS version to use. Accepted