MOSIP-43972: make allowed sign algorithm as configurable #506
Conversation
Signed-off-by: nagendra0721 <[email protected]>
WalkthroughReplaces a hard-coded signature-algorithm check with a configurable allowed algorithms list; removes an unused HASH_SHA2 constant; adds a CWTId field to the sign request DTO and logic to use/auto-generate CWT IDs (supporting "UUID" to request a random UUID); adds a RANDOM_UUID constant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (5)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureConstant.java (1)
137-138: Clarify sentinel value used for random CWT UUID generationUsing
RANDOM_UUID = "UUID"as a magic trigger works but is quite generic; a legitimate CWT ID of"UUID"would now be impossible to use literally. Consider either:
- Using a less collision‑prone sentinel (e.g.
"__RANDOM_UUID__"), or- Clearly documenting in the API contract that
"UUID"is reserved to mean “generate a random UUID”.This is non‑blocking but will reduce surprises for integrators.
kernel/kernel-keymanager-service/src/main/resources/application-local.properties (1)
135-137: Make ops expectations for allowed sign algorithms explicitThe new property is good, and the values look correct for common JCA names. Since validation does a plain
equalsIgnoreCasecheck againstX509Certificate.getSigAlgName(), it would help operators if the comment clarified that:
- Each entry must match
X509Certificate.getSigAlgName()exactly (case‑insensitive).- Values are comma‑separated (no extra commas / empty entries).
You might tweak the comment along the lines of:
-# Partner certificate allowed sign algorithms -mosip.kernel.partner.certificate.allowed.sign.algorithms=SHA256withRSA,SHA384withRSA,SHA512withRSA,SHA256withECDSA,SHA384withECDSA,SHA512withECDSA,Ed25519 +# Partner certificate allowed sign algorithms (must match X509Certificate.getSigAlgName(), comma-separated) +mosip.kernel.partner.certificate.allowed.sign.algorithms=SHA256withRSA,SHA384withRSA,SHA512withRSA,SHA256withECDSA,SHA384withECDSA,SHA512withECDSA,Ed25519Also double‑check non‑local profiles define this consistently if they need algorithms beyond the default.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (1)
470-474: CWT ID handling looks correct; consider trimming before sentinel checkThe new logic:
- Defaults
cwtIdtoBLANKwhen null,- Generates a random UUID when
cwtId.equalsIgnoreCase(RANDOM_UUID),- Only sets
ctiwhencwtUniqueIdis non‑blank,is consistent and avoids emitting empty
cticlaims.Two small optional improvements:
- Make the sentinel comparison tolerant to surrounding whitespace, in line with
isDataValid:-String cwtId = requestDto.getCWTId() != null ? requestDto.getCWTId() : SignatureConstant.BLANK; -String cwtUniqueId = (cwtId.equalsIgnoreCase(SignatureConstant.RANDOM_UUID)) +String cwtId = requestDto.getCWTId() != null ? requestDto.getCWTId().trim() : SignatureConstant.BLANK; +String cwtUniqueId = (cwtId.equalsIgnoreCase(SignatureConstant.RANDOM_UUID)) ? UUID.randomUUID().toString() : cwtId;
- Ensure the API docs for
CWTIdmention that the special value"UUID"triggers random UUID generation, so callers understand this behavior.Also applies to: 491-492
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/partnercertservice/service/impl/PartnerCertificateManagerServiceImpl.java (1)
128-129: Config‑driven algorithm check is good; add minimal safety around the listMoving to
allowedSignAlgorithmsis a nice improvement over the previous hard‑coded check.A couple of non‑blocking suggestions:
- Guard against empty/misconfigured config
If the property is accidentally set to an empty value, all partner cert uploads will start failing at runtime. You could fail fast or at least log clearly on startup, e.g.:
@PostConstruct private void validateAllowedSignAlgorithms() { if (allowedSignAlgorithms == null || allowedSignAlgorithms.isEmpty()) { LOGGER.error(PartnerCertManagerConstants.SESSIONID, PartnerCertManagerConstants.UPLOAD_PARTNER_CERT, PartnerCertManagerConstants.EMPTY, "No allowed certificate signature algorithms configured"); // optionally: throw a PartnerCertManagerException or fall back to a default list } }
- Normalize values once for cheaper lookups (optional)
If this check runs frequently, you might normalize
allowedSignAlgorithmsinto a lower‑caseSet<String>once and compare on lower‑casesignatureAlgorithm, instead of streaming each time. Not required for correctness, just a micro‑optimization.Overall, the comparison
allowedSignAlgorithms.stream().noneMatch(signatureAlgorithm::equalsIgnoreCase)is fine as‑is.Also applies to: 642-649
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/dto/CWTSignRequestDto.java (1)
96-100: Consider naming and document the special “UUID” behavior for CWTIdFunctionally this field works and matches the
getCWTId()usage inSignatureUtil.Two small improvements to consider while the API is still new:
- Field name style
CWTIdwill serialize as JSON property"CWTId". If you prefer conventional camelCase JSON ("cwtId"), you could rename the field tocwtIdand, if needed, use@JsonProperty("cwtId")to pin the wire name. Not mandatory, but more idiomatic.
- Surface the “UUID” sentinel in the contract
Since
SignatureUtiltreats the value"UUID"as “generate a random UUID”, it would be helpful to mention this explicitly in the Swagger annotation, e.g.:@ApiModelProperty( notes = "CWT Id (use value 'UUID' to auto-generate a random UUID)", example = "123", required = false ) private String CWTId;This makes the feature discoverable for clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/partnercertservice/constant/PartnerCertManagerConstants.java(0 hunks)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/partnercertservice/service/impl/PartnerCertificateManagerServiceImpl.java(2 hunks)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureConstant.java(1 hunks)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/dto/CWTSignRequestDto.java(1 hunks)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java(2 hunks)kernel/kernel-keymanager-service/src/main/resources/application-local.properties(1 hunks)
💤 Files with no reviewable changes (1)
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/partnercertservice/constant/PartnerCertManagerConstants.java
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureConstant.java (1)
SignatureConstant(10-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-kernel / maven-build
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.