remove undocumented requirement for gcpkms key_identifier_values to be 36 characters

doc/plugin_server_keymanager_gcp_kms.md

@@ -70,7 +70,12 @@ that it has previously managed and will recreate new keys on demand.
 If you need more control over the identifier that's used for the server, the
 `key_identifier_value` setting can be used to specify a
 static identifier for the server instance. This setting is appropriate in situations
-where a key identifier file can't be persisted.
+where a key identifier file can't be persisted. The value must contain only
+lowercase letters, numbers, underscores (`_`), and dashes (`-`), and must not be
+longer than 40 characters. The limit leaves room for the `spire-key-` prefix and
+the SPIRE key ID suffix so that the generated Cloud KMS
+[CryptoKey ID](https://cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings.cryptoKeys/create)
+stays within its 63-character limit.
 
 The plugin attempts to detect and prune stale CryptoKeys. To facilitate stale
 CryptoKey detection, the plugin actively updates the `spire-last-update` label

pkg/server/plugin/keymanager/gcpkms/fetcher.go

@@ -46,7 +46,7 @@ func (kf *keyFetcher) fetchKeyEntries(ctx context.Context) ([]*keyEntry, error)
 		if err != nil {
 			return nil, status.Errorf(codes.Internal, "failed to list SPIRE Server keys in Cloud KMS: %v", err)
 		}
-		spireKeyID, ok := getSPIREKeyIDFromCryptoKeyName(cryptoKey.Name)
+		spireKeyID, ok := getSPIREKeyIDFromCryptoKeyName(cryptoKey.Name, kf.serverID)
 		if !ok {
 			kf.log.Warn("Could not get SPIRE Key ID from CryptoKey", cryptoKeyNameTag, cryptoKey.Name)
 			continue
@@ -130,29 +130,27 @@ func (kf *keyFetcher) getKeyEntriesFromCryptoKey(ctx context.Context, cryptoKey
 
 // getSPIREKeyIDFromCryptoKeyName parses a CryptoKey resource name to get the
 // SPIRE Key ID. This Key ID is used in the Server KeyManager interface.
-func getSPIREKeyIDFromCryptoKeyName(cryptoKeyName string) (string, bool) {
+func getSPIREKeyIDFromCryptoKeyName(cryptoKeyName, serverID string) (string, bool) {
 	// cryptoKeyName is the resource name for the CryptoKey holding the SPIRE Key
-	// in the format: projects/*/locations/*/keyRings/*/cryptoKeys/spire-key-*-*.
+	// in the format: projects/*/locations/*/keyRings/*/cryptoKeys/spire-key-<serverID>-<spireKeyID>.
 	// Example: projects/project-name/locations/us-east1/keyRings/key-ring-name/cryptoKeys/spire-key-1f2e225a-91d8-4589-a4fe-f88b7bb04bac-x509-CA-A
+	//
+	// The server ID is not fixed-length (it is a UUID with key_identifier_file
+	// but an arbitrary value with key_identifier_value), so it must be stripped
+	// using the known prefix built by generateCryptoKeyID rather than a fixed
+	// offset.
 
-	// Get the last element of the path.
+	// Get the CryptoKeyID (last element of the path).
 	i := strings.LastIndex(cryptoKeyName, "/")
 	if i < 0 {
-		// All CryptoKeys are under a Key Ring; not a valid Crypto Key name.
 		return "", false
 	}
-
-	// The i index will indicate us where
-	// "spire-key-1f2e225a-91d8-4589-a4fe-f88b7bb04bac-x509-CA-A" starts.
-	// Now we have to get the position where the SPIRE Key ID starts.
-	// For that, we need to add the length of the CryptoKey name prefix that we
-	// are using, the UUID length, and the two "-" separators used in our format.
-	spireKeyIDIndex := i + len(cryptoKeyNamePrefix) + 39 // 39 is the UUID length plus two '-' separators
-	if spireKeyIDIndex >= len(cryptoKeyName) {
-		// The index is out of range.
+	cryptoKeyID := cryptoKeyName[i+1:]
+	prefix := fmt.Sprintf("%s-%s-", cryptoKeyNamePrefix, serverID)
+	spireKeyID, ok := strings.CutPrefix(cryptoKeyID, prefix)
+	if !ok || spireKeyID == "" {
 		return "", false
 	}
-	spireKeyID := cryptoKeyName[spireKeyIDIndex:]
 	return spireKeyID, true
 }
 

pkg/server/plugin/keymanager/gcpkms/gcpkms.go

@@ -50,6 +50,19 @@ const (
 	maxStaleDuration              = time.Hour * 24 * 14 // Two weeks.
 
 	cryptoKeyNamePrefix = "spire-key"
+
+	// maxCryptoKeyIDLength is the maximum length of a Cloud KMS CryptoKey ID,
+	// which must match the regular expression [a-zA-Z0-9_-]{1,63}.
+	// See https://cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings.cryptoKeys/create.
+	maxCryptoKeyIDLength = 63
+
+	// maxSPIREKeyIDLength is the length of the longest SPIRE key ID the server
+	// appends when building a CryptoKey ID (e.g. "JWT-Signer-A", "WIT-Signer-A").
+	// CryptoKey IDs have the form spire-key-<serverID>-<spireKeyID>, so the
+	// server ID must be short enough to leave room for the prefix and this
+	// suffix within maxCryptoKeyIDLength.
+	maxSPIREKeyIDLength = len("JWT-Signer-A")
+
 	labelNameServerID   = "spire-server-id"
 	labelNameLastUpdate = "spire-last-update"
 	labelNameServerTD   = "spire-server-td"
@@ -160,8 +173,12 @@ func buildConfig(coreConfig catalog.CoreConfig, hclText string, status *pluginco
 		if !validateCharacters(newConfig.KeyIdentifierValue) {
 			status.ReportError("Key identifier must contain only letters, numbers, underscores (_), and dashes (-)")
 		}
-		if len(newConfig.KeyIdentifierValue) > 63 {
-			status.ReportError("Key identifier must not be longer than 63 characters")
+		// The generated CryptoKey ID is spire-key-<serverID>-<spireKeyID>, and
+		// Cloud KMS caps CryptoKey IDs at maxCryptoKeyIDLength. Reserve room for
+		// the prefix, the two separators, and the longest SPIRE key ID suffix.
+		maxKeyIdentifierLength := maxCryptoKeyIDLength - len(cryptoKeyNamePrefix) - len("--") - maxSPIREKeyIDLength
+		if len(newConfig.KeyIdentifierValue) > maxKeyIdentifierLength {
+			status.ReportError(fmt.Sprintf("Key identifier must not be longer than %d characters", maxKeyIdentifierLength))
 		}
 	}
 
@@ -1028,9 +1045,10 @@ func cryptoKeyVersionAlgorithmFromKeyType(keyType keymanagerv1.KeyType) (kmspb.C
 }
 
 // generateCryptoKeyID returns a new identifier to be used as a CryptoKeyID.
-// The returned identifier has the form: spire-key-<UUID>-<SPIRE-KEY-ID>,
-// where UUID is a new randomly generated UUID and SPIRE-KEY-ID is provided
-// through the spireKeyID parameter.
+// The returned identifier has the form: spire-key-<serverID>-<SPIRE-KEY-ID>,
+// where serverID is the configured server ID (a UUID when key_identifier_file
+// is used, or an arbitrary value when key_identifier_value is used) and
+// SPIRE-KEY-ID is provided through the spireKeyID parameter.
 func (p *Plugin) generateCryptoKeyID(spireKeyID string) (cryptoKeyID string, err error) {
 	pd, err := p.getPluginData()
 	if err != nil {

pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go

@@ -265,9 +265,26 @@ func TestConfigure(t *testing.T) {
 		{
 			name:             "key identifier value too long",
 			configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","key_policy_file":"","key_ring":"%s"}`, validKeyRing)),
-			expectMsg:        "Key identifier must not be longer than 63 characters",
+			expectMsg:        "Key identifier must not be longer than 40 characters",
 			expectCode:       codes.InvalidArgument,
 		},
+		{
+			// A 41-character identifier would push the generated CryptoKey ID
+			// (spire-key-<serverID>-<spireKeyID>) past Cloud KMS's 63-character
+			// limit, so it must be rejected.
+			name:             "key identifier value one over limit",
+			configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","key_policy_file":"","key_ring":"%s"}`, validKeyRing)),
+			expectMsg:        "Key identifier must not be longer than 40 characters",
+			expectCode:       codes.InvalidArgument,
+		},
+		{
+			// A 40-character identifier is the longest that still fits.
+			name: "key identifier value at limit",
+			config: &Config{
+				KeyIdentifierValue: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+				KeyRing:            validKeyRing,
+			},
+		},
 		{
 			name: "custom policy file does not exist",
 			config: &Config{
@@ -430,7 +447,7 @@ func TestConfigure(t *testing.T) {
 			// Assert that the keys have been loaded
 			storedFakeCryptoKeys := ts.fakeKMSClient.store.fetchFakeCryptoKeys()
 			for _, expectedFakeCryptoKey := range storedFakeCryptoKeys {
-				spireKeyID, ok := getSPIREKeyIDFromCryptoKeyName(expectedFakeCryptoKey.Name)
+				spireKeyID, ok := getSPIREKeyIDFromCryptoKeyName(expectedFakeCryptoKey.Name, ts.plugin.pd.serverID)
 				require.True(t, ok)
 
 				entry, ok := ts.plugin.entries[spireKeyID]
@@ -443,6 +460,56 @@ func TestConfigure(t *testing.T) {
 	}
 }
 
+func TestGetSPIREKeyIDFromCryptoKeyName(t *testing.T) {
+	for _, tt := range []struct {
+		name          string
+		cryptoKeyName string
+		serverID      string
+		expectKeyID   string
+		expectOK      bool
+	}{
+		{
+			name:          "uuid server id (key_identifier_file)",
+			cryptoKeyName: path.Join(validKeyRing, "cryptoKeys", fmt.Sprintf("spire-key-%s-x509-CA-A", validServerID)),
+			serverID:      validServerID,
+			expectKeyID:   "x509-CA-A",
+			expectOK:      true,
+		},
+		{
+			name:          "short custom server id (key_identifier_value)",
+			cryptoKeyName: path.Join(validKeyRing, "cryptoKeys", "spire-key-dpt-gcp-38d0-x509-CA-A"),
+			serverID:      "dpt-gcp-38d0",
+			expectKeyID:   "x509-CA-A",
+			expectOK:      true,
+		},
+		{
+			name:          "server id containing dashes (key_identifier_value)",
+			cryptoKeyName: path.Join(validKeyRing, "cryptoKeys", "spire-key-my-server-JWT-B"),
+			serverID:      "my-server",
+			expectKeyID:   "JWT-B",
+			expectOK:      true,
+		},
+		{
+			name:          "prefix does not match server id",
+			cryptoKeyName: path.Join(validKeyRing, "cryptoKeys", "spire-key-other-server-x509-CA-A"),
+			serverID:      "dpt-gcp-38d0",
+			expectOK:      false,
+		},
+		{
+			name:          "no spire key id after server id",
+			cryptoKeyName: path.Join(validKeyRing, "cryptoKeys", "spire-key-dpt-gcp-38d0"),
+			serverID:      "dpt-gcp-38d0",
+			expectOK:      false,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			spireKeyID, ok := getSPIREKeyIDFromCryptoKeyName(tt.cryptoKeyName, tt.serverID)
+			require.Equal(t, tt.expectOK, ok)
+			require.Equal(t, tt.expectKeyID, spireKeyID)
+		})
+	}
+}
+
 func TestDisposeStaleCryptoKeys(t *testing.T) {
 	configureRequest := configureRequestWithDefaults(t)
 	ts := setupTest(t)