feat(core): add optional name to kas registry CRUD commands (#429)

opentdf · Nov 20, 2024 · f675d86 · f675d86
1 parent b6fbbd6
commit f675d86
Show file tree

Hide file tree

Showing 5 changed files with 157 additions and 35 deletions.
diff --git a/cmd/kas-registry.go b/cmd/kas-registry.go
@@ -11,11 +11,6 @@ import (
 	"google.golang.org/protobuf/encoding/protojson"
 )
 
-const (
-	keyRemote = "Remote"
-	keyCached = "Cached"
-)
-
 var policy_kasRegistryCmd *cobra.Command
 
 func policy_getKeyAccessRegistry(cmd *cobra.Command, args []string) {
@@ -31,19 +26,20 @@ func policy_getKeyAccessRegistry(cmd *cobra.Command, args []string) {
 		cli.ExitWithError(errMsg, err)
 	}
 
-	keyType := keyCached
 	key := &policy.PublicKey{}
 	key.PublicKey = &policy.PublicKey_Cached{Cached: kas.GetPublicKey().GetCached()}
 	if kas.GetPublicKey().GetRemote() != "" {
-		keyType = keyRemote
 		key.PublicKey = &policy.PublicKey_Remote{Remote: kas.GetPublicKey().GetRemote()}
 	}
 	rows := [][]string{
 		{"Id", kas.GetId()},
 		{"URI", kas.GetUri()},
-		{"PublicKey Type", keyType},
 		{"PublicKey", kas.GetPublicKey().String()},
 	}
+	name := kas.GetName()
+	if name != "" {
+		rows = append(rows, []string{"Name", name})
+	}
 
 	if mdRows := getMetadataRows(kas.GetMetadata()); mdRows != nil {
 		rows = append(rows, mdRows...)
@@ -66,24 +62,22 @@ func policy_listKeyAccessRegistries(cmd *cobra.Command, args []string) {
 	t := cli.NewTable(
 		cli.NewUUIDColumn(),
 		table.NewFlexColumn("uri", "URI", cli.FlexColumnWidthFour),
-		table.NewFlexColumn("pk_loc", "PublicKey Location", cli.FlexColumnWidthThree),
-		table.NewFlexColumn("pk", "PublicKey", cli.FlexColumnWidthThree),
+		table.NewFlexColumn("name", "Name", cli.FlexColumnWidthThree),
+		table.NewFlexColumn("pk", "PublicKey", cli.FlexColumnWidthFour),
 	)
 	rows := []table.Row{}
 	for _, kas := range list {
-		keyType := keyCached
 		key := policy.PublicKey{}
 		key.PublicKey = &policy.PublicKey_Cached{Cached: kas.GetPublicKey().GetCached()}
 		if kas.GetPublicKey().GetRemote() != "" {
-			keyType = keyRemote
 			key.PublicKey = &policy.PublicKey_Remote{Remote: kas.GetPublicKey().GetRemote()}
 		}
 
 		rows = append(rows, table.NewRow(table.RowData{
-			"id":     kas.GetId(),
-			"uri":    kas.GetUri(),
-			"pk_loc": keyType,
-			"pk":     kas.GetPublicKey().String(),
+			"id":   kas.GetId(),
+			"uri":  kas.GetUri(),
+			"name": kas.GetName(),
+			"pk":   kas.GetPublicKey().String(),
 		}))
 	}
 	t = t.WithRows(rows)
@@ -98,6 +92,7 @@ func policy_createKeyAccessRegistry(cmd *cobra.Command, args []string) {
 	uri := c.Flags.GetRequiredString("uri")
 	cachedJSON := c.Flags.GetOptionalString("public-keys")
 	remote := c.Flags.GetOptionalString("public-key-remote")
+	name := c.Flags.GetOptionalString("name")
 	metadataLabels = c.Flags.GetStringSlice("label", metadataLabels, cli.FlagsStringSliceOptions{Min: 0})
 
 	if cachedJSON == "" && remote == "" {
@@ -106,7 +101,6 @@ func policy_createKeyAccessRegistry(cmd *cobra.Command, args []string) {
 	}
 
 	key := &policy.PublicKey{}
-	keyType := keyCached
 	if cachedJSON != "" {
 		if remote != "" {
 			e := fmt.Errorf("only one public key is allowed. Please pass either a cached or remote public key but not both")
@@ -119,29 +113,26 @@ func policy_createKeyAccessRegistry(cmd *cobra.Command, args []string) {
 		}
 		key = cached
 	} else {
-		keyType = keyRemote
 		key.PublicKey = &policy.PublicKey_Remote{Remote: remote}
 	}
 
 	created, err := h.CreateKasRegistryEntry(
 		uri,
 		key,
+		name,
 		getMetadataMutable(metadataLabels),
 	)
 	if err != nil {
 		cli.ExitWithError("Failed to create Registered KAS entry", err)
 	}
 
-	keyJSON, err := protojson.Marshal(key)
-	if err != nil {
-		cli.ExitWithError("Failed to marshal public key to JSON", err)
-	}
-
 	rows := [][]string{
 		{"Id", created.GetId()},
 		{"URI", created.GetUri()},
-		{"PublicKey Type", keyType},
-		{"PublicKey", string(keyJSON)},
+		{"PublicKey", created.GetPublicKey().String()},
+	}
+	if name != "" {
+		rows = append(rows, []string{"Name", name})
 	}
 	if mdRows := getMetadataRows(created.GetMetadata()); mdRows != nil {
 		rows = append(rows, mdRows...)
@@ -158,19 +149,21 @@ func policy_updateKeyAccessRegistry(cmd *cobra.Command, args []string) {
 
 	id := c.Flags.GetRequiredID("id")
 	uri := c.Flags.GetOptionalString("uri")
+	name := c.Flags.GetOptionalString("name")
 	cachedJSON := c.Flags.GetOptionalString("public-keys")
 	remote := c.Flags.GetOptionalString("public-key-remote")
 	metadataLabels = c.Flags.GetStringSlice("label", metadataLabels, cli.FlagsStringSliceOptions{Min: 0})
 
-	if cachedJSON == "" && remote == "" && len(metadataLabels) == 0 && uri == "" {
-		cli.ExitWithError("No values were passed to update. Please pass at least one value to update (E.G. 'uri', 'public-keys', 'public-key-remote', 'label')", nil)
+	allEmpty := cachedJSON == "" && remote == "" && len(metadataLabels) == 0 && uri == "" && name == ""
+	if allEmpty {
+		cli.ExitWithError("No values were passed to update. Please pass at least one value to update (E.G. 'uri', 'name', 'public-keys', 'public-key-remote', 'label')", nil)
 	}
 
 	var pubKey *policy.PublicKey
 	//nolint:gocritic // this is more readable than a switch statement
 	if cachedJSON != "" && remote != "" {
 		e := fmt.Errorf("only one public key is allowed. Please pass either a cached or remote public key but not both")
-		cli.ExitWithError("Issue with update flags 'public-keys' and 'public-key-remote': ", e)
+		cli.ExitWithError("Issue with update flags 'public-keys' and 'public-key-remote'", e)
 	} else if cachedJSON != "" {
 		cached := new(policy.PublicKey)
 		err := protojson.Unmarshal([]byte(cachedJSON), cached)
@@ -185,6 +178,7 @@ func policy_updateKeyAccessRegistry(cmd *cobra.Command, args []string) {
 	updated, err := h.UpdateKasRegistryEntry(
 		id,
 		uri,
+		name,
 		pubKey,
 		getMetadataMutable(metadataLabels),
 		getMetadataUpdateBehavior(),
@@ -195,7 +189,12 @@ func policy_updateKeyAccessRegistry(cmd *cobra.Command, args []string) {
 	rows := [][]string{
 		{"Id", id},
 		{"URI", updated.GetUri()},
+		{"PublicKey", updated.GetPublicKey().String()},
 	}
+	if updated.GetName() != "" {
+		rows = append(rows, []string{"Name", updated.GetName()})
+	}
+
 	if mdRows := getMetadataRows(updated.GetMetadata()); mdRows != nil {
 		rows = append(rows, mdRows...)
 	}
@@ -248,7 +247,6 @@ func init() {
 	listDoc := man.Docs.GetCommand("policy/kas-registry/list",
 		man.WithRun(policy_listKeyAccessRegistries),
 	)
-	// TODO: active, inactive, any state querying [https://github.com/opentdf/otdfctl/issues/68]
 
 	createDoc := man.Docs.GetCommand("policy/kas-registry/create",
 		man.WithRun(policy_createKeyAccessRegistry),
@@ -271,6 +269,12 @@ func init() {
 		createDoc.GetDocFlag("public-key-remote").Default,
 		createDoc.GetDocFlag("public-key-remote").Description,
 	)
+	createDoc.Flags().StringP(
+		createDoc.GetDocFlag("name").Name,
+		createDoc.GetDocFlag("name").Shorthand,
+		createDoc.GetDocFlag("name").Default,
+		createDoc.GetDocFlag("name").Description,
+	)
 	injectLabelFlags(&createDoc.Command, false)
 
 	updateDoc := man.Docs.GetCommand("policy/kas-registry/update",
@@ -300,6 +304,12 @@ func init() {
 		updateDoc.GetDocFlag("public-key-remote").Default,
 		updateDoc.GetDocFlag("public-key-remote").Description,
 	)
+	updateDoc.Flags().StringP(
+		updateDoc.GetDocFlag("name").Name,
+		updateDoc.GetDocFlag("name").Shorthand,
+		updateDoc.GetDocFlag("name").Default,
+		updateDoc.GetDocFlag("name").Description,
+	)
 	injectLabelFlags(&updateDoc.Command, true)
 
 	deleteDoc := man.Docs.GetCommand("policy/kas-registry/delete",

diff --git a/docs/man/policy/kas-registry/create.md b/docs/man/policy/kas-registry/create.md
@@ -17,6 +17,10 @@ command:
     - name: public-key-remote
       shorthand: r
       description: Remote URI where the public key can be retrieved for the KAS
+    - name: label
+    - name: name
+      shorthand: n
+      description: Optional name of the registered KAS (must be unique within policy)
     - name: label
       description: "Optional metadata 'labels' in the format: key=value"
       shorthand: l

diff --git a/docs/man/policy/kas-registry/update.md b/docs/man/policy/kas-registry/update.md
@@ -18,6 +18,9 @@ command:
     - name: public-key-remote
       shorthand: r
       description: URI of the public key of the Key Access Server
+    - name: name
+      shorthand: n
+      description: Optional name of the registered KAS (must be unique within policy)
     - name: label
       description: "Optional metadata 'labels' in the format: key=value"
       shorthand: l

diff --git a/e2e/kas-registry.bats b/e2e/kas-registry.bats
@@ -32,12 +32,70 @@ teardown() {
 
 @test "create registration of a KAS with remote key" {
     URI="https://testing-create-remote.co"
-    run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY" --json
+    NAME="my_kas_name"
+    run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY" -n "$NAME" --json
         assert_output --partial "$REMOTE_KEY"
         assert_output --partial "$URI"
+        assert_output --partial "$NAME"
     export CREATED="$output"
 }
 
+@test "create KAS registration with invalid URI - fails" {
+    BAD_URIS=(
+        "no-scheme.co"
+        "localhost"
+        "http://example.com:abc"
+        "https ://example.com"
+    )
+
+    for URI in "${BAD_URIS[@]}"; do
+        run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY"
+            assert_failure
+            assert_output --partial "Failed to create Registered KAS"
+            assert_output --partial "uri: "
+    done
+}
+
+@test "create KAS registration with duplicate URI - fails" {
+    URI="https://testing-duplication.io"
+    run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY"
+        assert_success
+    run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY"
+        assert_failure
+        assert_output --partial "Failed to create Registered KAS entry"
+        assert_output --partial "AlreadyExists"
+}
+
+@test "create KAS registration with duplicate name - fails" {
+    NAME="duplicate_name_kas"
+    run_otdfctl_kasr create --uri "https://testing-duplication.name.io" -r "$REMOTE_KEY" -n "$NAME"
+        assert_success
+    run_otdfctl_kasr create --uri "https://testing-duplication.name.net" -r "$REMOTE_KEY" -n "$NAME"
+        assert_failure
+        assert_output --partial "Failed to create Registered KAS entry"
+        assert_output --partial "AlreadyExists"
+}
+
+@test "create KAS registration with invalid name - fails" {
+    URI="http://creating.kas.invalid.name/kas"
+    BAD_NAMES=(
+        "-bad-name"
+        "bad-name-"
+        "_bad_name"
+        "bad_name_"
+        "name@with!special#chars"
+        "$(printf 'a%.0s' {1..254})" # Generates a string of 254 'a' characters
+    )
+
+    for NAME in "${BAD_NAMES[@]}"; do
+        echo "testing $NAME"
+        run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY" -n "$NAME"
+            assert_failure
+            assert_output --partial "Failed to create Registered KAS"
+            assert_output --partial "name: "
+    done
+}
+
 @test "create KAS with cached key then get it" {
     URI="https://testing-get.gov"
     export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "$URI" -c "$CACHED_KEY" --json)
@@ -60,23 +118,68 @@ teardown() {
 
 @test "update registered KAS" {
     URI="https://testing-update.net"
-    export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "$URI" -c "$CACHED_KEY" --json)
+    NAME="new-kas-testing-update"
+    export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "$URI" -c "$CACHED_KEY" -n "$NAME" --json)
     ID=$(echo "$CREATED" | jq -r '.id')
-    run_otdfctl_kasr update --id "$ID" -u "https://newuri.com" --public-key-remote "$REMOTE_KEY" --json
+    run_otdfctl_kasr update --id "$ID" -u "https://newuri.com" -n "newer-name" --public-key-remote "$REMOTE_KEY" --json
         assert_output --partial "$ID"
         assert_output --partial "https://newuri.com"
         assert_output --partial "$REMOTE_KEY"
+        assert_output --partial "newer-name"
         assert_output --partial "uri"
         refute_output --partial "pem"
+        refute_output --partial "$NAME"
         refute_output --partial "cached"
 }
 
+@test "update registered KAS with invalid URI - fails" {
+    export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "https://bad-update.uri.kas" -c "$CACHED_KEY" --json)
+    ID=$(echo "$CREATED" | jq -r '.id')
+    BAD_URIS=(
+        "no-scheme.co"
+        "localhost"
+        "http://example.com:abc"
+        "https ://example.com"
+    )
+
+    for URI in "${BAD_URIS[@]}"; do
+        run_otdfctl_kasr update -i "$ID" --uri "$URI"
+            assert_failure
+            assert_output --partial "$ID"
+            assert_output --partial "Failed to update Registered KAS entry"
+            assert_output --partial "uri: "
+    done
+}
+
+@test "update registered KAS with invalid name - fails" {
+    export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "https://bad-update.name.kas" -r "$REMOTE_KEY" --json)
+    ID=$(echo "$CREATED" | jq -r '.id')
+    BAD_NAMES=(
+        "-bad-name"
+        "bad-name-"
+        "_bad_name"
+        "bad_name_"
+        "name@with!special#chars"
+        "$(printf 'a%.0s' {1..254})" # Generates a string of 254 'a' characters
+    )
+
+    for NAME in "${BAD_NAMES[@]}"; do
+        run_otdfctl_kasr update --name "$NAME" -r "$REMOTE_KEY" --id "$ID"
+            assert_failure
+            assert_output --partial "Failed to update Registered KAS"
+            assert_output --partial "name: "
+    done
+}
+
 @test "list registered KASes" {
     URI="https://testing-list.io"
-    export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "$URI" -c "$CACHED_KEY" --json)
+    NAME="listed-kas"
+    export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "$URI" -c "$CACHED_KEY" -n "$NAME" --json)
     ID=$(echo "$CREATED" | jq -r '.id')
     run_otdfctl_kasr list --json
     assert_output --partial "$ID"
     assert_output --partial "uri"
     assert_output --partial "$URI"
+    assert_output --partial "name"
+    assert_output --partial "$NAME"
 }
diff --git a/pkg/handlers/kas-registry.go b/pkg/handlers/kas-registry.go
@@ -27,10 +27,11 @@ func (h Handler) ListKasRegistryEntries() ([]*policy.KeyAccessServer, error) {
 }
 
 // Creates the KAS registry and then returns the KAS
-func (h Handler) CreateKasRegistryEntry(uri string, publicKey *policy.PublicKey, metadata *common.MetadataMutable) (*policy.KeyAccessServer, error) {
+func (h Handler) CreateKasRegistryEntry(uri string, publicKey *policy.PublicKey, name string, metadata *common.MetadataMutable) (*policy.KeyAccessServer, error) {
 	req := &kasregistry.CreateKeyAccessServerRequest{
 		Uri:       uri,
 		PublicKey: publicKey,
+		Name:      name,
 		Metadata:  metadata,
 	}
 
@@ -43,10 +44,11 @@ func (h Handler) CreateKasRegistryEntry(uri string, publicKey *policy.PublicKey,
 }
 
 // Updates the KAS registry and then returns the KAS
-func (h Handler) UpdateKasRegistryEntry(id string, uri string, publickey *policy.PublicKey, metadata *common.MetadataMutable, behavior common.MetadataUpdateEnum) (*policy.KeyAccessServer, error) {
+func (h Handler) UpdateKasRegistryEntry(id, uri, name string, publickey *policy.PublicKey, metadata *common.MetadataMutable, behavior common.MetadataUpdateEnum) (*policy.KeyAccessServer, error) {
 	_, err := h.sdk.KeyAccessServerRegistry.UpdateKeyAccessServer(h.ctx, &kasregistry.UpdateKeyAccessServerRequest{
 		Id:                     id,
 		Uri:                    uri,
+		Name:                   name,
 		PublicKey:              publickey,
 		Metadata:               metadata,
 		MetadataUpdateBehavior: behavior,