Clone policy permissions and then use existing values rather than policy values for modifications (#2826)

Should fix #2804

vault/acl.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	"github.com/armon/go-radix"
+	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/vault/helper/strutil"
 	"github.com/hashicorp/vault/logical"
 )
@@ -51,7 +52,11 @@ func NewACL(policies []*Policy) (*ACL, error) {
 			// Check for an existing policy
 			raw, ok := tree.Get(pc.Prefix)
 			if !ok {
-				tree.Insert(pc.Prefix, pc.Permissions)
+				clonedPerms, err := pc.Permissions.Clone()
+				if err != nil {
+					return nil, errwrap.Wrapf("error cloning ACL permissions: {{err}}", err)
+				}
+				tree.Insert(pc.Prefix, clonedPerms)
 				continue
 			}
 
@@ -66,15 +71,15 @@ func NewACL(policies []*Policy) (*ACL, error) {
 
 			case pc.Permissions.CapabilitiesBitmap&DenyCapabilityInt > 0:
 				// If this new policy explicitly denies, only save the deny value
-				pc.Permissions.CapabilitiesBitmap = DenyCapabilityInt
+				existingPerms.CapabilitiesBitmap = DenyCapabilityInt
-				pc.Permissions.AllowedParameters = nil
+				existingPerms.AllowedParameters = nil
-				pc.Permissions.DeniedParameters = nil
+				existingPerms.DeniedParameters = nil
 				goto INSERT
 
 			default:
 				// Insert the capabilities in this new policy into the existing
 				// value
-				pc.Permissions.CapabilitiesBitmap = existingPerms.CapabilitiesBitmap | pc.Permissions.CapabilitiesBitmap
+				existingPerms.CapabilitiesBitmap = existingPerms.CapabilitiesBitmap | pc.Permissions.CapabilitiesBitmap
 			}
 
 			// Note: In these stanzas, we're preferring minimum lifetimes. So
@@ -85,59 +90,58 @@ func NewACL(policies []*Policy) (*ACL, error) {
 			// If we have an existing max, and we either don't have a current
 			// max, or the current is greater than the previous, use the
 			// existing.
-			if existingPerms.MaxWrappingTTL > 0 &&
+			if pc.Permissions.MaxWrappingTTL > 0 &&
-				(pc.Permissions.MaxWrappingTTL == 0 ||
+				(existingPerms.MaxWrappingTTL == 0 ||
-					existingPerms.MaxWrappingTTL < pc.Permissions.MaxWrappingTTL) {
+					pc.Permissions.MaxWrappingTTL < existingPerms.MaxWrappingTTL) {
-				pc.Permissions.MaxWrappingTTL = existingPerms.MaxWrappingTTL
+				existingPerms.MaxWrappingTTL = pc.Permissions.MaxWrappingTTL
 			}
 			// If we have an existing min, and we either don't have a current
 			// min, or the current is greater than the previous, use the
 			// existing
-			if existingPerms.MinWrappingTTL > 0 &&
+			if pc.Permissions.MinWrappingTTL > 0 &&
-				(pc.Permissions.MinWrappingTTL == 0 ||
+				(existingPerms.MinWrappingTTL == 0 ||
-					existingPerms.MinWrappingTTL < pc.Permissions.MinWrappingTTL) {
+					pc.Permissions.MinWrappingTTL < existingPerms.MinWrappingTTL) {
-				pc.Permissions.MinWrappingTTL = existingPerms.MinWrappingTTL
+				existingPerms.MinWrappingTTL = pc.Permissions.MinWrappingTTL
 			}
 
-			if len(existingPerms.AllowedParameters) > 0 {
+			if len(pc.Permissions.AllowedParameters) > 0 {
-				if pc.Permissions.AllowedParameters == nil {
+				if existingPerms.AllowedParameters == nil {
-					pc.Permissions.AllowedParameters = existingPerms.AllowedParameters
+					existingPerms.AllowedParameters = pc.Permissions.AllowedParameters
 				} else {
-					for key, value := range existingPerms.AllowedParameters {
+					for key, value := range pc.Permissions.AllowedParameters {
-						pcValue, ok := pc.Permissions.AllowedParameters[key]
+						pcValue, ok := existingPerms.AllowedParameters[key]
 						// If an empty array exist it should overwrite any other
 						// value.
 						if len(value) == 0 || (ok && len(pcValue) == 0) {
-							pc.Permissions.AllowedParameters[key] = []interface{}{}
+							existingPerms.AllowedParameters[key] = []interface{}{}
 						} else {
 							// Merge the two maps, appending values on key conflict.
-							pc.Permissions.AllowedParameters[key] = append(value, pc.Permissions.AllowedParameters[key]...)
+							existingPerms.AllowedParameters[key] = append(value, existingPerms.AllowedParameters[key]...)
 						}
 					}
 				}
 			}
 
-			if len(existingPerms.DeniedParameters) > 0 {
+			if len(pc.Permissions.DeniedParameters) > 0 {
-				if pc.Permissions.DeniedParameters == nil {
+				if existingPerms.DeniedParameters == nil {
-					pc.Permissions.DeniedParameters = existingPerms.DeniedParameters
+					existingPerms.DeniedParameters = pc.Permissions.DeniedParameters
 				} else {
-					for key, value := range existingPerms.DeniedParameters {
+					for key, value := range pc.Permissions.DeniedParameters {
-						pcValue, ok := pc.Permissions.DeniedParameters[key]
+						pcValue, ok := existingPerms.DeniedParameters[key]
 						// If an empty array exist it should overwrite any other
 						// value.
 						if len(value) == 0 || (ok && len(pcValue) == 0) {
-							pc.Permissions.DeniedParameters[key] = []interface{}{}
+							existingPerms.DeniedParameters[key] = []interface{}{}
 						} else {
 							// Merge the two maps, appending values on key conflict.
-							pc.Permissions.DeniedParameters[key] = append(value, pc.Permissions.DeniedParameters[key]...)
+							existingPerms.DeniedParameters[key] = append(value, existingPerms.DeniedParameters[key]...)
 						}
 					}
 				}
 			}
 
 		INSERT:
-
+			tree.Insert(pc.Prefix, existingPerms)
-			tree.Insert(pc.Prefix, pc.Permissions)
 
 		}
 	}

vault/acl_test.go

@@ -2,6 +2,7 @@ package vault
 
 import (
 	"reflect"
+	"sync"
 	"testing"
 	"time"
 
@@ -245,7 +246,7 @@ func TestACL_PolicyMerge(t *testing.T) {
 		{"allow/all1", nil, nil, map[string][]interface{}{"*": []interface{}{}, "test": []interface{}{}, "test1": []interface{}{"foo"}}, nil},
 		{"deny/all", nil, nil, nil, map[string][]interface{}{"*": []interface{}{}, "test": []interface{}{}}},
 		{"deny/all1", nil, nil, nil, map[string][]interface{}{"*": []interface{}{}, "test": []interface{}{}}},
-		{"value/merge", nil, nil, map[string][]interface{}{"test": []interface{}{1, 2, 3, 4}}, map[string][]interface{}{"test": []interface{}{1, 2, 3, 4}}},
+		{"value/merge", nil, nil, map[string][]interface{}{"test": []interface{}{3, 4, 1, 2}}, map[string][]interface{}{"test": []interface{}{3, 4, 1, 2}}},
 		{"value/empty", nil, nil, map[string][]interface{}{"empty": []interface{}{}}, map[string][]interface{}{"empty": []interface{}{}}},
 	}
 
@@ -415,6 +416,35 @@ func TestACL_ValuePermissions(t *testing.T) {
 	}
 }
 
+// NOTE: this test doesn't catch any races ATM
+func TestACL_CreationRace(t *testing.T) {
+	policy, err := Parse(valuePermissionsPolicy)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	var wg sync.WaitGroup
+	stopTime := time.Now().Add(20 * time.Second)
+
+	for i := 0; i < 50; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for {
+				if time.Now().After(stopTime) {
+					return
+				}
+				_, err := NewACL([]*Policy{policy})
+				if err != nil {
+					t.Fatalf("err: %v", err)
+				}
+			}
+		}()
+	}
+
+	wg.Wait()
+}
+
 var tokenCreationPolicy = `
 name = "tokenCreation"
 path "auth/token/create*" {

vault/policy.go

@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/hcl/hcl/ast"
 	"github.com/hashicorp/vault/helper/parseutil"
+	"github.com/mitchellh/copystructure"
 )
 
 const (
@@ -84,6 +85,40 @@ type Permissions struct {
 	DeniedParameters   map[string][]interface{}
 }
 
+func (p *Permissions) Clone() (*Permissions, error) {
+	ret := &Permissions{
+		CapabilitiesBitmap: p.CapabilitiesBitmap,
+		MinWrappingTTL:     p.MinWrappingTTL,
+		MaxWrappingTTL:     p.MaxWrappingTTL,
+	}
+
+	switch {
+	case p.AllowedParameters == nil:
+	case len(p.AllowedParameters) == 0:
+		ret.AllowedParameters = make(map[string][]interface{})
+	default:
+		clonedAllowed, err := copystructure.Copy(p.AllowedParameters)
+		if err != nil {
+			return nil, err
+		}
+		ret.AllowedParameters = clonedAllowed.(map[string][]interface{})
+	}
+
+	switch {
+	case p.DeniedParameters == nil:
+	case len(p.DeniedParameters) == 0:
+		ret.DeniedParameters = make(map[string][]interface{})
+	default:
+		clonedDenied, err := copystructure.Copy(p.DeniedParameters)
+		if err != nil {
+			return nil, err
+		}
+		ret.DeniedParameters = clonedDenied.(map[string][]interface{})
+	}
+
+	return ret, nil
+}
+
 // Parse is used to parse the specified ACL rules into an
 // intermediary set of policies, before being compiled into
 // the ACL