From 7db7c7a5a66a55ed0dfabaa482345595445b86bd Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 6 Jun 2017 23:57:00 -0400 Subject: [PATCH] Clone policy permissions and then use existing values rather than policy values for modifications. Should fix #2804 --- vault/acl.go | 62 +++++++++++++++++++++++++---------------------- vault/acl_test.go | 32 +++++++++++++++++++++++- vault/policy.go | 35 ++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 30 deletions(-) diff --git a/vault/acl.go b/vault/acl.go index 550e0df029fc..73601788f4d9 100644 --- a/vault/acl.go +++ b/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 - pc.Permissions.AllowedParameters = nil - pc.Permissions.DeniedParameters = nil + existingPerms.CapabilitiesBitmap = DenyCapabilityInt + existingPerms.AllowedParameters = 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 && - (pc.Permissions.MaxWrappingTTL == 0 || - existingPerms.MaxWrappingTTL < pc.Permissions.MaxWrappingTTL) { - pc.Permissions.MaxWrappingTTL = existingPerms.MaxWrappingTTL + if pc.Permissions.MaxWrappingTTL > 0 && + (existingPerms.MaxWrappingTTL == 0 || + 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 && - (pc.Permissions.MinWrappingTTL == 0 || - existingPerms.MinWrappingTTL < pc.Permissions.MinWrappingTTL) { - pc.Permissions.MinWrappingTTL = existingPerms.MinWrappingTTL + if pc.Permissions.MinWrappingTTL > 0 && + (existingPerms.MinWrappingTTL == 0 || + pc.Permissions.MinWrappingTTL < existingPerms.MinWrappingTTL) { + existingPerms.MinWrappingTTL = pc.Permissions.MinWrappingTTL } - if len(existingPerms.AllowedParameters) > 0 { - if pc.Permissions.AllowedParameters == nil { - pc.Permissions.AllowedParameters = existingPerms.AllowedParameters + if len(pc.Permissions.AllowedParameters) > 0 { + if existingPerms.AllowedParameters == nil { + existingPerms.AllowedParameters = pc.Permissions.AllowedParameters } else { - for key, value := range existingPerms.AllowedParameters { - pcValue, ok := pc.Permissions.AllowedParameters[key] + for key, value := range pc.Permissions.AllowedParameters { + 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 pc.Permissions.DeniedParameters == nil { - pc.Permissions.DeniedParameters = existingPerms.DeniedParameters + if len(pc.Permissions.DeniedParameters) > 0 { + if existingPerms.DeniedParameters == nil { + existingPerms.DeniedParameters = pc.Permissions.DeniedParameters } else { - for key, value := range existingPerms.DeniedParameters { - pcValue, ok := pc.Permissions.DeniedParameters[key] + for key, value := range pc.Permissions.DeniedParameters { + 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, pc.Permissions) + tree.Insert(pc.Prefix, existingPerms) } } diff --git a/vault/acl_test.go b/vault/acl_test.go index 7eb45b8b43e9..2d96b577dabf 100644 --- a/vault/acl_test.go +++ b/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*" { diff --git a/vault/policy.go b/vault/policy.go index c808c2a866f4..79f3b564a7f3 100644 --- a/vault/policy.go +++ b/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