From a59b437b4e0bc511769e329b0e4a4aafcb8953b4 Mon Sep 17 00:00:00 2001 From: Kevin Hock Date: Mon, 6 Feb 2023 23:05:31 +0000 Subject: [PATCH] :tada: 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 | 6 ++--- client/create_test.go | 54 ++++++++++++++++++++++++------------------- client_test.go | 11 ++++++--- knox.go | 17 ++++++++++---- knox_test.go | 23 ++++++++++++------ server/routes.go | 8 +++---- server/routes_test.go | 19 +++++++++++---- 7 files changed, 87 insertions(+), 51 deletions(-) diff --git a/client/create.go b/client/create.go index 7bcd296..f59e35b 100644 --- a/client/create.go +++ b/client/create.go @@ -27,8 +27,8 @@ Please run "knox create --key-template ". 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 } diff --git a/client/create_test.go b/client/create_test.go index bd0eae3..0b04dd8 100644 --- a/client/create_test.go +++ b/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) { diff --git a/client_test.go b/client_test.go index b4b4b7a..c677666 100644 --- a/client_test.go +++ b/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) } diff --git a/knox.go b/knox.go index 399a792..620265c 100644 --- a/knox.go +++ b/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:///' format.") ErrACLInvalidServicePrefixURL = fmt.Errorf("Service prefix is invalid URL, must conform to 'spiffe:////' 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. diff --git a/knox_test.go b/knox_test.go index c07aeb6..6cf0e07 100644 --- a/knox_test.go +++ b/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") } } diff --git a/server/routes.go b/server/routes.go index f4d6a97..25c6635 100644 --- a/server/routes.go +++ b/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"] diff --git a/server/routes_test.go b/server/routes_test.go index 95ff488..7dc0747 100644 --- a/server/routes_test.go +++ b/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) }