[IAmJSD]

Root cause

AzureKeyVaultSigner.resolveOnce (internal/oidc/azure_signer.go) read only X and Y from the Key Vault JWK and parsed them with ecdsa.ParseUncompressedPublicKey(elliptic.P256(), ...). It never looked at resp.Key.Kty or resp.Key.Crv, even though Sign always requests ES256. So:

• a P-384 or P-521 key failed with EC key coordinate exceeds 32 bytes, which does not say what is actually wrong;
• a P-256K (secp256k1) key, whose coordinates are also 32 bytes, was accepted whenever the point also parsed on P-256, and would then be published in the JWKS as "crv":"P-256";
• a key with a missing or non-EC kty was not rejected either.

PublicJWK in jwks.go already rejects non-P-256 keys; the Azure fetch side did not.

Fix

Before building the point, resolveOnce now calls a new validateES256KeyType helper. It requires kty to be EC or EC-HSM and crv to be P-256, and treats a missing field as a failure. The error names the key and the values Key Vault reported, for example:

oidc: azure keyvault key "test-key": must be an EC or EC-HSM key on curve P-256 for ES256, got kty="EC" crv="P-384"

This covers both parts of the issue: the original Crv check and the Kty check added in the 2026-07-28 review comment. The Terraform module already provisions key_type = "EC", curve = "P-256" (terraform/modules/secrets/azure/signing-key.tf), so correctly provisioned deployments are not affected.

The test fake fakeAzureKeyVaultClient now reports kty/crv the way Key Vault does. By default it sends EC and the curve of the backing key, and it can override or omit either field. The other Azure fake, in backend_jws_test.go, already set both fields.

Closes #46

Verification

All of this is unit/fake evidence. No real Key Vault was called, so whether a live Key Vault always fills kty/crv for EC keys rests on the azkeys SDK model and the Key Vault REST docs, not on a live test. Tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because the local ~/.aws/config default profile is broken. That setting does not affect this package's logic.

Fail before / pass after with the new TestAzureSigner_RejectsNonP256Key (8 cases: EC P-256 accepted, EC-HSM P-256 accepted, P-384, P-521, P-256K with P-256-valid coordinates, missing crv, RSA kty, missing kty). Each rejected case also checks that PublicKey returns nil, that KeyID returns the same error, and that the error names the key.

go test ./internal/oidc/ -run TestAzureSigner_RejectsNonP256Key -count=1

• Before the fix (test only): 6 of 8 subtests FAIL. P-521, P-256K, missing crv, RSA and missing kty get An error is expected but got nil. P-384 gets "...coordinate exceeds 32 bytes" does not contain "crv=\"P-384\"".
• After the fix: 8 of 8 PASS.

Mutation probes, each applied separately and reverted. Every one makes go test ./internal/oidc/ fail:

• drop the EC-HSM acceptance
• ktyOK && crvOK changed to ||
• ignore kty
• ignore crv
• accept a missing crv
• accept a missing kty
• skip the validation call
• drop the key name from the error

Other checks:

• go build ./...: OK
• go vet ./internal/oidc/: OK
• go test ./internal/oidc/ -count=1 and go test -race ./internal/oidc/ -count=1: OK
• go test ./internal/mocks/ -count=1: OK
• golangci-lint v2.10.1 run --allow-parallel-runners ./internal/oidc/...: 0 issues
• gocyclo -over 10 -ignore "_test\.go" internal/oidc/: silent. resolveOnce is now at exactly 10; the new checks live in a helper to keep it at the limit.
• git-secrets --scan on both changed files, in a fresh git init repo with an isolated HOME using scripts/setup-git-secrets.sh and .gitallowed: clean
• scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL
• gofmt -l and git diff --check: clean

There are no DB, migration or concurrency changes. The check runs inside the existing sync.Once, so concurrent callers behave as before.

Out of scope / notes

• A rejected key is cached as a permanent error by the existing sync.Once, just like every other resolveOnce failure today. That is the right result for a misconfigured key, which needs a redeploy anyway. Retrying after transient errors is handled separately by test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152.
• test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152 (also mine, draft) replaces resolveOnce with fetchPublicKey. Whichever of the two merges second will hit a small textual conflict: the four-line validateES256KeyType call moves into fetchPublicKey as return nil, fmt.Errorf(...). I'll resolve it with a merge commit on whichever branch needs it.
• The AWS and GCP signers are unchanged. AWS KMS reports KeySpec and GCP reports the key version's algorithm, and whether those backends need the same check is a separate question.

Maintainers: please mirror the issue's triage labels onto this PR: bug, triaged, type/security, priority/p3, severity/low, urgency/eventually, impact/few, effort/s.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Azure Key Vault signing now validates key metadata before use.
  • Only EC or EC-HSM keys using the P-256 curve are accepted.
  • Unsupported key types, curves, or missing metadata now produce clear errors instead of being processed.

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6be10a8a-9bb7-4b2e-a8b9-385dd35b8862

📥 Commits

Reviewing files that changed from the base of the PR and between e304ebe and 79f5604.

📒 Files selected for processing: 2

• internal/oidc/azure_factory_test.go
• internal/oidc/azure_signer.go

+----------------------------------------------------------------------------------+
| Crash early. A dead program normally does a lot less damage than a crippled one. |
+----------------------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 79f5604

Findings (none blocking; a related low-severity note was shared privately with the maintainers per SECURITY.md):

• [non-blocking] internal/oidc/azure_signer.go:149 - kty is compared by exact string, and no test covers a case-folded value. A probe switching the check to strings.EqualFold(kty, "ec") passes all 40 tests. This is harmless today: Key Vault returns canonical enum strings, and azkeys unpopulate copies "kty"/"crv" exactly (models_serde.go:364). So "ec", "ec-hsm" and "p-256" are all rejected, which an ad-hoc test confirmed.
• [non-blocking, existing behaviour] azure_signer.go:103-140 - The check runs lazily inside a sync.Once on the first PublicKey/KeyID call, not when the factory builds the signer (factory.go:58). A bad key fails closed on the first JWKS request or Mint, not at startup, and that error is cached permanently (test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152 covers retry).
  • GetKey and Sign both use version "" (latest), and the key is fetched once.
  • After a Key Vault rotation, Sign uses the new version while the JWKS and kid still describe the cached old one, and the new check never runs on the new version.
  • The result is a verification failure, not a forgery.

Against the issue: the missing Crv check is fixed, together with the Kty check requested in the issue's review comment. It is not a duplicate of PublicJWK (jwks.go:47): that checks the curve on the parsed key, and cannot tell P-256K from P-256 once the coordinates are parsed as P-256. Deleting validateES256KeyType fails 6 subtests.

Attacked:

• azkeys v1.4.0:
  • constants.go:12-18 defines CurveName P-256, P-256K, P-384 and P-521; :181-189 define KeyType EC, EC-HSM, oct and RSA.
  • models.go:130 JSONWebKey has Crv *CurveName and Kty *KeyType, and the PR nil-checks both before dereferencing.
• Rejected inputs, all failing closed:
  • Real RSA keys: Key Vault returns N/E with nil X/Y, so azure_signer.go:110 rejects them already. The kty check also rejects an RSA kty that arrives with X/Y.
  • oct, mixed case and a leading-space " P-256".
  • P-256K, P-384 and P-521.
  • A missing kty or crv.
• Mint (signer.go:112) calls KeyID before Sign, so a rejected key can never sign.
• JWKS vs signing key: the kid is ComputeKeyID of the parsed point and is the same in the JWKS (jwks.go:83) and the Mint header. alg is ES256 everywhere.
• Mutation probes, each restored and confirmed identical with cmp:
  • Dropping EC-HSM: FAILS.
  • && changed to ||: FAILS.
  • Skipping the validation call: FAILS.
  • Also accepting P-256K: FAILS.
  • Case-folding kty: passes (finding 1).
• Blast radius: the helpers are unexported with no other callers, and git merge-tree against origin/main is clean.

Verified locally, on a git archive export of HEAD with no cloud credentials:

• go build ./... ok. go vet ./internal/oidc/ ok.
• go test -count=1 ./internal/oidc/ ./internal/api/: ok. gofmt -l internal/oidc/: clean.
• Fail-before: the head azure_factory_test.go on the base export FAILS 6 of 8 subtests (P-384, P-521, P-256K, missing crv, RSA, missing kty), matching the PR.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 79f5604

Scope: git diff origin/main...79f5604e (1 commit, 154+/3-, 157 changed lines, under the 400 cap). git merge-tree --write-tree origin/main 79f5604e: clean.

The change fixes #46 as the issue and its 2026-07-28 comment ask: kty must be EC or EC-HSM, crv must be P-256, a missing field fails, and the error names the key and the reported values. Constants match azkeys v1.4.0 (KeyTypeEC="EC", KeyTypeECHSM="EC-HSM", CurveNameP256="P-256", distinct from CurveNameP256K="P-256K"), which match the Key Vault JWK key types and RFC 7518 §6.2.1.1 crv values. No silent fallback: a rejected key sets s.err inside sync.Once, PublicKey/KeyID return nil plus the error, and signer.go:112 calls KeyID before Sign (signer.go:136), so token minting fails loudly and jwks.go:79-83 publishes nothing for that key.

Findings:

• nit internal/oidc/azure_signer.go:110-117 - the new type check runs after the X == nil || Y == nil guard. A real RSA or oct key from Key Vault carries N/E or K, not X/Y, so it still fails with the old "incomplete EC key (missing X or Y)" message and never reaches the check that would name kty="RSA". The RSA key type rejected test sets X/Y on an RSA kty, a shape Key Vault does not return. Fail mode stays loud, only the diagnostic is weaker. Running the type check right after resp.Key == nil would cover it.
• nit: the P-256K case in the test uses P-256 coordinates. A real secp256k1 point almost never lies on P-256, so before the fix it usually failed in ParseUncompressedPublicKey, not by being accepted. The check is still right, and the test does exercise the reported-curve branch.

Attacked:

• Callers of PublicKey/KeyID/Sign (signer.go, jwks.go): error propagates, no path signs with an unvalidated key.
• Other Azure fakes: backend_jws_test.go:124-125 already sets Kty/Crv, so it is not silently broken by the new check.
• go test ./internal/oidc/ -count=1: ok.
• Mutation probes (each in a scratch export, reverted after):
  • drop the EC-HSM acceptance: TestAzureSigner_RejectsNonP256Key FAIL
  • ktyOK && crvOK to ||: FAIL
  • accept a missing crv: FAIL
  • accept a missing kty: FAIL
  • remove the validateES256KeyType call: FAIL
  • accept P-256K instead of P-256: FAIL (3 tests)
• Not verified: a live Key Vault response. Evidence comes from the fake and the SDK model only.

[cristim]

Local verification at 79f5604e479f607bc93c3e81b4fe76bf9e6e7753 merged onto current main (9fceda3c; merge-tree a9f1a26c; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at 79f5604e479f607bc93c3e81b4fe76bf9e6e7753).