add: ECDH_ES encryption support with EC Keys | fix: direct_post.jwt response mode VP token encoding#184
Conversation
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
|
Warning Review limit reached
Next review available in: 34 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR updates authorization response serialization to produce typed maps instead of JSON-encoded string maps, simplifies JWE payload construction in ChangesDirect post JWT response encryption fix
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DirectPostJwtResponseModeHandler
participant AuthorizationResponse
participant JWEHandler
participant EncryptionProvider
DirectPostJwtResponseModeHandler->>AuthorizationResponse: toMap()
AuthorizationResponse-->>DirectPostJwtResponseModeHandler: Map<String, Any>
DirectPostJwtResponseModeHandler->>JWEHandler: generateEncryptedResponse(payload)
JWEHandler->>EncryptionProvider: getEncrypter(jwk)
EncryptionProvider->>EncryptionProvider: validate alg and kty (OKP/EC)
EncryptionProvider-->>JWEHandler: X25519Encrypter or ECDHEncrypter
JWEHandler->>JWEHandler: build JWEObject with Payload(payload)
JWEHandler-->>DirectPostJwtResponseModeHandler: serialized JWE
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
kotlin/openID4VP/src/commonTest/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProviderTest.kt (1)
57-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTest locks in the swapped-argument bug from
EncryptionProvider.ktinstead of catching it.The comment at line 63 explicitly acknowledges the bug ("Current implementation populates class name as the exception message"), and the assertion at line 66 asserts
expectedMessage = "EncryptionProvider". Once the root-cause fix inEncryptionProvider.kt(swappedJweEncryptionFailureargs) is applied, this test needs updating to assert the correct descriptive message (e.g. about the unsupported curve) instead of the class name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kotlin/openID4VP/src/commonTest/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProviderTest.kt` around lines 57 - 69, Update the `EncryptionProviderTest` case for `getEncrypter` so it no longer expects the exception message to be the class name `EncryptionProvider`. After fixing the swapped `JweEncryptionFailure` arguments in `EncryptionProvider.kt`, assert the real descriptive message for the unsupported EC curve path instead, while keeping the same `INVALID_REQUEST` error code. Use the existing `assertOpenId4VPException` helper and the `getEncrypter`/`JweEncryptionFailure` symbols to locate the affected assertion.
🧹 Nitpick comments (3)
kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/authorizationResponse/AuthorizationResponse.kt (1)
50-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving
toJsonEncodedMap()fromtoMap()to avoid duplicated branching.Both functions branch on the same sealed subtypes with nearly identical shape. Not urgent, but worth consolidating to avoid the two implementations drifting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/authorizationResponse/AuthorizationResponse.kt` around lines 50 - 66, The branching logic for AuthorizationResponse serialization is duplicated between toMap() and toJsonEncodedMap(), which risks the two implementations drifting. Refactor toJsonEncodedMap() to derive its payload from toMap() in AuthorizationResponse, and keep the subtype handling in one place so both PresentationExchange and Dcql stay consistent. Use the existing toMap() and AuthorizationResponse sealed subclasses as the single source of truth, and only apply JSON-encoding-specific transformation after that shared map is built.kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/JWEHandler.kt (1)
35-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBroad
catch (exception: Exception)re-wraps already-typedOpenID4VPExceptions(e.g.UnsupportedKeyExchangeAlgorithm,JweEncryptionFailurefromEncryptionProvider.getEncrypter).
EncryptionProvider.getEncrypter(publicKey)is called inside this try block and can throwJweEncryptionFailureorUnsupportedKeyExchangeAlgorithm. Both get caught by the genericExceptionhandler and re-wrapped into a newJweEncryptionFailure, losing the original exception type (only the message text is preserved, nested with a "JWE Encryption failed :" prefix). Callers/tests that pattern-match on the specific exception subtype downstream of this call path would no longer see it.♻️ Suggested fix — rethrow already-typed exceptions as-is
val encrypter = EncryptionProvider.getEncrypter(publicKey) val jweObject = JWEObject(header, Payload(payload)) jweObject.encrypt(encrypter) return jweObject.serialize() + } catch (exception: OpenID4VPExceptions) { + throw exception } catch (exception: Exception) { throw OpenID4VPExceptions.JweEncryptionFailure( "JWE Encryption failed : " + (exception.message ?: "Unknown error"), className, exception ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/JWEHandler.kt` around lines 35 - 46, In JWEHandler.encrypt, the broad catch block is re-wrapping exceptions that are already OpenID4VPExceptions, which hides the original subtype. Update the catch logic so EncryptionProvider.getEncrypter and jweObject.encrypt failures that already throw OpenID4VPExceptions (such as UnsupportedKeyExchangeAlgorithm and JweEncryptionFailure) are rethrown unchanged. Keep only unexpected exceptions wrapped into JweEncryptionFailure, preserving the current className context where needed.kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProvider.kt (1)
77-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing
ycoordinate throwsUnsupportedKeyExchangeAlgorithminstead of a message describing the actual problem.Throwing
UnsupportedKeyExchangeAlgorithm(message: "Required Key exchange algorithm is not supported") for a missingycoordinate is misleading — the algorithm/key exchange type is fine; the JWK is simply malformed/incomplete. AJweEncryptionFailurewith a specific message (e.g. "EC key is missing required 'y' coordinate") would aid debugging conformance-test decryption failures, which is a stated goal of this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProvider.kt` around lines 77 - 90, The `getPublicEcKey` helper is throwing `UnsupportedKeyExchangeAlgorithm` when `jwk.y` is missing, which masks the real issue. Update this branch in `EncryptionProvider` to fail with a `JweEncryptionFailure` (or the project’s equivalent encryption error) and a clear message that the EC JWK is incomplete, explicitly mentioning the missing `y` coordinate. Keep the rest of `getPublicEcKey` unchanged so valid `Curve`, `jwk.x`, `jwk.kid`, `jwk.alg`, and `jwk.use` handling still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/authorizationResponse/AuthorizationResponse.kt`:
- Around line 50-59: The AuthorizationResponse.toMap() helper is mutating the
shared Jackson mapper via getObjectMapper(), which can affect unrelated
serialization elsewhere. Update AuthorizationResponse.toMap() to copy the mapper
first and then set JsonInclude.Include.NON_NULL on the copy before converting
vpToken and presentationSubmission, keeping the change local to this method.
In
`@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProvider.kt`:
- Around line 44-59: The EC-curve validation in EncryptionProvider’s JWK
handling has the JweEncryptionFailure constructor arguments swapped, so the
exception message and className are reversed. Update the EC branch to pass the
descriptive unsupported-curve error text as the message and the current
className as the className, matching the OKP branch’s usage. Also change the
wording to refer to the unsupported curve rather than the recipient key type,
and update the related EncryptionProviderTest assertion to expect the corrected
message/className order.
---
Duplicate comments:
In
`@kotlin/openID4VP/src/commonTest/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProviderTest.kt`:
- Around line 57-69: Update the `EncryptionProviderTest` case for `getEncrypter`
so it no longer expects the exception message to be the class name
`EncryptionProvider`. After fixing the swapped `JweEncryptionFailure` arguments
in `EncryptionProvider.kt`, assert the real descriptive message for the
unsupported EC curve path instead, while keeping the same `INVALID_REQUEST`
error code. Use the existing `assertOpenId4VPException` helper and the
`getEncrypter`/`JweEncryptionFailure` symbols to locate the affected assertion.
---
Nitpick comments:
In
`@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/authorizationResponse/AuthorizationResponse.kt`:
- Around line 50-66: The branching logic for AuthorizationResponse serialization
is duplicated between toMap() and toJsonEncodedMap(), which risks the two
implementations drifting. Refactor toJsonEncodedMap() to derive its payload from
toMap() in AuthorizationResponse, and keep the subtype handling in one place so
both PresentationExchange and Dcql stay consistent. Use the existing toMap() and
AuthorizationResponse sealed subclasses as the single source of truth, and only
apply JSON-encoding-specific transformation after that shared map is built.
In
`@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProvider.kt`:
- Around line 77-90: The `getPublicEcKey` helper is throwing
`UnsupportedKeyExchangeAlgorithm` when `jwk.y` is missing, which masks the real
issue. Update this branch in `EncryptionProvider` to fail with a
`JweEncryptionFailure` (or the project’s equivalent encryption error) and a
clear message that the EC JWK is incomplete, explicitly mentioning the missing
`y` coordinate. Keep the rest of `getPublicEcKey` unchanged so valid `Curve`,
`jwk.x`, `jwk.kid`, `jwk.alg`, and `jwk.use` handling still works.
In
`@kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/JWEHandler.kt`:
- Around line 35-46: In JWEHandler.encrypt, the broad catch block is re-wrapping
exceptions that are already OpenID4VPExceptions, which hides the original
subtype. Update the catch logic so EncryptionProvider.getEncrypter and
jweObject.encrypt failures that already throw OpenID4VPExceptions (such as
UnsupportedKeyExchangeAlgorithm and JweEncryptionFailure) are rethrown
unchanged. Keep only unexpected exceptions wrapped into JweEncryptionFailure,
preserving the current className context where needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17c5f9b2-9957-4a81-ba6f-2bcd1e4bc8f4
📒 Files selected for processing (8)
kotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/authorizationResponse/AuthorizationResponse.ktkotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/exceptions/openId4VPExceptions.ktkotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/JWEHandler.ktkotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProvider.ktkotlin/openID4VP/src/commonMain/kotlin/io/mosip/openID4VP/responseModeHandler/types/DirectPostJwtResponseModeHandler.ktkotlin/openID4VP/src/commonTest/kotlin/io/mosip/openID4VP/jwt/jwe/JWEHandlerTest.ktkotlin/openID4VP/src/commonTest/kotlin/io/mosip/openID4VP/jwt/jwe/encryption/EncryptionProviderTest.ktkotlin/openID4VP/src/jvmTest/kotlin/io/mosip/openID4VP/jwt/JWEHandlerJvmTest.kt
KiruthikaJeyashankar
force-pushed
the
add-encryption-support-ec-keys
branch
from
d83f502 to
18e7974
Compare
Signed-off-by: KiruthikaJeyashankar <kiruthikavjshankar@gmail.com>
KiruthikaJeyashankar
force-pushed
the
add-encryption-support-ec-keys
branch
from
18e7974 to
7af4d7c
Compare
1 participant
Fixes: inji/inji-wallet#2482
Summary by CodeRabbit
New Features
Bug Fixes