[ES-2673] [ES-2655] Public key validations #1606

Md-Humair-KK · 2026-01-13T19:18:02Z

Added custom validations on top of public key validations

Summary by CodeRabbit

Bug Fixes
- Tightened JWK validation and error handling: mandatory kty/alg/use checks, consistent INVALID_PUBLIC_KEY responses, explicit rejection and logging for unsupported key types, and improved parsing error handling.
Tests
- Expanded unit tests for many valid/invalid RSA and EC JWK scenarios, including missing/empty alg/use and invalid alg/use cases.
Documentation
- Updated docs describing the tightened JWK validation and construction behavior.
Style
- Minor import cleanup and consistency updates.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Signed-off-by: Md-Humair-KK <[email protected]>

coderabbitai · 2026-01-13T19:18:16Z

Walkthrough

Validate and enforce presence/values of kty, alg, and use in getJWKString; construct RSA/EC JWK JSON via switch-expression; convert JoseExceptions and invalid inputs to EsignetException(INVALID_PUBLIC_KEY); expanded tests and aligned several test RSA JWK builders to use algorithm RS256.

Changes

Cohort / File(s)	Summary
JWK validation & utilities `esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java`	Added upfront null/empty checks for `kty`, `alg`, and `use`; validated `alg`/`use` values; switched to a switch-expression to build RSA/EC JWK JSON; explicit logging and throwing of `EsignetException(INVALID_PUBLIC_KEY)` on invalid/missing values or JoseException; minor import cleanup.
Unit tests (core JWK coverage) `esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java`	Replaced brittle assertions with assertThrows and added granular tests for missing/empty `kty`/`alg`/`use`, unsupported `kty`, invalid `alg`/`use` for RSA, and valid RSA/EC scenarios (including full RSA JWK).
Test JWK algorithm alignment `client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java`, `binding-service-impl/src/test/java/io/mosip/esignet/KeyBindingServiceTest.java`, `esignet-service/src/test/java/io/mosip/esignet/TestUtil.java`	Set RSA JWK builder algorithm to `RS256` via `.algorithm(new Algorithm("RS256"))` in multiple tests to align generated JWK content; no control-flow changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through maps of kty and alg,
I sniffed for missing use and gave a wag,
I stitched a JWK with careful paws,
Ran tests that caught the sneaky flaws,
A carrot for each passing clause!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: adding public key validations (specifically in IdentityProviderUtil.getJWKString with upfront validation of kty, use, and alg fields).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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.

❤️ Share

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java (1)

186-196: Consider adding negative test cases for new validation branches.

This positive test case is good for verifying the happy path with a complete RSA JWK. However, the new validation logic in getJWKString introduces several validation branches that lack test coverage:

Invalid algorithm for RSA (e.g., alg: "ES256" with kty: "RSA")
Invalid algorithm for EC (e.g., alg: "RS256" with kty: "EC")
Invalid use field (e.g., use: "enc")
Invalid RSA exponent (e.g., e: "AQE")
Valid EC key with algorithm specified

🧪 Example additional test cases

@Test
public void getJWKString_withInvalidAlgForRSA_thenFail() {
    Map<String, Object> jwkMap = new HashMap<>();
    jwkMap.put("kty", "RSA");
    jwkMap.put("n", "oahUIzUup5kqncCkHk5Zb1pRrLx7e6YtM-9jX1f5e6mHnZFkC2LJUZ0sEh0n5Y5KnQfW9s7d7gK2b8P0EEl0h3ZyHkWzA3YbsgzB4pDxP4RxMZ1I8xD2z3UvfA1zjvKDHz6wEweq4hVJ8nS8GzZJ2E_vb3s");
    jwkMap.put("e", "AQAB");
    jwkMap.put("alg", "ES256"); // Invalid for RSA

    EsignetException ex = Assertions.assertThrows(EsignetException.class,
        () -> IdentityProviderUtil.getJWKString(jwkMap));
    Assertions.assertEquals(ErrorConstants.INVALID_PUBLIC_KEY, ex.getMessage());
}

@Test
public void getJWKString_withInvalidUse_thenFail() {
    Map<String, Object> jwkMap = new HashMap<>();
    jwkMap.put("kty", "RSA");
    jwkMap.put("n", "oahUIzUup5kqncCkHk5Zb1pRrLx7e6YtM-9jX1f5e6mHnZFkC2LJUZ0sEh0n5Y5KnQfW9s7d7gK2b8P0EEl0h3ZyHkWzA3YbsgzB4pDxP4RxMZ1I8xD2z3UvfA1zjvKDHz6wEweq4hVJ8nS8GzZJ2E_vb3s");
    jwkMap.put("e", "AQAB");
    jwkMap.put("use", "enc"); // Invalid - only "sig" allowed

    EsignetException ex = Assertions.assertThrows(EsignetException.class,
        () -> IdentityProviderUtil.getJWKString(jwkMap));
    Assertions.assertEquals(ErrorConstants.INVALID_PUBLIC_KEY, ex.getMessage());
}

@Test
public void getJWKString_withInvalidRSAExponent_thenFail() {
    Map<String, Object> jwkMap = new HashMap<>();
    jwkMap.put("kty", "RSA");
    jwkMap.put("n", "oahUIzUup5kqncCkHk5Zb1pRrLx7e6YtM-9jX1f5e6mHnZFkC2LJUZ0sEh0n5Y5KnQfW9s7d7gK2b8P0EEl0h3ZyHkWzA3YbsgzB4pDxP4RxMZ1I8xD2z3UvfA1zjvKDHz6wEweq4hVJ8nS8GzZJ2E_vb3s");
    jwkMap.put("e", "Aw"); // Invalid exponent (base64url of 3)

    EsignetException ex = Assertions.assertThrows(EsignetException.class,
        () -> IdentityProviderUtil.getJWKString(jwkMap));
    Assertions.assertEquals(ErrorConstants.INVALID_PUBLIC_KEY, ex.getMessage());
}

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 423cb1c and e6e579b.

📒 Files selected for processing (2)

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java
esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.

📚 Learning: 2025-12-23T05:15:42.700Z

Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.

Applied to files:

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java
esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java

🧬 Code graph analysis (1)

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java (1)

esignet-core/src/main/java/io/mosip/esignet/core/constants/ErrorConstants.java (1)

ErrorConstants (8-90)

🔇 Additional comments (2)

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java (2)

221-261: Validation logic is well-structured and addresses key security concerns.

The enhanced validation correctly:

Enforces algorithm-key type compatibility when alg is specified

Restricts use to signature keys only

Enforces standard RSA public exponent (65537 encoded as "AQAB")

The comment on line 231 appropriately documents why custom validations are needed.

One minor consideration: the jwk.get("e") comparison at line 245 relies on the value being a String. If a malformed JWK contains a non-String type for e, the .equals("AQAB") call will safely return false (triggering the exception), so this is not a bug. However, if you want to be defensive, you could add an explicit type check.

68-77: Document the rationale for limiting RSA algorithms to RS256 and PS256. The current implementation intentionally restricts RSA_ALGORITHMS to only 256-bit variants (RS256, PS256), which differs from EC_ALGORITHMS that supports all standard variants (ES256, ES384, ES512). While test evidence confirms this limitation is by design—TokenServiceTest.java verifies that PS384 is properly rejected as INVALID_PUBLIC_KEY—the design rationale is not documented in code comments. Add a comment explaining whether this restriction is for security policy compliance, performance considerations, or other reasons.

Likely an incorrect or invalid review comment.

Signed-off-by: Md-Humair-KK <[email protected]>

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java (2)
224-234: LGTM with a minor suggestion.

The test correctly validates that RSA exponent must be "AQAB" (65537). Note that "Aw" decodes to 3, which is mathematically valid but considered insecure. Consider adding a brief comment clarifying the security rationale.
📝 Optional: Add clarifying comment
 @Test
 public void getJWKString_withInvalidRSAExponent_thenFail() {
     Map<String, Object> jwkMap = new HashMap<>();
     jwkMap.put("kty", "RSA");
     jwkMap.put("n", "oahUIzUup5kqncCkHk5Zb1pRrLx7e6YtM-9jX1f5e6mHnZFkC2LJUZ0sEh0n5Y5KnQfW9s7d7gK2b8P0EEl0h3ZyHkWzA3YbsgzB4pDxP4RxMZ1I8xD2z3UvfA1zjvKDHz6wEweq4hVJ8nS8GzZJ2E_vb3s");
-    jwkMap.put("e", "Aw");
+    jwkMap.put("e", "Aw"); // Base64url for 3; rejected because only 65537 ("AQAB") is allowed

     EsignetException ex = Assertions.assertThrows(EsignetException.class,
             () -> IdentityProviderUtil.getJWKString(jwkMap));
     Assertions.assertEquals(ErrorConstants.INVALID_PUBLIC_KEY, ex.getMessage());
 }
236-246: Add EC-specific validation failure tests to match RSA test coverage.

The implementation validates EC keys symmetrically with RSA keys (lines 236-237: algorithm validation via EC_ALGORITHMS containing ES256, ES384, ES512; lines 240-241: use validation requiring "sig"). However, test coverage is asymmetric: RSA has five tests including failure cases, while EC has only the valid-key test.

Consider adding corresponding EC failure tests:

getJWKString_withInvalidAlgForEC_thenFail – EC key with RSA algorithm (e.g., "RS256")

getJWKString_withInvalidUseForEC_thenFail – EC key with non-signature use (e.g., "enc")

This ensures both validation paths are equally exercised in tests.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6e579b and e8cfe0e.

📒 Files selected for processing (1)

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.

📚 Learning: 2025-12-23T05:15:42.700Z

Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.

Applied to files:

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java

🧬 Code graph analysis (1)

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java (1)

esignet-core/src/main/java/io/mosip/esignet/core/constants/ErrorConstants.java (1)

ErrorConstants (8-90)

🔇 Additional comments (3)

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java (3)

186-196: LGTM!

The test correctly validates that a full RSA JWK with kty, n, e, alg, and use fields is accepted. The assertion confirms the key type is properly included in the output.

198-209: LGTM!

Good test for algorithm-key type mismatch. Uses modern assertThrows pattern appropriately.

211-222: LGTM!

Correctly validates that only "sig" key use is accepted, rejecting "enc" as expected.

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java

Signed-off-by: Md-Humair-KK <[email protected]>

sacrana0

lgtm

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java

Signed-off-by: Md-Humair-KK <[email protected]>

coderabbitai

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In
`@esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java`:
- Around line 221-247: In getJWKString, add explicit allow-list checks for alg
and use before constructing RsaJsonWebKey or EllipticCurveJsonWebKey: verify use
equals "sig" and for keyType == "RSA" ensure alg is one of
{"RS256","RS384","RS512"}, and for keyType == "EC" ensure alg is one of
{"ES256","ES384","ES512"}; if any check fails log an error and throw
EsignetException(ErrorConstants.INVALID_PUBLIC_KEY) so mismatched key/alg or
wrong use are rejected prior to calling RsaJsonWebKey or
EllipticCurveJsonWebKey.

In
`@esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java`:
- Around line 231-254: The two tests call IdentityProviderUtil.getJWKString but
each omits the complementary JWK field so they fail for missing fields rather
than invalid values; update getJWKString_withInvalidAlgForRSA_thenFail to
include a valid "use" (e.g., "sig") alongside the invalid "alg" ("ES256"), and
update getJWKString_withInvalidUse_thenFail to include a valid "alg" (e.g.,
"RS256") alongside the invalid "use" ("enc"), so the failure is triggered by
invalid value checks in IdentityProviderUtil.getJWKString.

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java

Signed-off-by: Md-Humair-KK <[email protected]>

public key validations

e6e579b

Signed-off-by: Md-Humair-KK <[email protected]>

coderabbitai bot reviewed Jan 13, 2026

View reviewed changes

added new test cases

e8cfe0e

Signed-off-by: Md-Humair-KK <[email protected]>

coderabbitai bot reviewed Jan 13, 2026

View reviewed changes

sacrana0 reviewed Jan 14, 2026

View reviewed changes

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java Outdated Show resolved Hide resolved

sacrana0 reviewed Jan 14, 2026

View reviewed changes

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java Outdated Show resolved Hide resolved

Md-Humair-KK added 3 commits January 14, 2026 16:12

resolved review comments

4fb96fb

Signed-off-by: Md-Humair-KK <[email protected]>

public key validations

4aa6172

Signed-off-by: Md-Humair-KK <[email protected]>

removed exponenential validations to only AQAB

4a10b89

Signed-off-by: Md-Humair-KK <[email protected]>

sacrana0 approved these changes Jan 16, 2026

View reviewed changes

ase-101 reviewed Jan 19, 2026

View reviewed changes

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java Outdated Show resolved Hide resolved

Md-Humair-KK added 2 commits January 20, 2026 16:30

added test cases and removed alg and use validations

5d8773e

Signed-off-by: Md-Humair-KK <[email protected]>

Merge branch 'develop' of https://github.com/mosip/esignet into ES-2673

5aaf3ba

coderabbitai bot reviewed Jan 20, 2026

View reviewed changes

esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java Show resolved Hide resolved

esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java Show resolved Hide resolved

ase-101 approved these changes Jan 23, 2026

View reviewed changes

Md-Humair-KK added 3 commits January 23, 2026 12:35

test cases failures fixed

e02b447

Signed-off-by: Md-Humair-KK <[email protected]>

fixed test case failures

c0675ee

Signed-off-by: Md-Humair-KK <[email protected]>

Merge branch 'develop' of https://github.com/mosip/esignet into ES-2673

636607d

ase-101 merged commit f9d9164 into mosip:develop Jan 23, 2026
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES-2673] [ES-2655] Public key validations #1606

[ES-2673] [ES-2655] Public key validations #1606

Md-Humair-KK commented Jan 13, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 13, 2026 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

sacrana0 left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[ES-2673] [ES-2655] Public key validations #1606

[ES-2673] [ES-2655] Public key validations #1606

Conversation

Md-Humair-KK commented Jan 13, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sacrana0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Md-Humair-KK commented Jan 13, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 13, 2026 •

edited

Loading