fix(core): kas registry get should allow -i 'id' flag shorthand (#434)

Co-authored-by: Ryan Schumacher <jschumacher@virtru.com>

cmd/kas-registry.go

@@ -1,6 +1,9 @@
 package cmd
 
 import (
+	"encoding/json"
+	"encoding/pem"
+	"errors"
 	"fmt"
 
 	"github.com/evertras/bubble-table/table"
@@ -96,22 +99,18 @@ func policy_createKeyAccessRegistry(cmd *cobra.Command, args []string) {
 	metadataLabels = c.Flags.GetStringSlice("label", metadataLabels, cli.FlagsStringSliceOptions{Min: 0})
 
 	if cachedJSON == "" && remote == "" {
-		e := fmt.Errorf("a public key is required. Please pass either a cached or remote public key")
-		cli.ExitWithError("Issue with create flags 'public-keys' and 'public-key-remote'", e)
+		cli.ExitWithError("Empty flags 'public-keys' and 'public-key-remote'", errors.New("error: a public key is required"))
 	}
 
-	key := &policy.PublicKey{}
+	key := new(policy.PublicKey)
 	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")
-			cli.ExitWithError("Issue with create flags 'public-keys' and 'public-key-remote'", e)
+			cli.ExitWithError("Found values for both flags 'public-keys' and 'public-key-remote'", errors.New("error: only one public key is allowed"))
 		}
-		cached := new(policy.PublicKey)
-		err := protojson.Unmarshal([]byte(cachedJSON), cached)
+		err := unmarshalKASPublicKey(cachedJSON, key)
 		if err != nil {
-			cli.ExitWithError("Failed to unmarshal cached public key JSON", err)
+			cli.ExitWithError(fmt.Sprintf("KAS registry key is invalid: '%s', see help for examples", cachedJSON), err)
 		}
-		key = cached
 	} else {
 		key.PublicKey = &policy.PublicKey_Remote{Remote: remote}
 	}
@@ -159,20 +158,18 @@ func policy_updateKeyAccessRegistry(cmd *cobra.Command, args []string) {
 		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
+	pubKey := new(policy.PublicKey)
 	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)
-	} else if cachedJSON != "" {
-		cached := new(policy.PublicKey)
-		err := protojson.Unmarshal([]byte(cachedJSON), cached)
+		cli.ExitWithError("Issue with update flags 'public-keys' and 'public-key-remote': ", e)
+	}
+	if cachedJSON != "" {
+		err := unmarshalKASPublicKey(cachedJSON, pubKey)
 		if err != nil {
-			cli.ExitWithError("Failed to unmarshal cached public key JSON", err)
+			cli.ExitWithError(fmt.Sprintf("KAS registry key is invalid: '%s', see help for examples", cachedJSON), err)
 		}
-		pubKey = cached
 	} else if remote != "" {
-		pubKey = &policy.PublicKey{PublicKey: &policy.PublicKey_Remote{Remote: remote}}
+		pubKey.PublicKey = &policy.PublicKey_Remote{Remote: remote}
 	}
 
 	updated, err := h.UpdateKasRegistryEntry(
@@ -231,6 +228,32 @@ func policy_deleteKeyAccessRegistry(cmd *cobra.Command, args []string) {
 	HandleSuccess(cmd, kas.GetId(), t, kas)
 }
 
+// TODO: remove this when the data is structured
+func unmarshalKASPublicKey(keyStr string, key *policy.PublicKey) error {
+	if !json.Valid([]byte(keyStr)) {
+		return errors.New("invalid JSON")
+	}
+
+	if err := protojson.Unmarshal([]byte(keyStr), key); err != nil {
+		return errors.New("invalid shape")
+	}
+
+	// Validate all PEMs
+	keyErrs := []error{}
+	for i, k := range key.GetCached().GetKeys() {
+		block, _ := pem.Decode([]byte(k.GetPem()))
+		if block == nil {
+			keyErrs = append(keyErrs, fmt.Errorf("error in key[%d] with KID \"%s\": PEM is invalid", i, k.GetKid()))
+		}
+	}
+
+	if len(keyErrs) != 0 {
+		return errors.Join(keyErrs...)
+	}
+
+	return nil
+}
+
 func init() {
 	getDoc := man.Docs.GetCommand("policy/kas-registry/get",
 		man.WithRun(policy_getKeyAccessRegistry),

docs/man/policy/kas-registry/get.md

@@ -6,7 +6,7 @@ command:
     - g
   flags:
     - name: id
-      long: id
+      shorthand: i
       description: ID of the Key Access Server registration
       required: true
 ---

docs/man/policy/kas-registry/update.md

@@ -13,11 +13,11 @@ command:
       shorthand: u
       description: URI of the Key Access Server
     - name: public-keys
-      shorthand: p
-      description: One or more public keys saved for the KAS
+      shorthand: c
+      description: One or more 'cached' public keys saved for the KAS
     - name: public-key-remote
       shorthand: r
-      description: URI of the public key of the Key Access Server
+      description: URI of the 'remote' public key of the Key Access Server
     - name: name
       shorthand: n
       description: Optional name of the registered KAS (must be unique within policy)

e2e/kas-registry.bats

@@ -10,9 +10,9 @@ setup_file() {
   export DEBUG_LEVEL="--log-level debug"
 
   export REMOTE_KEY='https://hello.world/pubkey'
-  export PEM='LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUMvVENDQWVXZ0F3SUJBZ0lVTXU4bzhXaDJIVEE2VEFlTENqQzJmVjlwSWVJd0RRWUpLb1pJaHZjTkFRRUwKQlFBd0RqRU1NQW9HQTFVRUF3d0RhMkZ6TUI0WERUSTBNRFl4T0RFNE16WXlOMW9YRFRJMU1EWXhPREU0TXpZeQpOMW93RGpFTU1Bb0dBMVVFQXd3RGEyRnpNSUlCSWpBTkJna3Foa2lHOXcwQkFRRUZBQU9DQVE4QU1JSUJDZ0tDCkFRRUFyMXBRam83cGlPdlBDVHRkSUVOZkc4eVZpK1dWMUZVTi82eFREcXJMeFpUdEFrWjE0M3VIVGZQOWExdXEKaFcxSW9heUpPVWpuWXNuUUh6dUVCZGtaNEh1d3pkeTZ3Um5lT1RSY2o3TitEd25aS21EcTF1YWZ6bEdzdG8vQgpoZnRtaWxVRjRZbm5GY0ROK3ZxajJlcDNhYlVramhrbUlRVDhwcjI1YkZ4TGFpd3dPbmx5TTVWUWM4bmFoZ2xuCjBNMGdOV0tJV0ZFSndoajBab2poMUw0ZGptenFVaU9tTkhCUDRRelNwNyswK3RXb3hJb1AyT2Fqa0p5MEljWkgKcS9OOWlTelZiZzFLL2tLZytkdS9QbWRqUCtqNTZsa0pPU1J6ZXpoK2R5NytHaHJCVDNVc21QbmNWM2NXVk1pOApFc1lDS2NUNUVNSGhhTmFHMFhEakptRzI4d0lEQVFBQm8xTXdVVEFkQmdOVkhRNEVGZ1FVZ1BUTkZjemQ5ajBFClgzN3A2SGh3UFJpY0JqOHdId1lEVlIwakJCZ3dGb0FVZ1BUTkZjemQ5ajBFWDM3cDZIaHdQUmljQmo4d0R3WUQKVlIwVEFRSC9CQVV3QXdFQi96QU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FRRUFDS2VxRkswSlcyYTVzS2JPQnl3WgppazB5MmpyRHJaUG5mMG9kTjVIbThtZWVuQnhteW9CeVZWRm9uUGVDaG1uWUZTdERtMlFJUTZnWVBtdEFhQ3VKCnRVeU5zNkxPQm1wR2JKaFRnNXljZXFXWnhYY3NmVkZ3ZHFxVXQ2NnRXdmNPeFZUQmdrN3h6RFFPbkxnRkxqZDYKSlZIeE16RkxXVFEwa00yVXJOOGd0T2RMazRhZUJhSzdiVHdaUEZ0RnR1YUZlYlFUbTRLY2ZSNXpzZkxTKzhpRgp1MWZGOVpKWkg2ZzZibENUeE50d3Z2eVMxVTMvS1AwVlQ5WVB3OTVmcElWMlNLT2QzejNZMGRKOUE5TGQ5TUkzCkwvWS8rNW05NEZCMTdTSWtERXpZM2d2TkxDSVZxODh2WHlnK2doVEhzSHNjYzNWcUUwK0x6cmZkemltbzMxRWQKTkE9PQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0t'
+  PEM='-----BEGIN CERTIFICATE-----\nMIIC/TCCAeWgAwIBAgIUMu8o8Wh2HTA6TAeLCjC2f\n9pIeIwDQYJKoZIhvcNAQEL\nBQAwDjEMMAoGA1UEAwwDa2FzMB4XDTI0MDYxODE4M\nYyN1oXDTI1MDYxODE4MzYy\nN1owDjEMMAoGA1UEAwwDa2FzMIIBIjANBgkqhkiG9\n0BAQEFAAOCAQ8AMIIBCgKC\nAQEAr1pQjo7piOvPCTtdIENfG8yVi+WV1FUN/6xTD\nrLxZTtAkZ143uHTfP9a1uq\nhW1IoayJOUjnYsnQHzuEBdkZ4Huwzdy6wRneOTRcj\nN+DwnZKmDq1uafzlGsto/B\nhftmilUF4YnnFcDN+vqj2ep3abUkjhkmIQT8pr25b\nxLaiwwOnlyM5VQc8nahgln\n0M0gNWKIWFEJwhj0Zojh1L4djmzqUiOmNHBP4QzSp\n+0+tWoxIoP2OajkJy0IcZH\nq/N9iSzVbg1K/kKg+du/PmdjP+j56lkJOSRzezh+d\n7+GhrBT3UsmPncV3cWVMi8\nEsYCKcT5EMHhaNaG0XDjJmG28wIDAQABo1MwUTAdB\nNVHQ4EFgQUgPTNFczd9j0E\nX37p6HhwPRicBj8wHwYDVR0jBBgwFoAUgPTNFczd9\n0EX37p6HhwPRicBj8wDwYD\nVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCA\nEACKeqFK0JW2a5sKbOBywZ\nik0y2jrDrZPnf0odN5Hm8meenBxmyoByVVFonPeCh\nnYFStDm2QIQ6gYPmtAaCuJ\ntUyNs6LOBmpGbJhTg5yceqWZxXcsfVFwdqqUt66tW\ncOxVTBgk7xzDQOnLgFLjd6\nJVHxMzFLWTQ0kM2UrN8gtOdLk4aeBaK7bTwZPFtFt\naFebQTm4KcfR5zsfLS+8iF\nu1fF9ZJZH6g6blCTxNtwvvyS1U3/KP0VT9YPw95fp\nV2SKOd3z3Y0dJ9A9Ld9MI3\nL/Y/+5m94FB17SIkDEzY3gvNLCIVq88vXyg+ghTHs\nscc3VqE0+Lzrfdzimo31Ed\nNA==\n-----END CERTIFICATE-----'
   export KID='my_key_123'
-  export CACHED_KEY="{\"cached\":{\"keys\":[{\"pem\":\"$PEM\",\"kid\":\"$KID\",\"alg\":1}]}}"
+  export CACHED_KEY=$(printf '{"cached":{"keys":[{"kid":"%s","alg":1,"pem":"%s"}]}}' "$KID" "$PEM" )
 }
 
 setup() {
@@ -40,6 +40,21 @@ teardown() {
     export CREATED="$output"
 }
 
+@test "create KAS registration with invalid key - fails" {
+    BAD_CACHED=(
+        '{"cached":{"keys":[{"pem":"bad"}]}}'
+        '{"cached":[]}'
+        '{"cached":{"keys":[{]}}'
+    )
+
+    for BAD_KEY in "${BAD_CACHED[@]}"; do
+        URI='https://bad.pem/kas'
+        run_otdfctl_kasr create --uri "$URI" --public-keys "$BAD_KEY"
+            assert_failure
+            assert_output --partial "KAS registry key is invalid"
+    done
+}
+
 @test "create KAS registration with invalid URI - fails" {
     BAD_URIS=(
         "no-scheme.co"
@@ -60,6 +75,7 @@ teardown() {
     URI="https://testing-duplication.io"
     run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY"
         assert_success
+    export CREATED="$output"
     run_otdfctl_kasr create --uri "$URI" -r "$REMOTE_KEY"
         assert_failure
         assert_output --partial "Failed to create Registered KAS entry"
@@ -107,7 +123,7 @@ teardown() {
         assert_output --partial "$PEM"
         assert_output --partial "$KID"
 
-    run_otdfctl_kasr get --id "$ID" --json
+    run_otdfctl_kasr get -i "$ID" --json
         assert_output --partial "$ID"
         assert_output --partial "$URI"
         assert_output --partial "uri"
@@ -119,17 +135,19 @@ teardown() {
 @test "update registered KAS" {
     URI="https://testing-update.net"
     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)
+    export CREATED=$(./otdfctl $HOST $DEBUG_LEVEL $WITH_CREDS policy kas-registry create --uri "$URI" -r "$REMOTE_KEY" -n "$NAME" --json)
     ID=$(echo "$CREATED" | jq -r '.id')
-    run_otdfctl_kasr update --id "$ID" -u "https://newuri.com" -n "newer-name" --public-key-remote "$REMOTE_KEY" --json
+    run_otdfctl_kasr update --id "$ID" -u "https://newuri.com" -n "newer-name" -c '"$CACHED_KEY"' --json
         assert_output --partial "$ID"
         assert_output --partial "https://newuri.com"
-        assert_output --partial "$REMOTE_KEY"
+        assert_output --partial "kid"
+        assert_output --partial "pem"
+        assert_output --partial "alg"
         assert_output --partial "newer-name"
-        assert_output --partial "uri"
-        refute_output --partial "pem"
         refute_output --partial "$NAME"
-        refute_output --partial "cached"
+        refute_output --partial "$URI"
+        refute_output --partial "remote"
+        refute_output --partial "$REMOTE_KEY"
 }
 
 @test "update registered KAS with invalid URI - fails" {
@@ -143,7 +161,7 @@ teardown() {
     )
 
     for URI in "${BAD_URIS[@]}"; do
-        run_otdfctl_kasr update -i "$ID" --uri "$URI"
+        run_otdfctl_kasr update -i "$ID" -r "$REMOTE_KEY" --uri "$URI"
             assert_failure
             assert_output --partial "$ID"
             assert_output --partial "Failed to update Registered KAS entry"
@@ -164,7 +182,7 @@ teardown() {
     )
 
     for NAME in "${BAD_NAMES[@]}"; do
-        run_otdfctl_kasr update --name "$NAME" -r "$REMOTE_KEY" --id "$ID"
+        run_otdfctl_kasr update -c '"$CACHED_KEY"' --id "$ID" --name "$NAME"
             assert_failure
             assert_output --partial "Failed to update Registered KAS"
             assert_output --partial "name: "

go.mod

@@ -16,7 +16,7 @@ require (
 	github.com/golang-jwt/jwt v3.2.2+incompatible
 	github.com/google/uuid v1.6.0
 	github.com/opentdf/platform/lib/flattening v0.1.1
-	github.com/opentdf/platform/protocol/go v0.2.20
+	github.com/opentdf/platform/protocol/go v0.2.22
 	github.com/opentdf/platform/sdk v0.3.19
 	github.com/spf13/cobra v1.8.1
 	github.com/spf13/viper v1.19.0

go.sum

@@ -220,8 +220,8 @@ github.com/opentdf/platform/lib/flattening v0.1.1 h1:la1f6PcRsc+yLH8+9UEr0ux6IRK
 github.com/opentdf/platform/lib/flattening v0.1.1/go.mod h1:eyG7pe5UZlV+GI5/CymQD3xTAJxNhnP9M4QnBzaad1M=
 github.com/opentdf/platform/lib/ocrypto v0.1.6 h1:rd4ctCZOE/c3qDJORtkSK9tw6dEXb+jbJXRRk4LcxII=
 github.com/opentdf/platform/lib/ocrypto v0.1.6/go.mod h1:ne+l8Q922OdzA0xesK3XJmfECBnn5vLSGYU3/3OhiHM=
-github.com/opentdf/platform/protocol/go v0.2.20 h1:FPU1ZcXvPm/QeE2nqgbD/HMTOCICQSD0DoncQbAZ1ws=
-github.com/opentdf/platform/protocol/go v0.2.20/go.mod h1:TWIuf387VeR3q0TL4nAMKQTWEqqID+8Yjao76EX9Dto=
+github.com/opentdf/platform/protocol/go v0.2.22 h1:C/jjtwu5yTon8g0ewuN29QE7VXSQHyb2dx9W0U6Oqok=
+github.com/opentdf/platform/protocol/go v0.2.22/go.mod h1:skpOCVuWSjUHazLKOkh3nSB057OB4sHICe7MpmJY9KU=
 github.com/opentdf/platform/sdk v0.3.19 h1:4Ign6HPrxOH6ZllLO/cI6joSuqz8CqPlpxpTKunpMQs=
 github.com/opentdf/platform/sdk v0.3.19/go.mod h1:u+XZhVRsMq5blukCFCHcjk6HLCp4Y5mmIQu7GhtKQ3E=
 github.com/opentracing/opentracing-go v1.2.0 h1:uEJPy/1a5RIPAJ0Ov+OIO8OxWu77jEv+1B0VhjKrZUs=

pkg/handlers/kas-registry.go

@@ -44,12 +44,12 @@ func (h Handler) CreateKasRegistryEntry(uri string, publicKey *policy.PublicKey,
 }
 
 // Updates the KAS registry and then returns the KAS
-func (h Handler) UpdateKasRegistryEntry(id, uri, name string, publickey *policy.PublicKey, metadata *common.MetadataMutable, behavior common.MetadataUpdateEnum) (*policy.KeyAccessServer, error) {
+func (h Handler) UpdateKasRegistryEntry(id, uri, name string, pubKey *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,
+		PublicKey:              pubKey,
 		Metadata:               metadata,
 		MetadataUpdateBehavior: behavior,
 	})