🎉 Require 2 humans instead of 1 to be on ACL

Because we are not checking for typos in the provided user or group
names, we require 2 of more human principles to be in the ACL of a
SPIFFE-made Knox key.

client/create.go

@@ -27,8 +27,8 @@ Please run "knox create --key-template <template_name> <key_identifier>".
 
 The original key version id will be print to stdout.
 
-Only users or SPIFFEs can create a new key. For SPIFFEs, an ACL must be provided with at least 1 user or group set as the admin.
-If ACL is not provided, the default access list will include the creator of this key, as well as a limited set of site reliablity and security engineers.
+Only users or SPIFFEs can create a new key. For SPIFFEs, an ACL must be provided with at least 2 users/groups set as admins.
+The default ACL will include a limited set of site reliablity and security engineers, and the creator if they are a user.
 
 For more about knox, see https://github.com/pinterest/knox.
 
@@ -57,7 +57,7 @@ func parseAcl(aclString string) (knox.ACL, error) {
 	if err != nil {
 		return nil, err
 	}
-	err = acl.ValidateHasHumanAdmin()
+	err = acl.ValidateHasMultipleHumanAdmins()
 	if err != nil {
 		return nil, err
 	}

client/create_test.go

@@ -1,31 +1,32 @@
 package client
 
 import (
-	"fmt"
 	"testing"
 
 	"github.com/pinterest/knox"
 )
 
 func testAclEq(a, b knox.ACL) bool {
-    if len(a) != len(b) {
-        return false
-    }
-    for i := range a {
-        if a[i] != b[i] {
-            return false
-        }
-    }
-    return true
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
 }
 
 func TestParseAcl(t *testing.T) {
 	machineAdmin := knox.Access{ID: "testmachine1", AccessType: knox.Admin, Type: knox.Machine}
 	userWrite := knox.Access{ID: "testuser", AccessType: knox.Write, Type: knox.User}
 	userAdmin := knox.Access{ID: "testuser", AccessType: knox.Admin, Type: knox.User}
+	groupAdmin := knox.Access{ID: "testgroup", AccessType: knox.Admin, Type: knox.UserGroup}
 
 	validAclNoHumanAdmin := knox.ACL([]knox.Access{machineAdmin, userWrite})
-	validAclWithHumanAdmin := knox.ACL([]knox.Access{machineAdmin, userAdmin})
+	validAclWithOneHumanAdmin := knox.ACL([]knox.Access{machineAdmin, userAdmin})
+	validAclWithTwoHumanAdmins := knox.ACL([]knox.Access{machineAdmin, userAdmin, groupAdmin})
 	blankAcl := knox.ACL{}
 
 	testCases := []struct {
@@ -38,29 +39,34 @@ func TestParseAcl(t *testing.T) {
 			validAclNoHumanAdmin,  // ACL does not matter here
 			"json: Invalid AccessType to convert",
 		},
-	    {
+		{
 			`[{"type":"User","id":"testuser","access":"Write"}, {"type":"Machine","id":"testmachine1","access":"Admin"}]`,
 			validAclNoHumanAdmin,
-			"ACL needs to have a user or group set as an admin", // User only has Write access
-	    },
-	    {
+			"ACL needs to have at least 2 users/groups set as admins", // User only has Write access
+		},
+		{
 			`[{"type":"Machine","id":"testmachine1","access":"Admin"}, {"type":"User","id":"testuser","access":"Admin"}]`,
-			validAclWithHumanAdmin,
+			validAclWithOneHumanAdmin,
+			"ACL needs to have at least 2 users/groups set as admins", // Only 1 human admin
+		},
+		{
+			`[{"type":"Machine","id":"testmachine1","access":"Admin"}, {"type":"User","id":"testuser","access":"Admin"}, {"type":"UserGroup","id":"testgroup","access":"Admin"}]`,
+			validAclWithTwoHumanAdmins,
+			"", // Success, no error
+		},
+		// Original, and default, behaviour is a blank ACL
+		{
+			``,
+			blankAcl,
 			"", // Success, no error
-	    },
-	    // Original, and default, behaviour is a blank ACL
-        {
-            ``,
-            blankAcl,
-            "", // Success, no error
-        },
+		},
 	}
 
 	for _, tc := range testCases {
 		acl, err := parseAcl(tc.str)
 		if tc.errMsg != "" || err != nil {
 			if err.Error() != tc.errMsg {
-				t.Fatal(err)
+				t.Fatalf("%v should equal %v", tc.errMsg, err.Error())
 			}
 		} else{
 			if !testAclEq(acl, tc.acl) {

client_test.go

@@ -199,12 +199,17 @@ func TestCreateKey(t *testing.T) {
 
 	cli := MockClient(srv.Listener.Addr().String(), "")
 
-	aclWithUserAdmin := ACL([]Access{
+	aclWithMultipleUserAdmins := ACL([]Access{
 		{
 			Type:       User,
 			AccessType: Admin,
 			ID:         "test",
 		},
+		{
+			Type:       User,
+			AccessType: Admin,
+			ID:         "test2",
+		},
 	})
 	aclWithUserRead := ACL([]Access{
 		{
@@ -238,7 +243,7 @@ func TestCreateKey(t *testing.T) {
 		t.Fatalf("Expected err is %v", err)
 	}
 
-	// Note: In `client/create.go` and `server/routes.go`, acl.ValidateHasHumanAdmin() would be called, but not in `client.go`
+	// Note: In `client/create.go` and `server/routes.go`, acl.ValidateHasMultipleHumanAdmins() would be called, but not in `client.go`
 	k, err := cli.CreateKey("testkey", DataBytes, aclWithUserRead)
 	if err != nil {
 		t.Fatalf("%s is not nil", err)
@@ -247,7 +252,7 @@ func TestCreateKey(t *testing.T) {
 		t.Fatalf("%d is not %d", k, expected)
 	}
 
-	k, err = cli.CreateKey("testkey", DataBytes, aclWithUserAdmin)
+	k, err = cli.CreateKey("testkey", DataBytes, aclWithMultipleUserAdmins)
 	if err != nil {
 		t.Fatalf("%s is not nil", err)
 	}

knox.go

@@ -17,7 +17,7 @@ var (
 	ErrACLDuplicateEntries = fmt.Errorf("Duplicate entries in ACL")
 	ErrACLContainsNone     = fmt.Errorf("ACL contains None access")
 	ErrACLEmptyPrincipal   = fmt.Errorf("Principals of type user, user group, machine, or machine prefix may not be empty.")
-	ErrACLDoesNotContainHumanAdmin = fmt.Errorf("ACL needs to have a user or group set as an admin")
+	ErrACLDoesNotContainMultipleHumanAdmins = fmt.Errorf("ACL needs to have at least 2 users/groups set as admins")
 
 	ErrACLInvalidService               = fmt.Errorf("Service is invalid, must conform to 'spiffe://<domain>/<path>' format.")
 	ErrACLInvalidServicePrefixURL      = fmt.Errorf("Service prefix is invalid URL, must conform to 'spiffe://<domain>/<path>/' format.")
@@ -321,17 +321,24 @@ func (acl ACL) Validate() error {
 	return nil
 }
 
-// ValidateHasHumanAdmin ensures the ACL has at least 1 user or group set as an Admin. This is for when SPIFFEs create keys, we want to ensure there is a human owner set. Intended to be called separately from Validate.
-func (acl ACL) ValidateHasHumanAdmin() error {
+// ValidateHasMultipleHumanAdmins ensures the ACL has at least 2 users/groups set as 'Admin's.
+// This is for when SPIFFEs create keys, we want to ensure there are multiple human owners set.
+// Intended to be called separately from Validate().
+func (acl ACL) ValidateHasMultipleHumanAdmins() error {
+	var humanAdminCount uint64
+	humanAdminCount = 0
 	for _, a := range acl {
 		if a.AccessType != Admin {
 			continue;
 		}
 		if a.Type == User || a.Type == UserGroup {
+			humanAdminCount++
+		}
+		if humanAdminCount > 1 {
 			return nil
 		}
 	}
-	return ErrACLDoesNotContainHumanAdmin
+	return ErrACLDoesNotContainMultipleHumanAdmins
 }
 
 // Add appends an access to the ACL. It does so by overwriting any existing access
@@ -603,7 +610,7 @@ const (
 	BadKeyFormatCode
 	BadPrincipalIdentifier
 	BadAclCode
-	NoHumanAdminInAclCode
+	NoMultipleHumanAdminsInAclCode
 )
 
 // Response is the format for responses from the api server.

knox_test.go

@@ -231,30 +231,39 @@ func TestACLValidate(t *testing.T) {
 	}
 }
 
-func TestACLValidateHasHumanAdmin(t *testing.T) {
+func TestACLValidateHasMultipleHumanAdminss(t *testing.T) {
 	var accessEntries []Access
 
 	machineAdmin := Access{ID: "testmachine1", AccessType: Admin, Type: Machine}
-	userWrite := Access{ID: "testuser", AccessType: Write, Type: User}
+	userWrite := Access{ID: "testuserwrite", AccessType: Write, Type: User}
 	machinePrefixRead := Access{ID: "testmachine", AccessType: Read, Type: MachinePrefix}
 	serviceRead := Access{ID: "spiffe://example.com/serviceA", AccessType: Read, Type: Service}
 	servicePrefixRead := Access{ID: "spiffe://example.com/serviceA/", AccessType: Read, Type: ServicePrefix}
 
 	accessEntries = []Access{machineAdmin, userWrite, machinePrefixRead, serviceRead, servicePrefixRead}
+	// No human Admins
 	noHumanAdmin := ACL(accessEntries)
-	if noHumanAdmin.ValidateHasHumanAdmin() != ErrACLDoesNotContainHumanAdmin {
+	if noHumanAdmin.ValidateHasMultipleHumanAdmins() != ErrACLDoesNotContainMultipleHumanAdmins {
 		t.Error("ValidACL should not be valid")
 	}
 
-	userAdmin := Access{ID: "testuser", AccessType: Admin, Type: User}
+	// Only 1 user Admin
+	userAdmin := Access{ID: "testuseradmin", AccessType: Admin, Type: User}
 	validWithUserAdmin := ACL(append(noHumanAdmin, userAdmin))
-	if validWithUserAdmin.ValidateHasHumanAdmin() != nil {
+	if validWithUserAdmin.ValidateHasMultipleHumanAdmins() != ErrACLDoesNotContainMultipleHumanAdmins {
 		t.Error("ValidACL should be valid")
 	}
 
-	userGroupAdmin := Access{ID: "testuser", AccessType: Admin, Type: UserGroup}
+	// Only 1 group Admin
+	userGroupAdmin := Access{ID: "testgroup", AccessType: Admin, Type: UserGroup}
 	validWithGroupAdmin := ACL(append(noHumanAdmin, userGroupAdmin))
-	if validWithGroupAdmin.ValidateHasHumanAdmin() != nil {
+	if validWithGroupAdmin.ValidateHasMultipleHumanAdmins() != ErrACLDoesNotContainMultipleHumanAdmins {
+		t.Error("ValidACL should be valid")
+	}
+
+	// Success, both user admin and group admin
+	validWithMultipleHumanAdmins := ACL(append(noHumanAdmin, userAdmin, userGroupAdmin))
+	if validWithMultipleHumanAdmins.ValidateHasMultipleHumanAdmins() != nil {
 		t.Error("ValidACL should be valid")
 	}
 }

server/routes.go

@@ -149,18 +149,18 @@ func postKeysHandler(m KeyManager, principal knox.Principal, parameters map[stri
 		}
 	}
 
-	// Only users -- and SPIFFEs when a human group/user is an admin in the ACL -- can create keys
+	// Only users -- and SPIFFEs when at latest 2 human users/groups are admins in the ACL -- can create keys
 	if auth.IsService(principal) {
 		err = acl.Validate()
 		if err != nil {
 			return nil, errF(knox.BadAclCode, "Error validating parameter 'acl'")
 		}
-		err = acl.ValidateHasHumanAdmin()
+		err = acl.ValidateHasMultipleHumanAdmins()
 		if err != nil {
-			return nil, errF(knox.NoHumanAdminInAclCode, "Parameter 'acl' does not have human admin")
+			return nil, errF(knox.NoMultipleHumanAdminsInAclCode, "Parameter 'acl' does not have multiple human admins")
 		}
 	} else if !auth.IsUser(principal) {
-		return nil, errF(knox.UnauthorizedCode, fmt.Sprintf("Must be a user (or SPIFFE if human admin in ACL) to create keys, principal is %s", principal.GetID()))
+		return nil, errF(knox.UnauthorizedCode, fmt.Sprintf("Must be a user (or SPIFFE if multiple human admins in ACL) to create keys, principal is %s", principal.GetID()))
 	}
 
 	keyID, keyIDOK := parameters["id"]

server/routes_test.go

@@ -99,7 +99,7 @@ func TestPostKeys(t *testing.T) {
 		t.Fatal("Expected err")
 	} else if err.Subcode != knox.UnauthorizedCode {
 		t.Fatalf("Expected %v and got %v", knox.UnauthorizedCode, err.Subcode)
-	} else if err.Message != "Must be a user (or SPIFFE if human admin in ACL) to create keys, principal is MrRoboto" {
+	} else if err.Message != "Must be a user (or SPIFFE if multiple human admins in ACL) to create keys, principal is MrRoboto" {
 		t.Fatalf("Unexpected error message: %v", err.Message)
 	}
 
@@ -118,13 +118,22 @@ func TestPostKeys(t *testing.T) {
 	_, err = postKeysHandler(m, serviceA, map[string]string{"id": "a1", "data": Number1B64Encoded, "acl": `[{"type":"User","id":"testuser","access":"Write"}, {"type":"Machine","id":"testmachine1","access":"Admin"}]`})
 	if err == nil {
 		t.Fatal("Expected err")
-	} else if err.Subcode != knox.NoHumanAdminInAclCode {
-		t.Fatalf("Expected %v and got %v", knox.NoHumanAdminInAclCode, err.Subcode)
-	} else if err.Message != "Parameter 'acl' does not have human admin" {
+	} else if err.Subcode != knox.NoMultipleHumanAdminsInAclCode {
+		t.Fatalf("Expected %v and got %v", knox.NoMultipleHumanAdminsInAclCode, err.Subcode)
+	} else if err.Message != "Parameter 'acl' does not have multiple human admins" {
 		t.Fatalf("Unexpected error message: %v", err.Message)
 	}
-	// Valid ACL with human admin
+	// Valid ACL with only 1 human admin
 	_, err = postKeysHandler(m, serviceA, map[string]string{"id": "a0", "data": Number1B64Encoded, "acl": `[{"type":"User","id":"testuser","access":"Admin"}, {"type":"Machine","id":"testmachine1","access":"Admin"}]`})
+	if err == nil {
+		t.Fatal("Expected err")
+	} else if err.Subcode != knox.NoMultipleHumanAdminsInAclCode {
+		t.Fatalf("Expected %v and got %v", knox.NoMultipleHumanAdminsInAclCode, err.Subcode)
+	} else if err.Message != "Parameter 'acl' does not have multiple human admins" {
+		t.Fatalf("Unexpected error message: %v", err.Message)
+	}
+	// Valid ACL with 2 human admins
+	_, err = postKeysHandler(m, serviceA, map[string]string{"id": "a0", "data": Number1B64Encoded, "acl": `[{"type":"User","id":"testuser","access":"Admin"}, {"type":"User","id":"testuser2","access":"Admin"}]`})
 	if err != nil {
 		t.Fatalf("%+v is not nil", err)
 	}