Permissions were changed from a structure to and array of interfaces. Code optimization for acl.go. Fixed bug where multiple parameters would allow if second or following parameters were denied and there was a wildcard in allow.

vault/acl.go

@@ -52,72 +52,63 @@ func NewACL(policies []*Policy) (*ACL, error) {
 			}
 
 			// these are the ones already in the tree
-			permissions := raw.(*Permissions)
-			existing := permissions.CapabilitiesBitmap
+			existingPerms := raw.(*Permissions)
 
 			switch {
-			case existing&DenyCapabilityInt > 0:
+			case existingPerms.CapabilitiesBitmap&DenyCapabilityInt > 0:
 				// If we are explicitly denied in the existing capability set,
 				// don't save anything else
+				continue
 
 			case pc.Permissions.CapabilitiesBitmap&DenyCapabilityInt > 0:
 				// If this new policy explicitly denies, only save the deny value
 				pc.Permissions.CapabilitiesBitmap = DenyCapabilityInt
-				tree.Insert(pc.Prefix, pc.Permissions)
+				pc.Permissions.AllowedParameters = nil
+				pc.Permissions.DeniedParameters = nil
+				goto INSERT
 
 			default:
 				// Insert the capabilities in this new policy into the existing
 				// value
-				pc.Permissions.CapabilitiesBitmap = existing | pc.Permissions.CapabilitiesBitmap
-				tree.Insert(pc.Prefix, pc.Permissions)
+				pc.Permissions.CapabilitiesBitmap = existingPerms.CapabilitiesBitmap | pc.Permissions.CapabilitiesBitmap
 			}
 
-			// look for a * in allowed parameters for the node already in the tree
-			if _, ok := permissions.AllowedParameters["*"]; ok {
-				pc.Permissions.AllowedParameters = make(map[string]struct{})
-				pc.Permissions.AllowedParameters["*"] = struct{}{}
-				goto CHECK_DENIED
-			}
-
-			// look for a * in allowed parameters for the path capability we are merging
-			if _, ok := pc.Permissions.AllowedParameters["*"]; ok {
-				pc.Permissions.AllowedParameters = make(map[string]struct{})
-				pc.Permissions.AllowedParameters["*"] = struct{}{}
-				goto CHECK_DENIED
-			}
-
-			if pc.Permissions.AllowedParameters == nil {
-				pc.Permissions.AllowedParameters = make(map[string]struct{})
-			}
-			// Merge allowed parameters
-			for key, value := range permissions.AllowedParameters {
-				// Add new parameter
-				pc.Permissions.AllowedParameters[key] = value
+			if len(existingPerms.AllowedParameters) > 0 {
+				if pc.Permissions.AllowedParameters == nil {
+					pc.Permissions.AllowedParameters = make(map[string][]interface{}, len(existingPerms.AllowedParameters))
+				}
+				if _, ok := pc.Permissions.AllowedParameters["*"]; ok {
+					goto CHECK_DENIED
+				}
+				for key, value := range existingPerms.AllowedParameters {
+					if key == "*" {
+						pc.Permissions.AllowedParameters = map[string][]interface{}{
+							"*": []interface{}{},
+						}
+						goto CHECK_DENIED
+					}
+					pc.Permissions.AllowedParameters[key] = value
+				}
 			}
 
 		CHECK_DENIED:
 
-			// look for a * in denied parameters for the node already in the tree
-			if _, ok := permissions.DeniedParameters["*"]; ok {
-				pc.Permissions.DeniedParameters = make(map[string]struct{})
-				pc.Permissions.DeniedParameters["*"] = struct{}{}
-				goto INSERT
-			}
-
-			// look for a * in denied parameters for the path capability we are merging
-			if _, ok := pc.Permissions.DeniedParameters["*"]; ok {
-				pc.Permissions.DeniedParameters = make(map[string]struct{})
-				pc.Permissions.DeniedParameters["*"] = struct{}{}
-				goto INSERT
-			}
-
-			if pc.Permissions.DeniedParameters == nil {
-				pc.Permissions.DeniedParameters = make(map[string]struct{})
-			}
-			// Merge denied parameters
-			for key, value := range permissions.DeniedParameters {
-				// Add new parameter
-				pc.Permissions.DeniedParameters[key] = value
+			if len(existingPerms.DeniedParameters) > 0 {
+				if pc.Permissions.DeniedParameters == nil {
+					pc.Permissions.DeniedParameters = make(map[string][]interface{}, len(existingPerms.DeniedParameters))
+				}
+				if _, ok := pc.Permissions.DeniedParameters["*"]; ok {
+					goto INSERT
+				}
+				for key, value := range existingPerms.DeniedParameters {
+					if key == "*" {
+						pc.Permissions.DeniedParameters = map[string][]interface{}{
+							"*": []interface{}{},
+						}
+						break
+					}
+					pc.Permissions.DeniedParameters[key] = value
+				}
 			}
 
 		INSERT:
@@ -254,18 +245,20 @@ CHECK:
 		if _, ok := permissions.DeniedParameters["*"]; ok {
 			return false, sudo
 		}
-
+		allowedAll := false
+		if _, ok := permissions.AllowedParameters["*"]; ok {
+			allowedAll = true
+		}
+		if len(permissions.DeniedParameters) == 0 && allowedAll {
+			return true, sudo
+		}
 		for parameter, _ := range req.Data {
 			// Check if parameter has explictly denied
 			if _, ok := permissions.DeniedParameters[parameter]; ok {
 				return false, sudo
 			}
-			// Check if all parameters have been allowed.
-			if _, ok := permissions.AllowedParameters["*"]; ok {
-				return true, sudo
-			}
 			// Specfic parameters have been allowed
-			if len(permissions.AllowedParameters) > 0 {
+			if len(permissions.AllowedParameters) > 0 && !allowedAll {
 				// Requested parameter is not in allowed list
 				if _, ok := permissions.AllowedParameters[parameter]; !ok {
 					return false, sudo

vault/acl_test.go

@@ -267,7 +267,7 @@ func TestPolicyMerge(t *testing.T) {
 	}
 }
 func TestAllowOperation(t *testing.T) {
-  policy, err := Parse(permissionsPolicy)
+	policy, err := Parse(permissionsPolicy)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -275,35 +275,38 @@ func TestAllowOperation(t *testing.T) {
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
-  toperations := []logical.Operation {
-    logical.UpdateOperation,
-    logical.DeleteOperation,
-    logical.CreateOperation,
-  }
-	type tcase struct {
-		path  string
-		parameter string
-		allowed   bool
-		rootPrivs bool
+	toperations := []logical.Operation{
+		logical.UpdateOperation,
+		logical.DeleteOperation,
+		logical.CreateOperation,
 	}
-	
+	type tcase struct {
+		path       string
+		parameters []string
+		allowed    bool
+		rootPrivs  bool
+	}
+
 	tcases := []tcase{
-		{"dev/ops", "zip", true, false},
-		{"foo/bar", "zap", false, false},
-		{"foo/baz", "hello", true, false},
-		{"foo/baz", "zap", false, false},
-		{"broken/phone", "steve", false, false},
-		{"hello/world", "one", false, false},
-		{"tree/fort", "one", true, false},
-		{"tree/fort", "beer", false, false},
-		{"fruit/apple", "pear", false, false},
-		{"fruit/apple", "one", false, false},
-		{"cold/weather", "four", true, false},
-  }
-  
-  for _, tc := range tcases {
+		{"dev/ops", []string{"zip"}, true, false},
+		{"foo/bar", []string{"zap"}, false, false},
+		{"foo/baz", []string{"hello"}, true, false},
+		{"foo/baz", []string{"zap"}, false, false},
+		{"broken/phone", []string{"steve"}, false, false},
+		{"hello/world", []string{"one"}, false, false},
+		{"tree/fort", []string{"one"}, true, false},
+		{"tree/fort", []string{"beer"}, false, false},
+		{"fruit/apple", []string{"pear"}, false, false},
+		{"fruit/apple", []string{"one"}, false, false},
+		{"cold/weather", []string{"four"}, true, false},
+		{"var/aws", []string{"cold", "warm", "kitty"}, false, false},
+	}
+
+	for _, tc := range tcases {
 		request := logical.Request{Path: tc.path, Data: make(map[string]interface{})}
-		request.Data[tc.parameter] = ""
+		for _, parameter := range tc.parameters {
+			request.Data[parameter] = ""
+		}
 		for _, op := range toperations {
 			request.Operation = op
 			allowed, rootPrivs := acl.AllowOperation(&request)
@@ -317,107 +320,6 @@ func TestAllowOperation(t *testing.T) {
 	}
 }
 
-//test merging
-
-var permissionsPolicy2 = `
-name = "ops"
-path "foo/bar" {
-	policy = "write"
-	permissions = {
-		deniedparameters = {
-			"baz" = {}
-		}
-	}
-}
-path "foo/bar" {
-	policy = "write"
-	permissions = {
-		deniedparameters = {
-			"zip" = {}
-		}
-	}
-}
-path "hello/universe" {
-	policy = "write"
-	permissions = {
-		allowedparameters = {
-			"bob" = {}
-		}
-	}
-}
-path "hello/universe" {
-	policy = "write"
-	permissions = {
-		allowedparameters = {
-			"tom" = {}
-		}
-  }
-}
-path "rainy/day" {
-	policy = "write"
-	permissions = {
-		allowedparameters = {
-			"bob" = {}
-		}
-	}
-}
-path "rainy/day" {
-	policy = "write"
-	permissions = {
-		allowedparameters = {
-			"*" = {}
-		}
-  }
-}
-path "cool/bike" {
-	policy = "write"
-	permissions = {
-		deniedparameters = {
-			"frank" = {}
-		}
-	}
-}
-path "cool/bike" {
-	policy = "write"
-	permissions = {
-		deniedparameters = {
-			"*" = {}
-		}
-  }
-}
-path "clean/bed" {
-	policy = "write"
-	permissions = {
-		deniedparameters = {
-			"*" = {}
-		}
-	}
-}
-path "clean/bed" {
-	policy = "write"
-	permissions = {
-		allowedparameters = {
-			"*" = {}
-		}
-  }
-}
-path "coca/cola" {
-	policy = "write"
-	permissions = {
-		deniedparameters = {
-			"john" = {}
-		}
-	}
-}
-path "coca/cola" {
-	policy = "write"
-	permissions = {
-		allowedparameters = {
-			"john" = {}
-		}
-  }
-}
-`
 var tokenCreationPolicy = `
 name = "tokenCreation"
 path "auth/token/create*" {
@@ -475,6 +377,107 @@ path "foo/bar" {
 }
 `
 
+//test merging
+var permissionsPolicy2 = `
+name = "ops"
+path "foo/bar" {
+	policy = "write"
+	permissions = {
+		deniedparameters = {
+			"baz" = []
+		}
+	}
+}
+path "foo/bar" {
+	policy = "write"
+	permissions = {
+		deniedparameters = {
+			"zip" = []
+		}
+	}
+}
+path "hello/universe" {
+	policy = "write"
+	permissions = {
+		allowedparameters = {
+			"bob" = []
+		}
+	}
+}
+path "hello/universe" {
+	policy = "write"
+	permissions = {
+		allowedparameters = {
+			"tom" = []
+		}
+  }
+}
+path "rainy/day" {
+	policy = "write"
+	permissions = {
+		allowedparameters = {
+			"bob" = []
+		}
+	}
+}
+path "rainy/day" {
+	policy = "write"
+	permissions = {
+		allowedparameters = {
+			"*" = []
+		}
+  }
+}
+path "cool/bike" {
+	policy = "write"
+	permissions = {
+		deniedparameters = {
+			"frank" = []
+		}
+	}
+}
+path "cool/bike" {
+	policy = "write"
+	permissions = {
+		deniedparameters = {
+			"*" = []
+		}
+  }
+}
+path "clean/bed" {
+	policy = "write"
+	permissions = {
+		deniedparameters = {
+			"*" = []
+		}
+	}
+}
+path "clean/bed" {
+	policy = "write"
+	permissions = {
+		allowedparameters = {
+			"*" = []
+		}
+  }
+}
+path "coca/cola" {
+	policy = "write"
+	permissions = {
+		deniedparameters = {
+			"john" = []
+		}
+	}
+}
+path "coca/cola" {
+	policy = "write"
+	permissions = {
+		allowedparameters = {
+			"john" = []
+		}
+  }
+}
+`
+
 //allow operation testing
 var permissionsPolicy = `
 name = "dev"
@@ -483,7 +486,7 @@ path "dev/*" {
 	
   permissions = {
   	allowedparameters = {
-  		"zip" = {}
+  		"zip" = []
   	}
   }
 }
@@ -491,7 +494,7 @@ path "foo/bar" {
 	policy = "write"
 	permissions = {
 		deniedparameters = {
-			"zap" = {}
+			"zap" = []
 		}
   }
 }
@@ -499,10 +502,10 @@ path "foo/baz" {
 	policy = "write"
 	permissions = {
 		allowedparameters = {
-			"hello" = {}
+			"hello" = []
 		}
 		deniedparameters = {
-			"zap" = {}
+			"zap" = []
 		}
   }
 }
@@ -510,10 +513,10 @@ path "broken/phone" {
 	policy = "write"
 	permissions = {
 		allowedparameters = {
-		  "steve" = {}
+		  "steve" = []
 		}
 		deniedparameters = {
-		  "steve" = {}
+		  "steve" = []
 		}
 	}
 }
@@ -521,10 +524,10 @@ path "hello/world" {
 	policy = "write"
 	permissions = {
 		allowedparameters = {
-			"*" = {}
+			"*" = []
 		}
 		deniedparameters = {
-			"*" = {}
+			"*" = []
 		}
   }
 }
@@ -532,10 +535,10 @@ path "tree/fort" {
 	policy = "write"
 	permissions = {
 		allowedparameters = {
-			"*" = {}
+			"*" = []
 		}
 		deniedparameters = {
-			"beer" = {}
+			"beer" = []
 		}
   }
 }
@@ -543,10 +546,10 @@ path "fruit/apple" {
 	policy = "write"
 	permissions = {
 		allowedparameters = {
-			"pear" = {}
+			"pear" = []
 		}
 		deniedparameters = {
-			"*" = {}
+			"*" = []
 		}
   }
 }
@@ -557,4 +560,17 @@ path "cold/weather" {
 		deniedparameters = {}
 	}
 }
+path "var/aws" {
+  	policy = "write"
+	permissions = {
+	  	allowedparameters = {
+			"*" = []
+		}
+		deniedparameters = {
+		  	"soft" = []
+			"warm" = []
+			"kitty" = []
+		}
+	}
+}
 `

vault/policy.go

@@ -69,8 +69,8 @@ type PathCapabilities struct {
 
 type Permissions struct {
 	CapabilitiesBitmap uint32
-	AllowedParameters  map[string]struct{}
-	DeniedParameters   map[string]struct{}
+	AllowedParameters  map[string][]interface{}
+	DeniedParameters   map[string][]interface{}
 }
 
 // Parse is used to parse the specified ACL rules into an

vault/policy_test.go

@@ -42,8 +42,8 @@ path "foo/bar" {
 	capabilities = ["create", "sudo"]
 	permissions = {
 	  allowedparameters = {
-	    "zip" = {}
-	    "zap" = {}
+	    "zip" = []
+	    "zap" = []
 	  }
 	}
 }
@@ -53,8 +53,8 @@ path "baz/bar" {
 	capabilities = ["create", "sudo"]
 	permissions = {
 	  deniedparameters = {
-	    "zip" = {}
-	    "zap" = {}
+	    "zip" = []
+	    "zap" = []
 	  }
 	}
 }
@@ -64,12 +64,12 @@ path "biz/bar" {
 	capabilities = ["create", "sudo"]
 	permissions = {
 	  allowedparameters = {
-	    "zim" = {}
-	    "zam" = {}
+	    "zim" = []
+	    "zam" = []
 	  }
 	  deniedparameters = {
-	    "zip" = {}
-	    "zap" = {}
+	    "zip" = []
+	    "zap" = []
 	  }
 	}
 }
@@ -120,19 +120,19 @@ func TestPolicy_Parse(t *testing.T) {
 				"create",
 				"sudo",
 			}, &Permissions{(CreateCapabilityInt | SudoCapabilityInt),
-				map[string]struct{}{"zip": {}, "zap": {}}, nil}, false},
+				map[string][]interface{}{"zip": {}, "zap": {}}, nil}, false},
 		&PathCapabilities{"baz/bar", "",
 			[]string{
 				"create",
 				"sudo",
 			}, &Permissions{(CreateCapabilityInt | SudoCapabilityInt),
-				nil, map[string]struct{}{"zip": {}, "zap": {}}}, false},
+				nil, map[string][]interface{}{"zip": {}, "zap": {}}}, false},
 		&PathCapabilities{"biz/bar", "",
 			[]string{
 				"create",
 				"sudo",
 			}, &Permissions{(CreateCapabilityInt | SudoCapabilityInt),
-				map[string]struct{}{"zim": {}, "zam": {}}, map[string]struct{}{"zip": {}, "zap": {}}}, false},
+				map[string][]interface{}{"zim": {}, "zam": {}}, map[string][]interface{}{"zip": {}, "zap": {}}}, false},
 	}
 	if !reflect.DeepEqual(p.Paths, expect) {
 		t.Errorf("expected \n\n%#v\n\n to be \n\n%#v\n\n", p.Paths, expect)