Skip to content

remove undocumented requirement for gcpkms key_identifier_values to be 36 characters#7140

Open
daescha wants to merge 2 commits into
spiffe:mainfrom
daescha:rm-hidden-requirement-for-gcpkms-key_identifier_values-to-be-36-chars
Open

remove undocumented requirement for gcpkms key_identifier_values to be 36 characters#7140
daescha wants to merge 2 commits into
spiffe:mainfrom
daescha:rm-hidden-requirement-for-gcpkms-key_identifier_values-to-be-36-chars

Conversation

@daescha

@daescha daescha commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

this was an undocumented requirement. no docs updates needed.

Affected functionality

gcpkms config

Description of change

gcpkms plugin now accepts arbitrary length values for key_identifier_value.

Which issue this PR fixes

when using gcpkms plugin config key_identifier_value, there was a requirement for the value to be 36 characters exactly. This is not the case for the azurekeyvault or awskms plugins, and was a pain to find out. heres the error message

time="2026-07-10T16:58:42.50670432Z" level=error msg="Unable to rotate X509 CA" error="rpc error: code = Internal desc = keymanager(gcp_kms): failed to generate key: rpc error: code = Internal desc = failed to create CryptoKey: rpc error: code = AlreadyExists desc = CryptoKey projects/<redacted>/locations/<redacted>/keyRings/<redacted>/cryptoKeys/spire-key-a1b2c3d4-x509-CA-A already exists.\nerror details: name = ErrorInfo reason = RESOURCE_ALREADY_EXISTS domain = cloudkms.googleapis.com metadata = map[resource_name:projects/<redacted>/locations/<redacted>/keyRings/<redacted>/cryptoKeys/spire-key-a1b2c3d4-x509-CA-A]" subsystem_name=ca_manager 

Copilot AI review requested due to automatic review settings July 11, 2026 01:24
@daescha
daescha force-pushed the rm-hidden-requirement-for-gcpkms-key_identifier_values-to-be-36-chars branch from e3be3b2 to 39fddd4 Compare July 11, 2026 01:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the implicit/undocumented assumption that the GCP KMS key identifier (server ID) is a fixed-length UUID by updating CryptoKey name parsing to use the configured server ID prefix instead of a hard-coded offset. This aligns the gcpkms plugin behavior with key_identifier_value supporting non-UUID identifiers.

Changes:

  • Update getSPIREKeyIDFromCryptoKeyName to derive the SPIRE key ID by cutting a spire-key-<serverID>- prefix instead of using a fixed UUID-length offset.
  • Update call sites to pass the configured serverID into the parser.
  • Add unit tests covering UUID and non-UUID server ID formats.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/server/plugin/keymanager/gcpkms/fetcher.go Switch CryptoKey name parsing to strip a variable-length server ID prefix when deriving SPIRE key IDs.
pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go Update parser call site and add table-driven tests for variable-length server IDs in CryptoKey names.

Comment thread pkg/server/plugin/keymanager/gcpkms/fetcher.go Outdated
…e 36 characters

Signed-off-by: Daniel Schlatter <djschlatt@gmail.com>
@daescha
daescha force-pushed the rm-hidden-requirement-for-gcpkms-key_identifier_values-to-be-36-chars branch from eb4667e to ea90a5e Compare July 11, 2026 08:19

@sorindumitru sorindumitru left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me @daescha! Do you know if there's an upper limit to the key name? This is what I could find. If that's the requirement for the key name, could we also validate that the provided identifier matches it?

@github-actions github-actions Bot assigned daescha and unassigned sorindumitru Jul 13, 2026
@daescha

daescha commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@sorindumitru assuming you are referring to that regex on the page ([a-zA-Z0-9_-]{1,63}), that sounds right to me. Ill add code to validate that.

…name rules

Signed-off-by: Daniel Schlatter <djschlatt@gmail.com>
@daescha

daescha commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@sorindumitru i restricted key length to 40 chars to give room for the bits we are adding on to it and stay within gcps stated max of 63.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants