Sanitize policy behavior across backends (#3324)

Fixes #3323
Fixes #3318

* Fix tests

* Fix tests

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, "")
 }

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,
@@ -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

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")

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.")

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),
 	}

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

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)
 			}
 

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

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,

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)
 	}

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

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

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)
 			}
 

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)
 }

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()

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