Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compare groups case-insensitively at login time #3240

Merged
merged 2 commits into from
Aug 25, 2017
Merged

Compare groups case-insensitively at login time #3240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d99327b
Compare groups case-insensitively at login time, since Okta groups are
jefferai Aug 25, 2017
292b56e
Make other group operations case-preserving but otherwise
jefferai Aug 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions builtin/credential/okta/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ func (b *backend) Login(req *logical.Request, username string, password string)
var allGroups []string
// Import the custom added groups from okta backend
user, err := b.User(req.Storage, username)
if err != nil {
if b.Logger().IsDebug() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of unrelated, but why do we need to check the logger's level? Does it not respect the level set with SetLevel() so that logger.Debug() doesn't print if the logger is set at a higher level?

b.Logger().Debug("auth/okta: error looking up user", "error", err)
}
}
if err == nil && user != nil && user.Groups != nil {
if b.Logger().IsDebug() {
b.Logger().Debug("auth/okta: adding local groups", "num_local_groups", len(user.Groups), "local_groups", user.Groups)
Expand All @@ -96,9 +101,14 @@ func (b *backend) Login(req *logical.Request, username string, password string)
// Retrieve policies
var policies []string
for _, groupName := range allGroups {
group, err := b.Group(req.Storage, groupName)
if err == nil && group != nil && group.Policies != nil {
policies = append(policies, group.Policies...)
entry, _, err := b.Group(req.Storage, groupName)
if err != nil {
if b.Logger().IsDebug() {
b.Logger().Debug("auth/okta: error looking up group policies", "error", err)
}
}
if err == nil && entry != nil && entry.Policies != nil {
policies = append(policies, entry.Policies...)
}
}

Expand Down
11 changes: 6 additions & 5 deletions builtin/credential/okta/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import (
"github.com/hashicorp/vault/helper/policyutil"
log "github.com/mgutz/logxi/v1"

"time"

"github.com/hashicorp/vault/logical"
logicaltest "github.com/hashicorp/vault/logical/testing"
"time"
)

func TestBackend_Config(t *testing.T) {
Expand Down Expand Up @@ -52,15 +53,15 @@ func TestBackend_Config(t *testing.T) {
testConfigCreate(t, configData),
testLoginWrite(t, username, "wrong", "E0000004", 0, nil),
testLoginWrite(t, username, password, "user is not a member of any authorized policy", 0, nil),
testAccUserGroups(t, username, "local_group,local_group2"),
testAccGroups(t, "local_group", "local_group_policy"),
testAccUserGroups(t, username, "local_grouP,lOcal_group2"),
testAccGroups(t, "local_groUp", "loCal_group_policy"),
testLoginWrite(t, username, password, "", defaultLeaseTTLVal, []string{"local_group_policy"}),
testAccGroups(t, "Everyone", "everyone_group_policy,every_group_policy2"),
testAccGroups(t, "everyoNe", "everyone_grouP_policy,eveRy_group_policy2"),
testLoginWrite(t, username, password, "", defaultLeaseTTLVal, []string{"local_group_policy"}),
testConfigUpdate(t, configDataToken),
testConfigRead(t, token, configData),
testLoginWrite(t, username, password, "", updatedDuration, []string{"everyone_group_policy", "every_group_policy2", "local_group_policy"}),
testAccGroups(t, "local_group2", "testgroup_group_policy"),
testAccGroups(t, "locAl_group2", "testgroup_group_policy"),
testLoginWrite(t, username, password, "", updatedDuration, []string{"everyone_group_policy", "every_group_policy2", "local_group_policy", "testgroup_group_policy"}),
},
})
Expand Down
59 changes: 49 additions & 10 deletions builtin/credential/okta/path_groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package okta

import (
"strings"

"github.com/hashicorp/vault/helper/policyutil"
"github.com/hashicorp/vault/logical"
"github.com/hashicorp/vault/logical/framework"
Expand Down Expand Up @@ -45,34 +47,59 @@ func pathGroups(b *backend) *framework.Path {
}
}

func (b *backend) Group(s logical.Storage, n string) (*GroupEntry, error) {
// We look up groups in a case-insensitive manner since Okta is case-preserving
// but case-insensitive for comparisons
func (b *backend) Group(s logical.Storage, n string) (*GroupEntry, string, error) {
canonicalName := n
entry, err := s.Get("group/" + n)
if err != nil {
return nil, err
return nil, "", err
}
if entry == nil {
return nil, nil
entries, err := s.List("group/")
if err != nil {
return nil, "", err
}
for _, groupName := range entries {
if strings.ToLower(groupName) == strings.ToLower(n) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just look at group/+ strings.ToLower(n) and avoid looking at all the entries?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will only get us one possible capitalization. So if you have written "GroupFoo" before and now try to write "Groupfoo", you won't find it. With the current logic, you will.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it!

entry, err = s.Get("group/" + groupName)
if err != nil {
return nil, "", err
}
canonicalName = groupName
break
}
}
}
if entry == nil {
return nil, "", nil
}

var result GroupEntry
if err := entry.DecodeJSON(&result); err != nil {
return nil, err
return nil, "", err
}

return &result, nil
return &result, canonicalName, nil
}

func (b *backend) pathGroupDelete(
req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
name := d.Get("name").(string)
if len(name) == 0 {
return logical.ErrorResponse("Error empty name"), nil
return logical.ErrorResponse("'name' must be supplied"), nil
}

err := req.Storage.Delete("group/" + name)
entry, canonicalName, err := b.Group(req.Storage, name)
if err != nil {
return nil, err
}
if entry != nil {
err := req.Storage.Delete("group/" + canonicalName)
if err != nil {
return nil, err
}
}

return nil, nil
}
Expand All @@ -81,10 +108,10 @@ func (b *backend) pathGroupRead(
req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
name := d.Get("name").(string)
if len(name) == 0 {
return logical.ErrorResponse("Error empty name"), nil
return logical.ErrorResponse("'name' must be supplied"), nil
}

group, err := b.Group(req.Storage, name)
group, _, err := b.Group(req.Storage, name)
if err != nil {
return nil, err
}
Expand All @@ -103,7 +130,19 @@ func (b *backend) pathGroupWrite(
req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
name := d.Get("name").(string)
if len(name) == 0 {
return logical.ErrorResponse("Error empty name"), nil
return logical.ErrorResponse("'name' must be supplied"), nil
}

// Check for an existing group, possibly lowercased so that we keep using
// existing user set values
_, canonicalName, err := b.Group(req.Storage, name)
if err != nil {
return nil, err
}
if canonicalName != "" {
name = canonicalName
} else {
name = strings.ToLower(name)
}

entry, err := logical.StorageEntryJSON("group/"+name, &GroupEntry{
Expand Down