sec(oidc): switch JWT signing from PKCS1v15 to ES256#882
Conversation
Replace RSA-PKCS1v15 (RS256) with ECDSA P-256 (ES256) across the entire oidc package: - LocalSigner: ecdsa.GenerateKey(P256) + ecdsa.SignASN1 instead of rsa.GenerateKey + rsa.SignPKCS1v15 - Signer interface: PublicKey() returns crypto.PublicKey (was *rsa.PublicKey) - Algorithm constant: "ES256" (was "RS256") - ComputeKeyID: accepts crypto.PublicKey, hashes uncompressed EC point - JWK: EC fields (kty=EC, crv=P-256, x, y); RSA fields (n, e) removed - AWSKMSSigner: SigningAlgorithmSpecEcdsaSha256 + *ecdsa.PublicKey assertion - AzureKeyVaultSigner: SignatureAlgorithmES256 + EC key (X/Y) extraction - GCPKMSSigner: *ecdsa.PublicKey assertion (digest call is key-type-agnostic) - All tests: positive assertions on ES256 algorithm and EC JWK fields
|
Warning Review limit reached
More reviews will be available in 22 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…er backends (refs #422) Mint base64url-encoded the signer's raw output directly into the JWS signature segment, but the Local/AWS/GCP signers return DER/ASN.1 ECDSA signatures (ecdsa.SignASN1, AWS ECDSA_SHA_256, GCP EC sign all yield ASN.1 DER). RFC 7518 section 3.4 requires an ES256 JWS signature to be the raw fixed-length R || S concatenation (32 bytes each = 64 bytes for P-256), NOT DER. As a result AWS/GCP/Local minted tokens that real OIDC consumers (the stated Azure AD target) reject. Azure Key Vault already returns raw R||S, so only Azure was correct, leaving the four backends mutually inconsistent. The Azure Sign comment also wrongly claimed Key Vault returns DER. Contract: Signer.Sign now returns the RFC 7518 raw R||S form. DER backends (Local/AWS/GCP) convert via a single shared derToRawECDSASignature helper; Azure passes through unchanged (no double-conversion). Mint encodes the result directly and defensively rejects any signature that is not 64 bytes. - signer.go: add derToRawECDSASignature; LocalSigner.Sign converts; Mint enforces the 64-byte contract; document the contract on the Signer interface. - aws_signer.go, gcp_signer.go: convert KMS DER output to raw R||S. - azure_signer.go: fix the wrong "DER-encoded" comment; keep passthrough. - tests: add assertRawES256JWS asserting the JWS sig is exactly 64 bytes, splitting R||S for ecdsa.Verify, and parsing with a real golang-jwt ES256 parser. Cover Local, AWS (DER fake), GCP (DER fake) and Azure (raw fake). The DER-path tests fail on the pre-fix code (71/69-byte signatures) and pass after; the Azure raw path stays correct. - go.mod: promote golang-jwt/jwt/v5 to a direct dependency (test use).
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
internal/oidcpackageAlgorithmconstant,Signerinterface,LocalSigner, all three KMS signer stubs, the JWKS builder, and all testskty=EC,crv=P-256,x,y) -- regression to RS256 will fail testsCloses #422
Test plan
go test ./internal/oidc/...-- 9 tests passgo test ./internal/api/...-- handler_oidc_test.go asserts EC JWK fieldsgo vet ./...-- cleanTestLocalSignerMintAndVerifyassertsalg == "ES256"and verifies withecdsa.VerifyASN1TestBuildJWKSassertskty=EC,crv=P-256, presence ofx/y, absence of RSAn/eTestBuildDiscoveryassertsid_token_signing_alg_values_supported == ["ES256"]