🎉 All SPIFFEs to create keys

Also add the ability to specify ACL on creation

client/create.go

@@ -1,6 +1,7 @@
 package client
 
 import (
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -13,7 +14,7 @@ func init() {
 }
 
 var cmdCreate = &Command{
-	UsageLine: "create [--key-template template_name] <key_identifier>",
+	UsageLine: "create [--acl key_acl] [--key-template template_name] <key_identifier>",
 	Short:     "creates a new key",
 	Long: `
 Create will create a new key in knox with input as the primary key version. Key data should be sent to stdin unless a key-template is specified.
@@ -26,14 +27,43 @@ Please run "knox create --key-template <template_name> <key_identifier>".
 
 The original key version id will be print to stdout.
 
-To create a new key, user credentials are required. The default access list will include the creator of this key and 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 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.
 
 For more about knox, see https://github.com/pinterest/knox.
 
 See also: knox add, knox get
 	`,
 }
 var createTinkKeyset = cmdCreate.Flag.String("key-template", "", "name of a knox-supported Tink key template")
+var createAcl = cmdCreate.Flag.String("acl", "", "ACL for the created key")
+
+func parseAcl(aclString string) (knox.ACL, error) {
+	var err error
+	var accessList []knox.Access
+
+	if aclString == "" {
+		return knox.ACL{}, nil
+	}
+
+	err = json.Unmarshal([]byte(aclString), &accessList)
+	if err != nil {
+		return nil, err
+	}
+
+	acl := knox.ACL(accessList)
+
+	err = acl.Validate()
+	if err != nil {
+		return nil, err
+	}
+	err = acl.ValidateHasHumanAdmin()
+	if err != nil {
+		return nil, err
+	}
+
+	return acl, nil
+}
 
 func runCreate(cmd *Command, args []string) *ErrorStatus {
 	if len(args) != 1 {
@@ -55,8 +85,13 @@ func runCreate(cmd *Command, args []string) *ErrorStatus {
 	if err != nil {
 		return &ErrorStatus{err, false}
 	}
-	// TODO(devinlundberg): allow ACL to be entered as input
-	acl := knox.ACL{}
+
+	var acl knox.ACL
+	acl, err = parseAcl(*createAcl)
+	if err != nil {
+		return &ErrorStatus{fmt.Errorf("Error parsing ACL: %s", err.Error()), false}
+	}
+
 	versionID, err := cli.CreateKey(keyID, data, acl)
 	if err != nil {
 		return &ErrorStatus{fmt.Errorf("Error adding version: %s", err.Error()), true}

client/create_test.go

@@ -0,0 +1,71 @@
+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
+}
+
+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}
+
+	validAclNoHumanAdmin := knox.ACL([]knox.Access{machineAdmin, userWrite})
+	validAclWithHumanAdmin := knox.ACL([]knox.Access{machineAdmin, userAdmin})
+	blankAcl := knox.ACL{}
+
+	testCases := []struct {
+		str string
+		acl knox.ACL
+		errMsg string
+	}{
+		{
+			`[{"type":"foo","id":"bar","access":"test"}]`,
+			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
+	    },
+	    {
+			`[{"type":"Machine","id":"testmachine1","access":"Admin"}, {"type":"User","id":"testuser","access":"Admin"}]`,
+			validAclWithHumanAdmin,
+			"", // 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)
+			}
+		} else{
+			if !testAclEq(acl, tc.acl) {
+				t.Fatalf("%v should equal %v", acl, tc.acl)
+			}
+		}
+	}
+}

client_test.go

@@ -11,6 +11,10 @@ import (
 	"testing"
 )
 
+var DataBytes = []byte("data")
+// I.e. b64encode("data")
+const DataB64Encoded = "ZGF0YQ=="
+
 func TestMockClient(t *testing.T) {
 	p := "primary"
 	a := []string{"active1", "active2"}
@@ -181,8 +185,8 @@ func TestCreateKey(t *testing.T) {
 			t.Fatalf("%s is not %s", r.URL.Path, "/v0/keys/")
 		}
 		r.ParseForm()
-		if r.PostForm["data"][0] != "ZGF0YQ==" {
-			t.Fatalf("%s is not expected: %s", r.PostForm["data"][0], "ZGF0YQ==")
+		if r.PostForm["data"][0] != DataB64Encoded {
+			t.Fatalf("%s is not expected: %s", r.PostForm["data"][0], DataB64Encoded)
 		}
 		if r.PostForm["id"][0] != "testkey" {
 			t.Fatalf("%s is not expected: %s", r.PostForm["id"][0], "testkey")
@@ -195,27 +199,55 @@ func TestCreateKey(t *testing.T) {
 
 	cli := MockClient(srv.Listener.Addr().String(), "")
 
-	acl := ACL([]Access{
+	aclWithUserAdmin := ACL([]Access{
+		{
+			Type:       User,
+			AccessType: Admin,
+			ID:         "test",
+		},
+	})
+	aclWithUserRead := ACL([]Access{
 		{
 			Type:       User,
 			AccessType: Read,
 			ID:         "test",
 		},
 	})
 
-	badACL := ACL([]Access{
+	invalidTypeACL := ACL([]Access{
 		{
-			Type:       233,
-			AccessType: 80927,
+			Type:       233,  // Not a principal
+			AccessType: Read,
 			ID:         "test",
 		},
 	})
-	_, err = cli.CreateKey("testkey", []byte("data"), badACL)
-	if err == nil {
-		t.Fatal("error is nil")
+	_, err = cli.CreateKey("testkey", DataBytes, invalidTypeACL)
+	if err.Error() != "json: error calling MarshalJSON for type knox.PrincipalType: json: Invalid PrincipalType to convert" {
+		t.Fatalf("Expected err is %v", err)
+	}
+
+	invalidAccessTypeACL := ACL([]Access{
+		{
+			Type:       User,
+			AccessType: 80927, // Not an access type
+			ID:         "test",
+		},
+	})
+	_, err = cli.CreateKey("testkey", DataBytes, invalidAccessTypeACL)
+	if err.Error() != "json: error calling MarshalJSON for type knox.AccessType: json: Invalid AccessType to convert" {
+		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`
+	k, err := cli.CreateKey("testkey", DataBytes, aclWithUserRead)
+	if err != nil {
+		t.Fatalf("%s is not nil", err)
+	}
+	if k != expected {
+		t.Fatalf("%d is not %d", k, expected)
 	}
 
-	k, err := cli.CreateKey("testkey", []byte("data"), acl)
+	k, err = cli.CreateKey("testkey", DataBytes, aclWithUserAdmin)
 	if err != nil {
 		t.Fatalf("%s is not nil", err)
 	}
@@ -238,15 +270,15 @@ func TestAddVersion(t *testing.T) {
 			t.Fatalf("%s is not %s", r.URL.Path, "/v0/keys/testkey/versions/")
 		}
 		r.ParseForm()
-		if r.PostForm["data"][0] != "ZGF0YQ==" {
-			t.Fatalf("%s is not expected: %s", r.PostForm["data"][0], "ZGF0YQ==")
+		if r.PostForm["data"][0] != DataB64Encoded {
+			t.Fatalf("%s is not expected: %s", r.PostForm["data"][0], DataB64Encoded)
 		}
 	})
 	defer srv.Close()
 
 	cli := MockClient(srv.Listener.Addr().String(), "")
 
-	k, err := cli.AddVersion("testkey", []byte("data"))
+	k, err := cli.AddVersion("testkey", DataBytes)
 	if err != nil {
 		t.Fatalf("%s is not nil", err)
 	}

knox.go

@@ -17,6 +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")
 
 	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.")
@@ -43,7 +44,7 @@ const (
 	spiffeScheme = "spiffe"
 )
 
-// InvalidTypeError is an error for to throw when in the json conversion.
+// An invalidTypeError is an error to throw when in the json conversion.
 type invalidTypeError struct {
 	badType string
 }
@@ -320,6 +321,19 @@ 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 {
+	for _, a := range acl {
+		if a.AccessType != Admin {
+			continue;
+		}
+		if a.Type == User || a.Type == UserGroup {
+			return nil
+		}
+	}
+	return ErrACLDoesNotContainHumanAdmin
+}
+
 // Add appends an access to the ACL. It does so by overwriting any existing access
 // that principal or group may have had.
 func (acl ACL) Add(a Access) ACL {
@@ -588,6 +602,8 @@ const (
 	BadRequestDataCode
 	BadKeyFormatCode
 	BadPrincipalIdentifier
+	BadAclCode
+	NoHumanAdminInAclCode
 )
 
 // Response is the format for responses from the api server.

knox_test.go

@@ -203,29 +203,62 @@ func TestKeyPathMarhaling(t *testing.T) {
 }
 
 func TestACLValidate(t *testing.T) {
-	a1 := Access{ID: "testmachine1", AccessType: Admin, Type: Machine}
-	a2 := Access{ID: "testuser", AccessType: Write, Type: User}
-	a3 := Access{ID: "testmachine", AccessType: Read, Type: MachinePrefix}
-	a6 := Access{ID: "spiffe://example.com/serviceA", AccessType: Read, Type: Service}
-	a7 := Access{ID: "spiffe://example.com/serviceA/", AccessType: Read, Type: ServicePrefix}
-	validACL := ACL([]Access{a1, a2, a3, a6, a7})
+	var accessEntries []Access
+
+	machineAdmin := Access{ID: "testmachine1", AccessType: Admin, Type: Machine}
+	userWrite := Access{ID: "testuser", 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}
+	validACL := ACL(accessEntries)
 	if validACL.Validate() != nil {
-		t.Error("ValidACL should be valid")
+		t.Error("validACL should be valid")
 	}
 
-	a4 := Access{ID: "testmachine", AccessType: None, Type: MachinePrefix}
-	noneACL := ACL([]Access{a1, a2, a4})
-	if noneACL.Validate() == nil {
-		t.Error("noneACL should err")
+	machinePrefixNone := Access{ID: "unique", AccessType: None, Type: MachinePrefix}
+	accessEntriesPlusNoneACL := ACL(append(accessEntries, machinePrefixNone))
+	if accessEntriesPlusNoneACL.Validate() != ErrACLContainsNone {
+		t.Error("accessEntriesPlusNoneACL should err")
 	}
 
-	a5 := Access{ID: "testmachine1", AccessType: Write, Type: Machine}
-	dupACL := ACL([]Access{a1, a5, a3})
-	if dupACL.Validate() == nil {
+	machineWrite := Access{ID: "testmachine1", AccessType: Write, Type: Machine}
+	// machineAdmin (inside accessEntries) and machineWrite have the same ID and Type
+	dupACL := ACL(append(accessEntries, machineWrite))
+	if dupACL.Validate() != ErrACLDuplicateEntries {
 		t.Error("dupACL should err")
 	}
 }
 
+func TestACLValidateHasHumanAdmin(t *testing.T) {
+	var accessEntries []Access
+
+	machineAdmin := Access{ID: "testmachine1", AccessType: Admin, Type: Machine}
+	userWrite := Access{ID: "testuser", 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}
+	noHumanAdmin := ACL(accessEntries)
+	if noHumanAdmin.ValidateHasHumanAdmin() != ErrACLDoesNotContainHumanAdmin {
+		t.Error("ValidACL should not be valid")
+	}
+
+	userAdmin := Access{ID: "testuser", AccessType: Admin, Type: User}
+	validWithUserAdmin := ACL(append(noHumanAdmin, userAdmin))
+	if validWithUserAdmin.ValidateHasHumanAdmin() != nil {
+		t.Error("ValidACL should be valid")
+	}
+
+	userGroupAdmin := Access{ID: "testuser", AccessType: Admin, Type: UserGroup}
+	validWithGroupAdmin := ACL(append(noHumanAdmin, userGroupAdmin))
+	if validWithGroupAdmin.ValidateHasHumanAdmin() != nil {
+		t.Error("ValidACL should be valid")
+	}
+}
+
 func TestACLAddMultiple(t *testing.T) {
 	a1 := Access{ID: "testmachine", AccessType: Admin, Type: Machine}
 	a3 := Access{ID: "testmachine", AccessType: None, Type: Machine}