From ee90be8dac36928cd1b13ba7bc2e70d0e421039b Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 12 Sep 2017 10:02:27 -0400 Subject: [PATCH 1/4] Ingress policies into aws role via TypeCommaStringSlice. Fixes #3323 --- builtin/credential/aws/path_role.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/credential/aws/path_role.go b/builtin/credential/aws/path_role.go index 78ff0dad1ab7..476becab3a40 100644 --- a/builtin/credential/aws/path_role.go +++ b/builtin/credential/aws/path_role.go @@ -129,7 +129,7 @@ to 0, in which case the value will fallback to the system/mount defaults.`, Description: "The maximum allowed lifetime of tokens issued using this role.", }, "policies": { - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Default: "default", Description: "Policies to be set on tokens issued using this role.", }, @@ -626,11 +626,11 @@ func (b *backend) pathRoleCreateUpdate( return logical.ErrorResponse("at least be one bound parameter should be specified on the role"), nil } - policiesStr, ok := data.GetOk("policies") + policiesRaw, ok := data.GetOk("policies") if ok { - roleEntry.Policies = policyutil.ParsePolicies(policiesStr.(string)) + roleEntry.Policies = policyutil.ParsePolicies(policiesRaw) } else if req.Operation == logical.CreateOperation { - roleEntry.Policies = []string{"default"} + roleEntry.Policies = []string{} } disallowReauthenticationBool, ok := data.GetOk("disallow_reauthentication") From ef3c0e11568861de7ee3a96c899ef14537a62c64 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 12 Sep 2017 10:48:18 -0400 Subject: [PATCH 2/4] Sanitize policy behavior across backends * Ingress TypeCommaStringSlice * Read returns a slice * Default is not added to the saved set of policies, we let core do its thing Fixes #3318 --- builtin/credential/approle/path_role.go | 2 +- builtin/credential/aws/path_role_tag.go | 6 +++--- builtin/credential/cert/path_certs.go | 6 +++--- builtin/credential/ldap/path_groups.go | 8 +++----- builtin/credential/ldap/path_users.go | 6 +++--- builtin/credential/okta/path_groups.go | 4 ++-- builtin/credential/radius/path_users.go | 4 ++-- builtin/credential/userpass/path_user_policies.go | 4 ++-- builtin/credential/userpass/path_users.go | 6 +++--- helper/policyutil/policyutil.go | 4 ++-- 10 files changed, 24 insertions(+), 26 deletions(-) diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go index 593fffa16e8d..b9f7e5b98de4 100644 --- a/builtin/credential/approle/path_role.go +++ b/builtin/credential/approle/path_role.go @@ -1359,7 +1359,7 @@ func (b *backend) pathRolePoliciesDelete(req *logical.Request, data *framework.F lock.Lock() defer lock.Unlock() - role.Policies = policyutil.ParsePolicies(data.GetDefaultOrZero("policies")) + role.Policies = []string{} return nil, b.setRoleEntry(req.Storage, roleName, role, "") } diff --git a/builtin/credential/aws/path_role_tag.go b/builtin/credential/aws/path_role_tag.go index 5c8a119b14b3..0f5dc5ee7a81 100644 --- a/builtin/credential/aws/path_role_tag.go +++ b/builtin/credential/aws/path_role_tag.go @@ -35,7 +35,7 @@ If set, the created tag can only be used by the instance with the given ID.`, }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Policies to be associated with the tag. If set, must be a subset of the role's policies. If set, but set to an empty value, only the 'default' policy will be given to issued tokens.", }, @@ -107,9 +107,9 @@ func (b *backend) pathRoleTagUpdate( // should be inherited. So, by leaving the policies var unset to anything when it is not // supplied, we ensure that it inherits all the policies on the role. var policies []string - policiesStr, ok := data.GetOk("policies") + policiesRaw, ok := data.GetOk("policies") if ok { - policies = policyutil.ParsePolicies(policiesStr.(string)) + policies = policyutil.ParsePolicies(policiesRaw) } if !strutil.StrListSubset(roleEntry.Policies, policies) { resp.AddWarning("Policies on the tag are not a subset of the policies set on the role. Login will not be allowed with this tag unless the role policies are updated.") diff --git a/builtin/credential/cert/path_certs.go b/builtin/credential/cert/path_certs.go index 2c002f6e3f3f..fc5254f2891e 100644 --- a/builtin/credential/cert/path_certs.go +++ b/builtin/credential/cert/path_certs.go @@ -52,7 +52,7 @@ certificate.`, }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Comma-seperated list of policies.", }, @@ -133,7 +133,7 @@ func (b *backend) pathCertRead( Data: map[string]interface{}{ "certificate": cert.Certificate, "display_name": cert.DisplayName, - "policies": strings.Join(cert.Policies, ","), + "policies": cert.Policies, "ttl": duration / time.Second, }, }, nil @@ -144,7 +144,7 @@ func (b *backend) pathCertWrite( name := strings.ToLower(d.Get("name").(string)) certificate := d.Get("certificate").(string) displayName := d.Get("display_name").(string) - policies := policyutil.ParsePolicies(d.Get("policies").(string)) + policies := policyutil.ParsePolicies(d.Get("policies")) allowedNames := d.Get("allowed_names").([]string) // Default the display name to the certificate name if not given diff --git a/builtin/credential/ldap/path_groups.go b/builtin/credential/ldap/path_groups.go index 998fdc41b6aa..48c0d251b7c5 100644 --- a/builtin/credential/ldap/path_groups.go +++ b/builtin/credential/ldap/path_groups.go @@ -1,8 +1,6 @@ package ldap import ( - "strings" - "github.com/hashicorp/vault/helper/policyutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -31,7 +29,7 @@ func pathGroups(b *backend) *framework.Path { }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Comma-separated list of policies associated to the group.", }, }, @@ -86,7 +84,7 @@ func (b *backend) pathGroupRead( return &logical.Response{ Data: map[string]interface{}{ - "policies": strings.Join(group.Policies, ","), + "policies": group.Policies, }, }, nil } @@ -95,7 +93,7 @@ func (b *backend) pathGroupWrite( req *logical.Request, d *framework.FieldData) (*logical.Response, error) { // Store it entry, err := logical.StorageEntryJSON("group/"+d.Get("name").(string), &GroupEntry{ - Policies: policyutil.ParsePolicies(d.Get("policies").(string)), + Policies: policyutil.ParsePolicies(d.Get("policies")), }) if err != nil { return nil, err diff --git a/builtin/credential/ldap/path_users.go b/builtin/credential/ldap/path_users.go index 605f779cb3d5..6845a4177cd6 100644 --- a/builtin/credential/ldap/path_users.go +++ b/builtin/credential/ldap/path_users.go @@ -37,7 +37,7 @@ func pathUsers(b *backend) *framework.Path { }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Comma-separated list of policies associated with the user.", }, }, @@ -93,7 +93,7 @@ func (b *backend) pathUserRead( return &logical.Response{ Data: map[string]interface{}{ "groups": strings.Join(user.Groups, ","), - "policies": strings.Join(user.Policies, ","), + "policies": user.Policies, }, }, nil } @@ -102,7 +102,7 @@ func (b *backend) pathUserWrite( req *logical.Request, d *framework.FieldData) (*logical.Response, error) { name := d.Get("name").(string) groups := strutil.RemoveDuplicates(strutil.ParseStringSlice(d.Get("groups").(string), ","), false) - policies := policyutil.ParsePolicies(d.Get("policies").(string)) + policies := policyutil.ParsePolicies(d.Get("policies")) for i, g := range groups { groups[i] = strings.TrimSpace(g) } diff --git a/builtin/credential/okta/path_groups.go b/builtin/credential/okta/path_groups.go index ba2697b3095d..9f879a12cd96 100644 --- a/builtin/credential/okta/path_groups.go +++ b/builtin/credential/okta/path_groups.go @@ -31,7 +31,7 @@ func pathGroups(b *backend) *framework.Path { }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Comma-separated list of policies associated to the group.", }, }, @@ -146,7 +146,7 @@ func (b *backend) pathGroupWrite( } entry, err := logical.StorageEntryJSON("group/"+name, &GroupEntry{ - Policies: policyutil.ParsePolicies(d.Get("policies").(string)), + Policies: policyutil.ParsePolicies(d.Get("policies")), }) if err != nil { return nil, err diff --git a/builtin/credential/radius/path_users.go b/builtin/credential/radius/path_users.go index ac9a97175ff5..1e0fc613f77f 100644 --- a/builtin/credential/radius/path_users.go +++ b/builtin/credential/radius/path_users.go @@ -32,7 +32,7 @@ func pathUsers(b *backend) *framework.Path { }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Comma-separated list of policies associated to the user.", }, }, @@ -111,7 +111,7 @@ func (b *backend) pathUserRead( func (b *backend) pathUserWrite( req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - var policies = policyutil.ParsePolicies(d.Get("policies").(string)) + var policies = policyutil.ParsePolicies(d.Get("policies")) for _, policy := range policies { if policy == "root" { return logical.ErrorResponse("root policy cannot be granted by an authentication backend"), nil diff --git a/builtin/credential/userpass/path_user_policies.go b/builtin/credential/userpass/path_user_policies.go index 6165c189f381..d03a6c28050a 100644 --- a/builtin/credential/userpass/path_user_policies.go +++ b/builtin/credential/userpass/path_user_policies.go @@ -17,7 +17,7 @@ func pathUserPolicies(b *backend) *framework.Path { Description: "Username for this user.", }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Comma-separated list of policies", }, }, @@ -44,7 +44,7 @@ func (b *backend) pathUserPoliciesUpdate( return nil, fmt.Errorf("username does not exist") } - userEntry.Policies = policyutil.ParsePolicies(d.Get("policies").(string)) + userEntry.Policies = policyutil.ParsePolicies(d.Get("policies")) return nil, b.setUser(req.Storage, username, userEntry) } diff --git a/builtin/credential/userpass/path_users.go b/builtin/credential/userpass/path_users.go index f8d4eb089c0a..b2075981cd6c 100644 --- a/builtin/credential/userpass/path_users.go +++ b/builtin/credential/userpass/path_users.go @@ -38,7 +38,7 @@ func pathUsers(b *backend) *framework.Path { }, "policies": &framework.FieldSchema{ - Type: framework.TypeString, + Type: framework.TypeCommaStringSlice, Description: "Comma-separated list of policies", }, "ttl": &framework.FieldSchema{ @@ -137,7 +137,7 @@ func (b *backend) pathUserRead( return &logical.Response{ Data: map[string]interface{}{ - "policies": strings.Join(user.Policies, ","), + "policies": user.Policies, "ttl": user.TTL.Seconds(), "max_ttl": user.MaxTTL.Seconds(), }, @@ -166,7 +166,7 @@ func (b *backend) userCreateUpdate(req *logical.Request, d *framework.FieldData) } if policiesRaw, ok := d.GetOk("policies"); ok { - userEntry.Policies = policyutil.ParsePolicies(policiesRaw.(string)) + userEntry.Policies = policyutil.ParsePolicies(policiesRaw) } ttlStr := userEntry.TTL.String() diff --git a/helper/policyutil/policyutil.go b/helper/policyutil/policyutil.go index 22129bc26366..f6d9f6687487 100644 --- a/helper/policyutil/policyutil.go +++ b/helper/policyutil/policyutil.go @@ -27,14 +27,14 @@ func ParsePolicies(policiesRaw interface{}) []string { switch policiesRaw.(type) { case string: if policiesRaw.(string) == "" { - return []string{"default"} + return []string{} } policies = strings.Split(policiesRaw.(string), ",") case []string: policies = policiesRaw.([]string) } - return SanitizePolicies(policies, true) + return SanitizePolicies(policies, false) } // SanitizePolicies performs the common input validation tasks From 32d0fcdba5145c6e45ae7e414dda76c749645089 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 12 Sep 2017 11:29:34 -0400 Subject: [PATCH 3/4] Fix tests --- builtin/credential/approle/path_role_test.go | 2 +- builtin/credential/aws/path_role_test.go | 2 +- builtin/credential/ldap/backend_test.go | 9 +++++---- builtin/credential/ldap/path_login.go | 2 -- builtin/credential/userpass/backend_test.go | 13 +++++++------ 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/builtin/credential/approle/path_role_test.go b/builtin/credential/approle/path_role_test.go index e35a4eba7f29..3f33ec8a4260 100644 --- a/builtin/credential/approle/path_role_test.go +++ b/builtin/credential/approle/path_role_test.go @@ -608,7 +608,7 @@ func TestAppRole_RoleCRUD(t *testing.T) { expected := map[string]interface{}{ "bind_secret_id": true, - "policies": []string{"default", "p", "q", "r", "s"}, + "policies": []string{"p", "q", "r", "s"}, "secret_id_num_uses": 10, "secret_id_ttl": 300, "token_ttl": 400, diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 584b6dae7bc1..21c87ab22369 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -566,7 +566,7 @@ func TestAwsEc2_RoleCrud(t *testing.T) { "allow_instance_migration": true, "ttl": time.Duration(600), "max_ttl": time.Duration(1200), - "policies": []string{"default", "testpolicy1", "testpolicy2"}, + "policies": []string{"testpolicy1", "testpolicy2"}, "disallow_reauthentication": true, "period": time.Duration(60), } diff --git a/builtin/credential/ldap/backend_test.go b/builtin/credential/ldap/backend_test.go index d900c21ae79b..3b1d936281d2 100644 --- a/builtin/credential/ldap/backend_test.go +++ b/builtin/credential/ldap/backend_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/hashicorp/vault/helper/policyutil" "github.com/hashicorp/vault/logical" logicaltest "github.com/hashicorp/vault/logical/testing" "github.com/mitchellh/mapstructure" @@ -94,7 +95,7 @@ func TestLdapAuthBackend_UserPolicies(t *testing.T) { if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("err:%v resp:%#v", err, resp) } - expected := []string{"default", "grouppolicy", "userpolicy"} + expected := []string{"grouppolicy", "userpolicy"} if !reflect.DeepEqual(expected, resp.Auth.Policies) { t.Fatalf("bad: policies: expected: %q, actual: %q", expected, resp.Auth.Policies) } @@ -211,7 +212,7 @@ func TestBackend_groupCrud(t *testing.T) { Backend: b, Steps: []logicaltest.TestStep{ testAccStepGroup(t, "g1", "foo"), - testAccStepReadGroup(t, "g1", "default,foo"), + testAccStepReadGroup(t, "g1", "foo"), testAccStepDeleteGroup(t, "g1"), testAccStepReadGroup(t, "g1", ""), }, @@ -357,13 +358,13 @@ func testAccStepReadGroup(t *testing.T, group string, policies string) logicalte } var d struct { - Policies string `mapstructure:"policies"` + Policies []string `mapstructure:"policies"` } if err := mapstructure.Decode(resp.Data, &d); err != nil { return err } - if d.Policies != policies { + if !reflect.DeepEqual(d.Policies, policyutil.ParsePolicies(policies)) { return fmt.Errorf("bad: %#v", resp) } diff --git a/builtin/credential/ldap/path_login.go b/builtin/credential/ldap/path_login.go index e859adb01a54..2266e8da14c9 100644 --- a/builtin/credential/ldap/path_login.go +++ b/builtin/credential/ldap/path_login.go @@ -3,7 +3,6 @@ package ldap import ( "fmt" "sort" - "strings" "github.com/hashicorp/vault/helper/policyutil" "github.com/hashicorp/vault/logical" @@ -59,7 +58,6 @@ func (b *backend) pathLogin( Policies: policies, Metadata: map[string]string{ "username": username, - "policies": strings.Join(policies, ","), }, InternalData: map[string]interface{}{ "password": password, diff --git a/builtin/credential/userpass/backend_test.go b/builtin/credential/userpass/backend_test.go index f04dc6a8f5a9..4f077eee2b4c 100644 --- a/builtin/credential/userpass/backend_test.go +++ b/builtin/credential/userpass/backend_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/hashicorp/vault/helper/policyutil" "github.com/hashicorp/vault/logical" logicaltest "github.com/hashicorp/vault/logical/testing" "github.com/mitchellh/mapstructure" @@ -106,7 +107,7 @@ func TestBackend_userCrud(t *testing.T) { Backend: b, Steps: []logicaltest.TestStep{ testAccStepUser(t, "web", "password", "foo"), - testAccStepReadUser(t, "web", "default,foo"), + testAccStepReadUser(t, "web", "foo"), testAccStepDeleteUser(t, "web"), testAccStepReadUser(t, "web", ""), }, @@ -150,7 +151,7 @@ func TestBackend_passwordUpdate(t *testing.T) { Backend: b, Steps: []logicaltest.TestStep{ testAccStepUser(t, "web", "password", "foo"), - testAccStepReadUser(t, "web", "default,foo"), + testAccStepReadUser(t, "web", "foo"), testAccStepLogin(t, "web", "password", []string{"default", "foo"}), testUpdatePassword(t, "web", "newpassword"), testAccStepLogin(t, "web", "newpassword", []string{"default", "foo"}), @@ -175,10 +176,10 @@ func TestBackend_policiesUpdate(t *testing.T) { Backend: b, Steps: []logicaltest.TestStep{ testAccStepUser(t, "web", "password", "foo"), - testAccStepReadUser(t, "web", "default,foo"), + testAccStepReadUser(t, "web", "foo"), testAccStepLogin(t, "web", "password", []string{"default", "foo"}), testUpdatePolicies(t, "web", "foo,bar"), - testAccStepReadUser(t, "web", "bar,default,foo"), + testAccStepReadUser(t, "web", "bar,foo"), testAccStepLogin(t, "web", "password", []string{"bar", "default", "foo"}), }, }) @@ -311,13 +312,13 @@ func testAccStepReadUser(t *testing.T, name string, policies string) logicaltest } var d struct { - Policies string `mapstructure:"policies"` + Policies []string `mapstructure:"policies"` } if err := mapstructure.Decode(resp.Data, &d); err != nil { return err } - if d.Policies != policies { + if !reflect.DeepEqual(d.Policies, policyutil.ParsePolicies(policies)) { return fmt.Errorf("bad: %#v", resp) } From 8ec702b41303d3fd4b6077c317827296f59556a6 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 12 Sep 2017 13:37:26 -0400 Subject: [PATCH 4/4] Fix tests --- builtin/credential/approle/path_role_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/credential/approle/path_role_test.go b/builtin/credential/approle/path_role_test.go index 3f33ec8a4260..fa3e68150918 100644 --- a/builtin/credential/approle/path_role_test.go +++ b/builtin/credential/approle/path_role_test.go @@ -656,7 +656,7 @@ func TestAppRole_RoleCRUD(t *testing.T) { } expected = map[string]interface{}{ - "policies": []string{"a", "b", "c", "d", "default"}, + "policies": []string{"a", "b", "c", "d"}, "secret_id_num_uses": 100, "secret_id_ttl": 3000, "token_ttl": 4000, @@ -764,7 +764,7 @@ func TestAppRole_RoleCRUD(t *testing.T) { t.Fatalf("err:%v resp:%#v", err, resp) } - if !reflect.DeepEqual(resp.Data["policies"].([]string), []string{"a1", "b1", "c1", "d1", "default"}) { + if !reflect.DeepEqual(resp.Data["policies"].([]string), []string{"a1", "b1", "c1", "d1"}) { t.Fatalf("bad: policies: actual:%s\n", resp.Data["policies"].([]string)) } roleReq.Operation = logical.DeleteOperation