Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 7, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 33 additions & 29 deletions vault/acl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
}

Expand All @@ -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
Expand All @@ -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)

}
}
Expand Down
32 changes: 31 additions & 1 deletion vault/acl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package vault

import (
"reflect"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -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{}{}}},
}

Expand Down Expand Up @@ -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*" {
Expand Down
35 changes: 35 additions & 0 deletions vault/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeff why do we need both a nil check and a len == 0 check, Can you please add a comment to help understand it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure that the clone is an exact clone. A map of length 0 is an empty map, but not the same as a nil (unallocated) map.

case len(p.AllowedParameters) == 0:
ret.AllowedParameters = make(map[string][]interface{})
default:
clonedAllowed, err := copystructure.Copy(p.AllowedParameters)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy structure might cover the above case as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just doing this for efficiency rather than call into it!

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
Expand Down