Merge pull request #1235 from hashicorp/policies-validation

Strip out other policies if root is present

helper/policyutil/policyutil.go

@@ -7,6 +7,12 @@ import (
 	"github.com/hashicorp/vault/helper/strutil"
 )
 
+// ParsePolicies parses a comma-delimited list of policies.
+// The resulting collection will have no duplicate elements.
+// If 'root' policy was present in the list of policies, then
+// all other policies will be ignored, the result will contain
+// just the 'root'. In cases where 'root' is not present, if
+// 'default' policy is not already present, it will be added.
 func ParsePolicies(policiesRaw string) []string {
 	if policiesRaw == "" {
 		return []string{"default"}
@@ -14,10 +20,18 @@ func ParsePolicies(policiesRaw string) []string {
 
 	policies := strings.Split(policiesRaw, ",")
 
-	return SanitizePolicies(policies)
+	return SanitizePolicies(policies, true)
 }
 
-func SanitizePolicies(policies []string) []string {
+// SanitizePolicies performs the common input validation tasks
+// which are performed on the list of policies across Vault.
+// The resulting collection will have no duplicate elements.
+// If 'root' policy was present in the list of policies, then
+// all other policies will be ignored, the result will contain
+// just the 'root'. In cases where 'root' is not present, if
+// 'default' policy is not already present, it will be added
+// if addDefault is set to true.
+func SanitizePolicies(policies []string, addDefault bool) []string {
 	defaultFound := false
 	for i, p := range policies {
 		policies[i] = strings.ToLower(strings.TrimSpace(p))
@@ -38,7 +52,7 @@ func SanitizePolicies(policies []string) []string {
 	}
 
 	// Always add 'default' except only if the policies contain 'root'.
-	if len(policies) == 0 || !defaultFound {
+	if addDefault && (len(policies) == 0 || !defaultFound) {
 		policies = append(policies, "default")
 	}
 

helper/policyutil/policyutil_test.go

@@ -2,6 +2,21 @@ package policyutil
 
 import "testing"
 
+func TestSanitizePolicies(t *testing.T) {
+	expected := []string{"foo", "bar"}
+	actual := SanitizePolicies([]string{"foo", "bar"}, false)
+	if !EquivalentPolicies(expected, actual) {
+		t.Fatal("bad: expected:%s\ngot:%s\n", expected, actual)
+	}
+
+	// If 'default' is already added, do not remove it.
+	expected = []string{"foo", "bar", "default"}
+	actual = SanitizePolicies([]string{"foo", "bar", "default"}, false)
+	if !EquivalentPolicies(expected, actual) {
+		t.Fatal("bad: expected:%s\ngot:%s\n", expected, actual)
+	}
+}
+
 func TestParsePolicies(t *testing.T) {
 	expected := []string{"foo", "bar", "default"}
 	actual := ParsePolicies("foo,bar")

vault/token_store.go

@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"regexp"
-	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -510,6 +509,8 @@ func (ts *TokenStore) create(entry *TokenEntry) error {
 		entry.ID = entryUUID
 	}
 
+	entry.Policies = policyutil.SanitizePolicies(entry.Policies, false)
+
 	err := ts.createAccessor(entry)
 	if err != nil {
 		return err
@@ -1001,8 +1002,8 @@ func (ts *TokenStore) handleCreateCommon(
 			data.Policies = role.AllowedPolicies
 		} else {
 			// Sanitize passed-in and role policies before comparison
-			sanitizedInputPolicies := policyutil.SanitizePolicies(data.Policies)
-			sanitizedRolePolicies := policyutil.SanitizePolicies(role.AllowedPolicies)
+			sanitizedInputPolicies := policyutil.SanitizePolicies(data.Policies, true)
+			sanitizedRolePolicies := policyutil.SanitizePolicies(role.AllowedPolicies, true)
 
 			if !strutil.StrListSubset(sanitizedRolePolicies, sanitizedInputPolicies) {
 				return logical.ErrorResponse(fmt.Sprintf("token policies (%v) must be subset of the role's allowed policies (%v)", sanitizedInputPolicies, sanitizedRolePolicies)), logical.ErrInvalidRequest
@@ -1016,33 +1017,16 @@ func (ts *TokenStore) handleCreateCommon(
 	// the client has root or sudo privileges
 	case !isSudo:
 		// Sanitize passed-in and parent policies before comparison
-		sanitizedInputPolicies := policyutil.SanitizePolicies(data.Policies)
-		sanitizedParentPolicies := policyutil.SanitizePolicies(parent.Policies)
+		sanitizedInputPolicies := policyutil.SanitizePolicies(data.Policies, true)
+		sanitizedParentPolicies := policyutil.SanitizePolicies(parent.Policies, true)
 
 		if !strutil.StrListSubset(sanitizedParentPolicies, sanitizedInputPolicies) {
 			return logical.ErrorResponse("child policies must be subset of parent"), logical.ErrInvalidRequest
 		}
 	}
 
-	// Use a map to filter out/prevent duplicates
-	policyMap := map[string]bool{}
-	for _, policy := range data.Policies {
-		if policy == "" {
-			// Don't allow a policy with no name, even though it is a valid
-			// slice member
-			continue
-		}
-		policyMap[policy] = true
-	}
-	if !policyMap["root"] &&
-		!data.NoDefaultPolicy {
-		policyMap["default"] = true
-	}
-
-	for k, _ := range policyMap {
-		te.Policies = append(te.Policies, k)
-	}
-	sort.Strings(te.Policies)
+	// Do not add the 'default' policy if requested not to.
+	te.Policies = policyutil.SanitizePolicies(data.Policies, !data.NoDefaultPolicy)
 
 	switch {
 	case role != nil:
@@ -1547,14 +1531,11 @@ func (ts *TokenStore) tokenStoreRoleCreateUpdate(
 		entry.PathSuffix = data.Get("path_suffix").(string)
 	}
 
-	allowedPoliciesInt, ok := data.GetOk("allowed_policies")
+	allowedPoliciesStr, ok := data.GetOk("allowed_policies")
 	if ok {
-		allowedPolicies := allowedPoliciesInt.(string)
-		if allowedPolicies != "" {
-			entry.AllowedPolicies = strings.Split(allowedPolicies, ",")
-		}
+		entry.AllowedPolicies = policyutil.ParsePolicies(allowedPoliciesStr.(string))
 	} else if req.Operation == logical.CreateOperation {
-		entry.AllowedPolicies = strings.Split(data.Get("allowed_policies").(string), ",")
+		entry.AllowedPolicies = policyutil.ParsePolicies(data.Get("allowed_policies").(string))
 	}
 
 	// Explicit max TTLs and periods cannot be used at the same time since the

vault/token_store_test.go

@@ -1270,7 +1270,7 @@ func TestTokenStore_RoleCRUD(t *testing.T) {
 		"name":             "test",
 		"orphan":           true,
 		"period":           int64(259200),
-		"allowed_policies": []string{"test1", "test2"},
+		"allowed_policies": []string{"default", "test1", "test2"},
 		"path_suffix":      "happenin",
 		"explicit_max_ttl": int64(0),
 	}
@@ -1311,7 +1311,7 @@ func TestTokenStore_RoleCRUD(t *testing.T) {
 		"name":             "test",
 		"orphan":           true,
 		"period":           int64(284400),
-		"allowed_policies": []string{"test3"},
+		"allowed_policies": []string{"default", "test3"},
 		"path_suffix":      "happenin",
 		"explicit_max_ttl": int64(0),
 	}
@@ -1358,7 +1358,7 @@ func TestTokenStore_RoleCRUD(t *testing.T) {
 		"name":             "test",
 		"orphan":           true,
 		"explicit_max_ttl": int64(5),
-		"allowed_policies": []string{"test3"},
+		"allowed_policies": []string{"default", "test3"},
 		"path_suffix":      "happenin",
 		"period":           int64(0),
 	}