Repository navigation
fix(oidc): provision EC P-256 signing keys in Terraform to match ES256 code (follow-up to #882) - #1480
Merged
Conversation
cristim
added
triaged
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
PR #882 switched the OIDC issuer (JWKS + client-assertion signing) to require ECDSA P-256 (ES256) keys and reject any non-ECDSA key at resolve time. The Terraform modules that provision the actual signing key still specified RSA, so a fresh deploy would provision a key the Go code immediately rejects, taking down the OIDC issuer. Update all three provisioning modules to match: - AWS: aws_kms_key customer_master_key_spec -> ECC_NIST_P256 - GCP: google_kms_crypto_key version_template.algorithm -> EC_SIGN_P256_SHA256 - Azure: azurerm_key_vault_key key_type -> EC with curve = P-256 Also update the design spec (specs/azure-wif-redesign.md) to describe the ES256/EC design instead of the superseded RSA/RS256 plan. Changing the key spec forces key replacement on the next terraform apply (new kid), which is expected.
…t JWK PublicJWK's doc comment claimed only P-256 keys are accepted, but the code accepted any *ecdsa.PublicKey and unconditionally hardcoded Crv: "P-256" / Alg: "ES256" in the output. A misconfigured wrong-curve key (e.g. P-384) would be published through /.well-known/jwks.json as a mislabeled P-256 JWK with actually-longer coordinates, and the real failure would only surface later at the KMS signing call, far from the misconfiguration's cause. Check ecPub.Curve against elliptic.P256() and return an explicit error immediately in PublicJWK, and in the AWS KMS / GCP KMS signers' resolveOnce so a wrong-curve key fails fast with a clear message at key resolution instead of producing a corrupt public JWKS. (Azure Key Vault's resolveOnce already forces P-256 when parsing the raw point, so it needed no change.) Adds regression tests generating a P-384 key and asserting each path returns an error; verified failing pre-fix and passing post-fix.
cristim
force-pushed
the
fix/882-oidc-ec-vs-rsa-keys
branch
from
July 22, 2026 21:28
83165d3 to
03c3c9c
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
…y replace The cudly-terraform-deploy GitHub Actions role holds KMS data-plane and alias actions but no key-lifecycle actions, so the OIDC issuer signing key was bootstrap-created. PR #1480 migrated aws_kms_key.signing from RSA_2048 to ECC_NIST_P256 (ES256); KMS cannot change a key spec in place, so Terraform must replace the key. The deploy then failed on: AccessDenied: not authorized to perform kms:ScheduleKeyDeletion on arn:aws:kms:us-east-1:...:key/a8e28eac-... Add a tag-gated key-lifecycle grant so the replace can complete: - ScheduleKeyDeletion + management reads/mutates gated to Project=CUDly keys (deploy SA can never schedule deletion of an unrelated account CMK). - TagResource/UntagResource gated on aws:RequestTag/Project=CUDly (a new key is untagged at create, so the request tag is used). - CreateKey on "*" (cannot be resource/tag-scoped; benign - creating a CMK grants no data access; deletion stays tag-gated). Requires the ci-cd-permissions bootstrap apply to take effect before the runtime deploy can complete.
cristim
added a commit
that referenced
this pull request
Jul 23, 2026
…y replace (#1496) * fix(iac/aws): grant deploy role KMS key-lifecycle for OIDC signing-key replace The cudly-terraform-deploy GitHub Actions role holds KMS data-plane and alias actions but no key-lifecycle actions, so the OIDC issuer signing key was bootstrap-created. PR #1480 migrated aws_kms_key.signing from RSA_2048 to ECC_NIST_P256 (ES256); KMS cannot change a key spec in place, so Terraform must replace the key. The deploy then failed on: AccessDenied: not authorized to perform kms:ScheduleKeyDeletion on arn:aws:kms:us-east-1:...:key/a8e28eac-... Add a tag-gated key-lifecycle grant so the replace can complete: - ScheduleKeyDeletion + management reads/mutates gated to Project=CUDly keys (deploy SA can never schedule deletion of an unrelated account CMK). - TagResource/UntagResource gated on aws:RequestTag/Project=CUDly (a new key is untagged at create, so the request tag is used). - CreateKey on "*" (cannot be resource/tag-scoped; benign - creating a CMK grants no data access; deletion stays tag-gated). Requires the ci-cd-permissions bootstrap apply to take effect before the runtime deploy can complete. * fix(iac/aws): match KMS tag condition on Project casing, drop no-op grants The prior fix added kms:ScheduleKeyDeletion and other lifecycle actions gated on aws:ResourceTag/Project=CUDly in policy_compute_b.tf, but those actions were already granted under an identical condition by KMSMutateTaggedOnly in policy_compute.tf (IAM unions statements, so the new grants were no-ops). The real blocker is a tag-value case mismatch: the signing key's Project tag comes from var.project_name, which defaults to lowercase "cudly", overriding the provider's default_tags value of "CUDly" for that resource. StringEquals is case-sensitive, so the existing tag-gated grant never matched the signing key. Switch KMSMutateTaggedOnly's condition to StringEqualsIgnoreCase so it matches regardless of tag casing, add the one genuinely new action (kms:CancelKeyDeletion) to that statement, and remove the duplicate KMSKeyLifecycleTaggedOnly/KMSTagResourceRequestGated/KMSCreateKey statements from policy_compute_b.tf (their actions were already granted elsewhere; the RequestTag-gated UntagResource was additionally a dead grant since UntagResource evaluates aws:TagKeys, not aws:RequestTag).
cristim
added a commit
that referenced
this pull request
Jul 27, 2026
…1480) #1480 migrated aws_kms_key.signing from RSA_2048 to ECC_NIST_P256, which forces a KMS key replacement. Terraform's default destroy-before-create schedules the live signing key for deletion (immediately unusable for kms:Sign) before the new EC key exists, briefly breaking OIDC client-assertion JWT minting on deploy. Add lifecycle { create_before_destroy = true } so the new key is created and the alias cut over before the old key is scheduled for deletion. GCP crypto keys are versioned (no gap) and Azure Key Vault soft-delete keeps the old key recoverable, so only the AWS KMS key needs this.
cristim
added a commit
that referenced
this pull request
Jul 27, 2026
…1480) (#1512) #1480 migrated aws_kms_key.signing from RSA_2048 to ECC_NIST_P256, which forces a KMS key replacement. Terraform's default destroy-before-create schedules the live signing key for deletion (immediately unusable for kms:Sign) before the new EC key exists, briefly breaking OIDC client-assertion JWT minting on deploy. Add lifecycle { create_before_destroy = true } so the new key is created and the alias cut over before the old key is scheduled for deletion. GCP crypto keys are versioned (no gap) and Azure Key Vault soft-delete keeps the old key recoverable, so only the AWS KMS key needs this.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #882. That PR switched the CUDly OIDC issuer (JWKS +
client-assertion signing in
internal/oidc/*) to require ECDSA P-256(ES256) signing keys, and made the resolver reject any non-ECDSA key
at resolve time (
kms key is not ECDSA; key must be ECC_NIST_P256,etc.). However, all three Terraform modules that actually provision
the signing key still specified RSA:
terraform/modules/compute/aws/lambda/signing-key.tf:customer_master_key_spec = "RSA_2048"terraform/modules/compute/gcp/cloud-run/signing-key.tf:algorithm = "RSA_SIGN_PKCS1_2048_SHA256"terraform/modules/secrets/azure/signing-key.tf:key_type = "RSA",key_size = 2048A fresh
terraform applyon any of the three clouds would provision akey the Go code immediately rejects, taking down the OIDC issuer
(JWKS endpoint + credential minting) for the process lifetime (the
resolver caches the error via
sync.Once).Changes
ECC_NIST_P256, GCP KMS key ->EC_SIGN_P256_SHA256, Azure Key Vault key ->EC/curve = "P-256".Updated the AWS key's description and
specs/azure-wif-redesign.mdto describe the ES256/EC design instead of the superseded RSA/RS256
plan.
internal/oidc):PublicJWK's doc comment claimed only P-256keys are accepted, but it actually accepted any
*ecdsa.PublicKeyand hardcoded
Crv: "P-256"/Alg: "ES256"regardless of the realcurve. A misconfigured wrong-curve key (e.g. P-384) would be
published as a mislabeled, corrupt P-256 JWK, with the real failure
only surfacing later at the KMS signing call. Added an explicit
curve check in
PublicJWKand in the AWS/GCPresolveOncepaths soa wrong-curve key fails fast and loud at resolution time (Azure's
resolveOncealready forces P-256 when parsing the raw point, so itneeded no change).
the three paths above returns an explicit error. Verified failing
against the pre-fix code and passing post-fix.
Note: changing the KMS/Key Vault key spec forces key replacement
on the next
terraform apply(rotates thekid) -- expected andcorrect.
Test plan
go build ./...-- exit 0go vet ./...-- exit 0go test ./internal/oidc/... ./internal/credentials/...-- 119 passed, exit 0go test ./...(full suite) -- 5900 passed in 38 packages, exit 0git stashof the prod-code changes)terraform fmt -check -recursiveon the three changed modules -- exit 0terraform init -backend=false && terraform validatein each of the three changed module directories -- exit 0 (Success! The configuration is valid.)golangci-lint run ./internal/oidc/...at the exact CI-pinned version (v2.10.1, matching.github/workflows/ci.yml) -- 0 issues, exit 0gosecat the exact CI-pinned version (v2.28.0) oninternal/oidc/...-- 0 issues, exit 0gocyclo -over 10on the actually-touched files -- exit 0 (three pre-existing findings insigner_test.go/azure_factory_test.goconfirmed identical onorigin/main, unrelated to this change)Closes #882 follow-up.