MOSIP-36428: ecc encryption support #514
Conversation
Signed-off-by: nagendra0721 <[email protected]>
WalkthroughAdds ECC and X25519 support across the keymanager: new EC constants and error code, EcCryptomanagerService interface and implementation, ECC-aware encrypt/decrypt flows, key generation for EC/X25519/Ed25519, signature algorithm resolution by certificate, and a new RSA sign key API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CryptomanagerServiceImpl
participant CryptomanagerUtils
participant EcCryptomanagerServiceImpl
participant KeyAgreement
participant HKDF
participant AES-GCM
Client->>CryptomanagerServiceImpl: encrypt(publicKey, data)
CryptomanagerServiceImpl->>CryptomanagerUtils: getAlgorithmNameFromHeader()/resolve alg
alt RSA
CryptomanagerServiceImpl->>CryptomanagerServiceImpl: RSA envelope encrypt path
else ECC/X25519
CryptomanagerServiceImpl->>EcCryptomanagerServiceImpl: asymmetricEcEncrypt(publicKey, data, ecCurveName)
EcCryptomanagerServiceImpl->>EcCryptomanagerServiceImpl: generate ephemeral key pair
EcCryptomanagerServiceImpl->>KeyAgreement: derive shared secret (ephemeralPriv, recipientPub)
KeyAgreement-->>EcCryptomanagerServiceImpl: sharedSecret
EcCryptomanagerServiceImpl->>HKDF: derive AES key(sharedSecret, IV, info)
HKDF-->>EcCryptomanagerServiceImpl: aesKey
EcCryptomanagerServiceImpl->>AES-GCM: encrypt(aesKey, data, AAD)
AES-GCM-->>EcCryptomanagerServiceImpl: ciphertext
EcCryptomanagerServiceImpl-->>CryptomanagerServiceImpl: packaged ciphertext + IV + ephemeralPub + header
end
CryptomanagerServiceImpl-->>Client: encrypted payload
sequenceDiagram
participant Client
participant CryptomanagerServiceImpl
participant CryptomanagerUtils
participant KeyStore
participant EcCryptomanagerServiceImpl
participant KeyAgreement
participant HKDF
participant AES-GCM
Client->>CryptomanagerServiceImpl: decrypt(encryptedPayload)
CryptomanagerServiceImpl->>CryptomanagerUtils: getAlgorithmNameFromHeader(payload)
alt RSA
CryptomanagerServiceImpl->>CryptomanagerServiceImpl: RSA decrypt path (unwrap session key, AES-GCM decrypt)
else ECC/X25519
CryptomanagerServiceImpl->>CryptomanagerUtils: getEncryptedPrivateKey(certThumbprint)
CryptomanagerUtils->>KeyStore: retrieve private key and cert
CryptomanagerServiceImpl->>EcCryptomanagerServiceImpl: asymmetricEcDecrypt(privateKey, payload, aad, curveName)
EcCryptomanagerServiceImpl->>EcCryptomanagerServiceImpl: extract ephemeralPub, IV, ciphertext
EcCryptomanagerServiceImpl->>KeyAgreement: derive shared secret(privateKey, ephemeralPub)
KeyAgreement-->>EcCryptomanagerServiceImpl: sharedSecret
EcCryptomanagerServiceImpl->>HKDF: derive AES key(sharedSecret, IV, info)
HKDF-->>EcCryptomanagerServiceImpl: aesKey
EcCryptomanagerServiceImpl->>AES-GCM: decrypt(aesKey, ciphertext, AAD)
AES-GCM-->>EcCryptomanagerServiceImpl: plaintext
EcCryptomanagerServiceImpl-->>CryptomanagerServiceImpl: plaintext
end
CryptomanagerServiceImpl-->>Client: plaintext
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java (2)
487-488: Bug: Condition will cause NullPointerException.The condition
altValuesMap == null && altValuesMap.isEmpty()will throw NPE whenaltValuesMapis null becauseisEmpty()is called on a null reference. The operator should be||(OR) to short-circuit when null.🐛 Proposed fix
- if (altValuesMap == null && altValuesMap.isEmpty()) { + if (altValuesMap == null || altValuesMap.isEmpty()) {
555-556: Bug: Same NullPointerException issue as line 487.This condition has the same bug with
&&instead of||.🐛 Proposed fix
- if (altValuesMap == null && altValuesMap.isEmpty()) { + if (altValuesMap == null || altValuesMap.isEmpty()) {
🤖 Fix all issues with AI agents
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java`:
- Line 244: Remove the System.out.println debug output in
EcCryptomanagerServiceImpl; replace it with the class logger (e.g.,
LOGGER.debug) or remove it entirely: locate the statement printing "Number of
iterations: " that uses variables i and bytegenerated and either delete it or
wrap the same message in LOGGER.debug(...) (optionally guarded with
LOGGER.isDebugEnabled()) so no direct System.out calls remain in production
code.
- Around line 117-128: The finally block can NPE because ephemeralKeyPair may be
null and the public/private check is inverted; update the checks so you first
verify ephemeralKeyPair != null before accessing its getters and destroy the
correct key types, e.g. keep the existing if (ephemeralKeyPair != null) call to
destroyKey(ephemeralKeyPair.getPrivate().getEncoded()) only when getPrivate() !=
null, and replace the final line with a guarded check that destroys the public
key: if (ephemeralKeyPair != null && ephemeralKeyPair.getPublic() != null)
destroyKey(ephemeralKeyPair.getPublic().getEncoded()); ensure similar null
guards for aesKey and ephemeralPublicKey; reference symbols: ephemeralKeyPair,
ephemeralPublicKey, aesKey, destroyKey, generateAlgorithmBasedEphemeralKeyPair.
- Around line 132-182: The decrypt method asymmetricEcDecrypt must zero out
sensitive key material after use: add a finally block that checks and securely
wipes the byte[] variables sharedSecret and aesKeyBytes (e.g.,
Arrays.fill(sharedSecret, (byte)0); Arrays.fill(aesKeyBytes, (byte)0)) and nulls
sensitive references like aesKey and keyAgreement; also wipe iv and any
temporary cipherText buffers if present, to ensure no sensitive data remains in
memory after decryption.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java`:
- Around line 104-114: Remove the duplicate field signApplicationid and
consolidate configuration to the existing signApplicationId and signRefId names:
keep the `@Value` annotations as needed but rename/remove certificateSignRefID to
signRefId (or vice versa) so only signApplicationId and signRefId exist; update
all usages that currently reference signApplicationid and certificateSignRefID
(e.g., in methods that build signing context or calls around
signApplicationId/signRefId) to use the consolidated signApplicationId and
signRefId fields, and remove the redundant field declaration to avoid confusion.
- Around line 454-461: The code calls refId.get() before checking presence which
can throw NoSuchElementException; modify CryptomanagerUtils to avoid unguarded
get(): either first check refId.isPresent() and only call refId.get().trim()
inside that branch, or replace uses with refId.map(String::trim).orElse("") (or
refId.orElse("").trim()) and then test if the resulting string is empty; update
the block that currently reads refId.get().trim() and the subsequent if
(!refId.isPresent() || refId.get().trim().isEmpty()) to use the safe value so
the fallback to HSM (keyStore.getAsymmetricKey(ksAlias) and the return new
Object[] {masterPrivateKey, masterCert}) only executes when the refId is
actually absent/blank.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/KeyGenerator.java`:
- Around line 126-134: The getECKeyPair method currently calls
KeyGeneratorUtils.getECKeyPairGenerator(asymmetricKeyAlgorithm, eccCurve,
getSecureRandom()) but asymmetricKeyAlgorithm is RSA-configured and causes
InvalidAlgorithmParameterException; add a new ecKeyAlgorithm field/property
defaulting to "EC" (mirroring PKCS12KeyStoreImpl/PKCS11KeyStoreImpl patterns)
and change getECKeyPair to pass ecKeyAlgorithm instead of asymmetricKeyAlgorithm
to KeyGeneratorUtils.getECKeyPairGenerator so an EC KeyPairGenerator is created
and initialized with ECGenParameterSpec.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java`:
- Around line 621-630: The code treats X25519 as a signature algorithm which is
invalid; update the flow so X25519 keys are never used to sign X.509
certificates: either in CertificateUtility.getSignatureAlgorithm() return a
valid signer for signing keys and do not return "X25519", or in
CertificateUtility.generateX509Certificate() detect
io.mosip.kernel.keymanagerservice.constant.KeymanagerConstant.X25519_KEY_TYPE
(and related callers like generateAndStoreAsymmetricKey and
generateX25519KeyPair) and reject/throw a clear exception when asked to use
X25519 as a signing key, or map certificate signing to a proper algorithm (e.g.,
ED25519/RSA) while allowing X25519 only as a subject public key for key
agreement; ensure the error path uses the X25519_KEY_TYPE constant and the
methods CertificateUtility.getSignatureAlgorithm(),
CertificateUtility.generateX509Certificate(), generateAndStoreAsymmetricKey(),
and generateX25519KeyPair for locating where to change behavior.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java`:
- Around line 290-303: getSignatureAlgorithm currently returns
KeymanagerConstant.X25519_KEY_TYPE for X25519 private keys which is invalid for
signing and will cause JcaContentSignerBuilder to fail; change
getSignatureAlgorithm (and any callers expecting a signature algorithm) to
detect KeymanagerConstant.X25519_KEY_TYPE and throw a clear
UnsupportedOperationException or IllegalArgumentException stating X25519 is not
a signing algorithm (or require a separate signing key), instead of returning a
bogus algorithm string, so callers (e.g., code that constructs a
JcaContentSignerBuilder) can handle the unsupported key type appropriately.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/helper/SessionKeyDecrytorHelper.java`:
- Around line 378-384: The code in SessionKeyDecrytorHelper currently builds the
KeyFactory using masterPrivateKey.getAlgorithm(), which is incorrect; change it
to extract the algorithm from the base key's certificate: call
keymanagerUtil.convertToCertificate(dbKeyStore.get().getCertificateData()).getPublicKey().getAlgorithm()
and use that algorithm in KeyFactory.getInstance(...), then proceed to generate
the PrivateKey with new PKCS8EncodedKeySpec(decryptedPrivateKey) and return the
private key and certificate as before.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java`:
- Around line 245-267: The file KeymanagerServiceImpl contains a large
commented-out block (the old conditional handling around referenceId,
ecRefIdsAlgoNamesMap and generateEd25519KeyPairDetails) that is now obsolete
because the new logic using keyStore.generateAndStoreAsymmetricKey(alias,
rootKeyAlias, certParams[, eccCurve]) replaces it; remove the entire commented
block to avoid clutter: delete the multi-line comment that references
KeyReferenceIdConsts, ecRefIdsAlgoNamesMap, ed25519SupportFlag and
generateEd25519KeyPairDetails so only the current if/else implementation remains
in KeymanagerServiceImpl.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java`:
- Around line 805-827: getEcCurveName currently assumes
SubjectPublicKeyInfo.getAlgorithm().getParameters() is non-null and will NPE
when parameters are missing; update getEcCurveName to defensively check that
subjectPublicKeyInfo.getAlgorithm() and its getParameters() are not null before
casting to ASN1ObjectIdentifier (or when oid is null), and if missing throw the
same io.mosip.kernel.core.exception.NoSuchAlgorithmException (or a new
descriptive NoSuchAlgorithmException using
KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE) so malformed keys produce a
controlled exception instead of an NPE; reference SubjectPublicKeyInfo,
getAlgorithm(), getParameters(), ASN1ObjectIdentifier oid and getEcCurveName
when making the change.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java`:
- Line 413: The ECC branch uses a misspelled variable name secreteDataBytes;
rename it to secretDataBytes to match the RSA branch and any subsequent usages.
Update the declaration/assignment where ecCrypto.asymmetricEcDecrypt(...) is
called (and any references to secreteDataBytes) so the variable consistently
uses secretDataBytes throughout KeyMigratorServiceImpl (e.g., in the ECC branch
around the ecCrypto.asymmetricEcDecrypt call and later processing of the
decrypted data).
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java`:
- Around line 676-685: The fallback for determining the signing algorithm in
signv2() is inconsistent with jwsSign(); replace the current fallback that uses
certificateResponse.getCertificateEntry().getPrivateKey().getAlgorithm() with
SignatureUtil.getJwtSignAlgorithm(certificateResponse.getCertificateEntry()) so
both signv2() and jwsSign() derive the JWT-style algorithm consistently; ensure
SignatureProviderEnum lookups remain compatible and update any inline comment to
state that JWT-style algorithm names (e.g., RS256/ES256/EdDSA) are used as the
canonical fallback.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java`:
- Around line 603-616: The getJwtSignAlgorithm method assumes
SubjectPublicKeyInfo.getAlgorithm().getParameters() is an ASN1ObjectIdentifier
and casts it directly, which can throw ClassCastException for keys with implicit
parameters or other encodings; update getJwtSignAlgorithm to first check the
parameters' actual type (e.g., instanceof ASN1ObjectIdentifier) before casting,
handle alternative types (e.g., null or ASN1Sequence) by falling back to a safe
default or using a different extraction strategy, and ensure any unexpected
types/exceptions are caught so the method returns a valid AlgorithmIdentifiers
constant rather than propagating the exception.
In
`@kernel/keys-migrator/src/main/java/io/mosip/kernel/migrate/impl/BaseKeysMigrator.java`:
- Around line 360-362: decryptRandomKey can return null on exception; add an
explicit null check immediately after the decryptedZKKey assignment in the block
that computes encryptedRandomKey (after the call to decryptRandomKey) and before
calling cryptoCore.asymmetricEncrypt or ecCrypto.asymmetricEcEncrypt; if
decryptedZKKey is null, log an error including key id/context (use the existing
logger variable) and continue/skip this key instead of calling
asymmetricEcEncrypt/asymmetricEncrypt, otherwise proceed to call
cryptoCore.asymmetricEncrypt for RSA and
ecCrypto.asymmetricEcEncrypt(zkPublicKey, decryptedZKKey,
keymanagerUtil.getEcCurveName(zkPublicKey)) for non‑RSA.
🧹 Nitpick comments (15)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java (1)
397-424: ECB mode cipher usage and code duplication.
ECB Mode: Static analysis correctly flags that ECB mode doesn't provide semantic security for multi-block data. However, for wrapping a single AES key (typically 16-32 bytes = 1-2 blocks), ECB is acceptable since the key material is random. Verify this is the intended use case.
Code duplication: The RSA and ECC branches share identical post-decryption logic (cipher initialization and encryption). Consider extracting the common code.
♻️ Suggested refactor to reduce duplication
private byte[] encryptRandomKey(byte[] encryptedKeyBytes, Key zkMasterKey, PrivateKey tempPrivateKey, PublicKey tempPublicKey) { + byte[] secretDataBytes = null; if (tempPublicKey.getAlgorithm().equalsIgnoreCase(KeymanagerConstant.RSA)) { try { - byte[] secretDataBytes = cryptoCore.asymmetricDecrypt(tempPrivateKey, tempPublicKey, encryptedKeyBytes); - Cipher cipher = Cipher.getInstance(aesECBTransformation); - - cipher.init(Cipher.ENCRYPT_MODE, zkMasterKey); - return cipher.doFinal(secretDataBytes, 0, secretDataBytes.length); + secretDataBytes = cryptoCore.asymmetricDecrypt(tempPrivateKey, tempPublicKey, encryptedKeyBytes); } catch (NoSuchAlgorithmException | InvalidKeyException | NoSuchPaddingException | IllegalBlockSizeException | BadPaddingException | IllegalArgumentException | InvalidDataException | io.mosip.kernel.core.crypto.exception.InvalidKeyException e) { LOGGER.error(KeyMigratorConstants.SESSIONID, KeyMigratorConstants.ZK_KEYS, KeyMigratorConstants.EMPTY, "Error in encrypting random Key in key migration process.", e); + return null; } } else { try { - byte[] secreteDataBytes = ecCrypto.asymmetricEcDecrypt(tempPrivateKey, encryptedKeyBytes, null, keymanagerUtil.getEcCurveName(tempPublicKey)); - Cipher cipher = Cipher.getInstance(aesECBTransformation); - cipher.init(Cipher.ENCRYPT_MODE, zkMasterKey); - return cipher.doFinal(secreteDataBytes, 0, secreteDataBytes.length); + secretDataBytes = ecCrypto.asymmetricEcDecrypt(tempPrivateKey, encryptedKeyBytes, null, keymanagerUtil.getEcCurveName(tempPublicKey)); } catch (NoSuchAlgorithmException | InvalidKeyException | NoSuchPaddingException | IllegalBlockSizeException | BadPaddingException | IllegalArgumentException | InvalidDataException | io.mosip.kernel.core.crypto.exception.InvalidKeyException e) { LOGGER.error(KeyMigratorConstants.SESSIONID, KeyMigratorConstants.ZK_KEYS, KeyMigratorConstants.EMPTY, "Error in encrypting random Key in key migration process.", e); + return null; } } + try { + Cipher cipher = Cipher.getInstance(aesECBTransformation); + cipher.init(Cipher.ENCRYPT_MODE, zkMasterKey); + return cipher.doFinal(secretDataBytes, 0, secretDataBytes.length); + } catch (NoSuchAlgorithmException | InvalidKeyException | NoSuchPaddingException + | IllegalBlockSizeException | BadPaddingException e) { + LOGGER.error(KeyMigratorConstants.SESSIONID, KeyMigratorConstants.ZK_KEYS, + KeyMigratorConstants.EMPTY, "Error in encrypting random Key in key migration process.", e); + } return null; }kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureProviderEnum.java (1)
18-21: Verify the intentional mixing of JWS algorithm names and key type identifiers.The existing enum constants (PS256, RS256, ES256, ES256K, EDDSA) use JWS algorithm name constants from
SignatureConstant, while the new constants (ECDSA, ED25519, RSA) use key type identifiers fromKeymanagerConstant. This creates two lookup paths for similar providers (e.g., ES256 vs ECDSA both map toEC256SignatureProviderImpl).Please confirm this dual-lookup approach is intentional for supporting both JWS algorithm-based and key-type-based provider resolution.
Also, the trailing comma on line 21 (
RSA(KeymanagerConstant.RSA, new RS256SignatureProviderImpl()),;) is syntactically valid but unusual—consider removing it for consistency.🧹 Remove trailing comma
- RSA(KeymanagerConstant.RSA, new RS256SignatureProviderImpl()),; + RSA(KeymanagerConstant.RSA, new RS256SignatureProviderImpl());kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/KeymanagerService.java (1)
148-156: LGTM - minor indentation inconsistency.The new
generateRSASignKeymethod appropriately mirrors the existinggenerateECSignKeymethod pattern, providing a symmetric API for RSA signature key generation. The Javadoc is clear and consistent.Note: Lines 149-156 use space indentation while the rest of the file uses tab indentation. Consider aligning with the file's existing style for consistency.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerErrorCode.java (1)
70-71: LGTM!The new
UNSUPPORTED_EC_CURVEerror code follows the established numbering sequence (KER-CRY-016) and provides a clear, actionable error message for ECC curve validation failures.Minor: Lines 70-71 use space indentation while the rest of the file uses tabs. Consider aligning with the file's existing style.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
69-74: Consider consolidating duplicate RSA signature algorithm constants.
RSA_SIGN_ALGORITHMat line 69 duplicatesSIGNATURE_ALGORITHMat line 25 — both are"SHA256withRSA". Consider using one constant to avoid confusion and maintain consistency.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (2)
105-110: UnusedsignAlgorithmparameter in this code path.The
signAlgorithmparameter passed at line 91 is ignored whenExtendedCertificateParametersis used (line 105) and in the else branch (line 108). Meanwhile, other overloaded methods (lines 158-170, 172-189) still use the parameter directly. This inconsistency may confuse callers.Consider either removing the unused parameter from this method signature or documenting that the algorithm is derived dynamically for certain certificate types.
113-132: Same issue:signAlgorithmparameter passed but unused.This private method receives
signAlgorithmat line 114 but line 119 derives the algorithm from the private key instead. This compounds the inconsistency noted above.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java (1)
340-357: Variable shadowing: localecCurveNameshadows class field.At Line 343, the local variable
ecCurveNameshadows the class field of the same name (defined at Line 125). While the local assignment fromalgorithmNameappears intentional here (to use the algorithm derived from the header), this shadowing can cause confusion during maintenance. Consider renaming the local variable to clarify its purpose.♻️ Suggested rename for clarity
} else { LOGGER.info(CryptomanagerConstant.SESSIONID, CryptomanagerConstant.DECRYPT, KeymanagerConstant.EC_KEY_TYPE, "Decrytping the data with EC Key."); - String ecCurveName = algorithmName; + String curveName = algorithmName; byte[] thumbprint = copyOfRange(encryptedHybridData, keyDemiliterIndex + keySplitter.length(), keyDemiliterIndex + keySplitter.length() + CryptomanagerConstant.THUMBPRINT_LENGTH); byte[] encryptedDataWithIv = copyOfRange(encryptedHybridData, keyDemiliterIndex + keySplitter.length() + CryptomanagerConstant.THUMBPRINT_LENGTH, encryptedHybridData.length); String certThumbprintHex = Hex.toHexString(thumbprint).toUpperCase(); PrivateKey privateKey = (PrivateKey) cryptomanagerUtil.getEncryptedPrivateKey(cryptoRequestDto.getApplicationId(), Optional.ofNullable(cryptoRequestDto.getReferenceId()), certThumbprintHex)[0]; byte[] aad = Arrays.copyOfRange(encryptedDataWithIv, 0, CryptomanagerConstant.GCM_AAD_LENGTH); byte[] encryptedData = Arrays.copyOfRange(encryptedDataWithIv, CryptomanagerConstant.GCM_AAD_LENGTH,encryptedDataWithIv.length); - byte[] decryptedData = ecCryptomanagerService.asymmetricEcDecrypt(privateKey, encryptedData, aad, ecCurveName); + byte[] decryptedData = ecCryptomanagerService.asymmetricEcDecrypt(privateKey, encryptedData, aad, curveName); cryptoResponseDto.setData(CryptoUtil.encodeToURLSafeBase64(decryptedData)); }kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java (1)
204-206: Unused configuration property.The
ecCurveNamefield is declared but never referenced anywhere in the code. If this property is intended for future use, consider removing it until needed; otherwise, integrate it into the EC curve handling logic.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureAlgorithmIdentifyEnum.java (1)
4-4: Unused import.
ECCurvesis imported but not referenced anywhere in this file.♻️ Proposed fix
-import io.mosip.kernel.keymanagerservice.constant.ECCurves;kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java (3)
123-125: Duplicate configuration field.
certificateSignRefIDhas the same default value assignRefid(Line 106). Consider reusingsignRefidinstead of introducing a duplicate field, or clarify if these are intended to diverge in configuration.
586-597: Remove commented-out code.The commented-out lines 586-588 should be removed as the new implementation on lines 593-596 replaces them. Dead code hinders maintainability.
♻️ Proposed fix
-// String signAlgorithm = (jwsSignRequestDto.getSignAlgorithm() == null || jwsSignRequestDto.getSignAlgorithm().isBlank()) ? -// SignatureUtil.getSignAlgorithm(referenceId) : jwsSignRequestDto.getSignAlgorithm(); - SignatureCertificate certificateResponse = keymanagerService.getSignatureCertificate(applicationId, Optional.of(referenceId), timestamp);
976-985: Remove commented-out code in jwsSignV2.Similar to
jwsSign(), the commented-out code (lines 976-977) should be removed.♻️ Proposed fix
-// String signAlgorithm = (jwsSignRequestDto.getSignAlgorithm() == null || jwsSignRequestDto.getSignAlgorithm().isBlank()) ? -// SignatureUtil.getSignAlgorithm(referenceId) : jwsSignRequestDto.getSignAlgorithm(); - SignatureCertificate certificateResponse = keymanagerService.getSignatureCertificate(applicationId,kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java (1)
386-407: Complex branching for key pair generation.The nested conditionals for Ed25519/X25519/ECC key generation are difficult to follow. Consider extracting this into a helper method that returns the appropriate
KeyPairbased on algorithm configuration.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java (1)
280-285: Public method should be package-private or private.
getKeyAgreementAlorithmBasedis declaredpublicbut appears to be an internal helper. Consider reducing visibility unless external access is required.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerConstant.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerErrorCode.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/EcCryptomanagerService.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/KeyGenerator.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/util/KeyGeneratorUtils.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/KeyStoreImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeyReferenceIdConsts.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerErrorConstant.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/controller/KeymanagerController.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/helper/PrivateKeyDecryptorHelper.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/helper/SessionKeyDecrytorHelper.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/KeymanagerService.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureAlgorithmIdentifyEnum.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureProviderEnum.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/CoseSignatureServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.javakernel/keys-migrator/src/main/java/io/mosip/kernel/migrate/impl/BaseKeysMigrator.java
🧰 Additional context used
🧬 Code graph analysis (10)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)
kernel/keys-migrator/src/main/java/io/mosip/kernel/migrate/impl/BaseKeysMigrator.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/util/KeyGeneratorUtils.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerConstant.java (1)
CryptomanagerConstant(10-78)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureAlgorithmIdentifyEnum.java (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java (3)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/exception/NoUniqueAliasException.java (1)
NoUniqueAliasException(12-29)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerConstant.java (1)
CryptomanagerConstant(10-78)
🪛 ast-grep (0.40.5)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java
[warning] 400-400: Cipher in ECB mode is detected. ECB mode produces the same output for the same input each time which allows an attacker to intercept and replay the data. Further, ECB mode does not provide any integrity checking. See https://find-sec-bugs.github.io/bugs.htm#CIPHER_INTEGRITY.
Context: Cipher cipher = Cipher.getInstance(aesECBTransformation);
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(ecb-cipher-java)
[warning] 413-413: Cipher in ECB mode is detected. ECB mode produces the same output for the same input each time which allows an attacker to intercept and replay the data. Further, ECB mode does not provide any integrity checking. See https://find-sec-bugs.github.io/bugs.htm#CIPHER_INTEGRITY.
Context: Cipher cipher = Cipher.getInstance(aesECBTransformation);
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(ecb-cipher-java)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java
[warning] 221-221: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(HMAC_SHA_256)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 267-267: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyPairGenerator.getInstance(EC_ALGORITHM, BC_PROVIDER)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 221-221: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(HMAC_SHA_256)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 267-267: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: KeyPairGenerator.getInstance(EC_ALGORITHM, BC_PROVIDER)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ 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
🔇 Additional comments (46)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java (3)
92-97: LGTM - Configuration properties for ECC support.The new configuration properties
keyAlgorithmandecCurveNamealign with the broader ECC support infrastructure. Default values (RSAandSECP256R1) provide backward compatibility.
129-130: LGTM - EcCryptomanagerService injection.The service is properly autowired for ECC-based decryption operations in the migration flow.
297-302: LGTM - Conditional key generation based on algorithm.The branching logic correctly handles RSA vs ECC key generation paths using the appropriate
generateAndStoreAsymmetricKeyoverloads.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/EcCryptomanagerService.java (1)
6-42: LGTM - Well-designed interface for EC cryptographic operations.The interface provides a clean abstraction for EC-based encryption/decryption with:
- Two encryption overloads (with IV/AAD and with curveName only)
- One decryption method supporting AAD
- Comprehensive Javadoc documentation
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerErrorConstant.java (1)
84-85: LGTM - New error constant for X25519 CSR limitation.The error constant follows the existing naming convention and sequential error code pattern. The message clearly indicates that CSR generation is not supported for X25519 algorithm.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/helper/PrivateKeyDecryptorHelper.java (1)
121-127: LGTM - Consistent algorithm detection for key reconstruction.This change mirrors the update in
SessionKeyDecrytorHelper, ensuring both helpers use the master key's algorithm for reconstructing decrypted private keys. This consistency is important for proper ECC support across the codebase.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/KeyStoreImpl.java (2)
120-125: LGTM!The Ed25519 algorithm configuration follows the established pattern used for EC algorithm (lines 117-118). The property naming convention and default value are appropriate for EdDSA key operations.
184-184: LGTM!Correctly propagates the Ed25519 algorithm configuration to the keystore parameters map, consistent with how other algorithm properties are handled.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeyReferenceIdConsts.java (1)
7-8: LGTM!The
RSA_2048_SIGNconstant follows the established naming convention ({ALGORITHM}_{PARAMS}_SIGN) and aligns with the PR objective of adding RSA signature key generation support.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/CoseSignatureServiceImpl.java (1)
140-144: LGTM - Algorithm resolution logic is sound.The dynamic algorithm detection correctly prioritizes an explicitly provided algorithm, falling back to deriving it from the certificate chain when not specified. This aligns well with the ECC support additions in the PR.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (1)
127-127: LGTM - Provider-aware certificate conversion.Adding
.setProvider(providerName)ensures the certificate is created using the same provider as the content signer, which is important for HSM compatibility.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/KeyGenerator.java (2)
70-73: LGTM - ECC curve configuration.Good addition with a sensible default (
SECP256R1/ P-256). The externalized configuration allows flexibility for different deployments.
136-144: LGTM - X25519 key pair generation.Clean implementation following the established pattern. The delegation to
KeyGeneratorUtils.getX25519KeyPairGeneratorkeeps the key generation logic centralized.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java (3)
471-472: X25519 key type added for key pair generation.The logic correctly routes X25519 key type to the new generator method. The use of the fully qualified constant (
io.mosip.kernel.keymanagerservice.constant.KeymanagerConstant.X25519_KEY_TYPE) is verbose but avoids ambiguity with the HSM constant class.
247-261: KeyStore instantiation now explicitly binds the provider.Using
KeyStore.getInstance(keystoreType, provider)ensures consistent use of the BouncyCastle provider for PKCS12 keystore operations. This is a good change for provider consistency.
481-488: RSA key generation now uses explicit constant.The change from
asymmetricKeyAlgorithmtoKeymanagerConstant.RSA_KEY_TYPEmakes the RSA path explicit and prevents potential misconfiguration. This is a sound improvement.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (2)
627-633: Algorithm derivation logic is correct.The
getAlgorithmStringmethod appropriately checks if therefIdmatches a knownKeyReferenceIdConstsvalue before falling back to certificate-based algorithm derivation. The use ofArrays.streamwithanyMatchis clear and correct.
618-625: The EC curve support is intentionally limited to secp256r1 and secp256k1.The
mapCurveOidToCurveNamemethod correctly handles the only two EC curves the system supports. No other curves (secp384r1, secp521r1, etc.) are defined or used in the codebase. The exception for unsupported curves is the correct behavior.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/util/KeyGeneratorUtils.java (2)
138-149: EC key pair generator with configurable curve.The
getECKeyPairGeneratormethod provides flexibility by accepting the curve name as a parameter, unlike the existinggetEdKeyPairGeneratorwhich uses the algorithm name for both. This is a cleaner approach for EC key generation.
151-163: X25519 key pair generator implementation.The implementation correctly uses
NamedParameterSpecfor X25519, which is the appropriate spec class for XDH (X25519/X448) algorithms in Java 11+. The redundant initialization withNamedParameterSpecaftergetInstancemay not be strictly necessary for X25519 but ensures explicit curve selection.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java (3)
198-274: ECC encryption path added alongside RSA.The encrypt method now correctly branches based on the public key algorithm. The ECC path appropriately:
- Generates random AAD for GCM
- Maps Ed25519 to X25519 for encryption (since Ed25519 is for signing, X25519 for key exchange)
- Prepends AAD to encrypted data for later extraction during decryption
508-521: JWT encryption now supports ECDH-ES key management.The dynamic selection between
RSA_OAEP_256andECDH_ES_A256KWbased on the certificate's public key algorithm enables JWT encryption with EC keys. This is correct for JOSE/JWE interoperability.
259-261: Review comment verified: ECC encryption service correctly handles ED25519 to X25519 curve mapping.The
ecCryptomanagerService.asymmetricEcEncryptmethod correctly processes the X25519 algorithm name. BothgenerateAlgorithmBasedEphemeralKeyPair(lines 262-266) andgetKeyAgreementAlorithmBased(lines 280-285) explicitly check for X25519_KEY_TYPE and initialize the appropriate key agreement algorithm, ensuring ECDH key agreement works correctly with X25519.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/controller/KeymanagerController.java (1)
309-337: No review comment was provided to rewrite. Please provide the review comment content within<review_comment>tags so I can verify the concerns and generate a rewritten version.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java (3)
316-329: LGTM!The branching logic for RSA vs EC key encryption is clean. RSA path uses symmetric key wrapping, while EC path delegates directly to
ecCryptomanagerService.
342-357: LGTM!The decryption branching mirrors the encryption logic appropriately, handling RSA with symmetric key unwrapping and EC via the dedicated service.
618-625: LGTM!The CSR generation correctly handles X25519 alongside Ed25519 for content signer selection.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java (2)
141-145: LGTM!New dependencies for EC cryptography support are properly autowired.
403-407: LGTM!The ternary cleanly branches between RSA and EC encryption based on public key algorithm.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerConstant.java (1)
66-77: LGTM!The new EC curve version headers and curve name constants follow existing naming conventions and are properly documented with inline comments.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
251-262: LGTM!The new EC curve OIDs match standard values (SECP256R1:
1.2.840.10045.3.1.7, SECP256K1:1.3.132.0.10), and the X25519/XDH constants properly support the new key exchange algorithm paths.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureAlgorithmIdentifyEnum.java (1)
13-22: LGTM with note on duplicate algorithm mappings.The expanded enum correctly maps reference IDs to jose4j algorithm identifiers. Note that both
RSA(referenceId: "RSA") andRS256(referenceId: "RS256") map to the same algorithmAlgorithmIdentifiers.RSA_USING_SHA256, which appears intentional to support lookup by either reference ID format.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java (2)
334-336: Algorithm selection logic looks correct.The conditional properly derives the JWT signing algorithm from the certificate when
referenceIdis empty or matchescertificateSignRefID, otherwise falls back to the enum-based lookup.
894-896: Algorithm selection in signV2 is consistent with sign().This follows the same pattern as line 334-336, deriving from certificate or enum based on referenceId.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java (7)
126-133: New configuration fields for ECC support.The addition of
masterKeyAlgorithmandeccCurveconfiguration fields enables flexible key algorithm selection. Ensure these are documented in application properties templates.
427-435: X25519 master key handling for signing.When the master public key is X25519 (encryption-only), the code correctly fetches an Ed25519 certificate for signing operations. This handles the case where X25519 keys cannot be used for certificate signing.
697-705: Automatic X25519 encryption key generation.When generating an Ed25519 signing key pair, the code automatically generates a companion X25519 encryption key. This is a good pattern for systems that need both signing and encryption capabilities.
762-766: RSA_2048_SIGN handling added.New case for
RSA_2048_SIGNreference ID correctly generates RSA keys and stores them.
930-934: X25519 CSR generation blocked.Correctly throws an exception when attempting to generate CSR for X25519 keys, as X25519 is an encryption-only algorithm that cannot be used for certificate signing.
1011-1011: Dynamic key algorithm detection for private key reconstruction.Using
keyAlgorithmfrom the certificate's public key instead of hardcoded "RSA" enables proper reconstruction of EC/EdDSA private keys.
1474-1490: The validator actually supports RSA key generation.ECKeyPairGenRequestValidatorvalidates against allKeyReferenceIdConstsvalues, which includesRSA_2048_SIGN. BothgenerateECSignKey()andgenerateRSASignKey()correctly use the same validator since the validation logic is agnostic to key type (validates response type and allowed reference IDs regardless of algorithm).The confusing naming (class name, error code) should be addressed for clarity, but the current implementation is functionally correct.
Likely an incorrect or invalid review comment.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java (1)
534-549: getHeaderByte implementation is correct.Properly maps EC curve names to version header bytes with appropriate error handling for unsupported curves.
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java (2)
27-53: Well-structured ECC cryptography service.The service properly implements ephemeral ECDH key agreement with HKDF-derived AES-GCM encryption. Constants are appropriately defined.
Note: The static analysis warnings about DES/ECB are false positives -
Mac.getInstance(HMAC_SHA_256)andKeyPairGenerator.getInstance(EC_ALGORITHM)are not related to DES or ECB mode.
259-278: Ephemeral key pair generation is correct.Properly handles both X25519 and standard EC curves with appropriate parameter specs.
kernel/keys-migrator/src/main/java/io/mosip/kernel/migrate/impl/BaseKeysMigrator.java (2)
170-172: LGTM!The
EcCryptomanagerServiceautowiring follows the existing dependency injection pattern in this class and is correctly used for EC encryption support.
256-265: No action needed—encryptKey method already supports EC key encryption.The
keymanagerUtil.encryptKey()method at line 259 already handles EC keys properly. The implementation checks the algorithm of the master key (lines 320-327 of KeymanagerUtil.java) and routes to the appropriate encryption mechanism:
- RSA keys use symmetric key wrapping with asymmetric encryption
- Non-RSA keys (EC, Ed25519, etc.) use
ecCryptomanagerService.asymmetricEcEncrypt()This centralized approach is more robust than the ZK migration path's inline algorithm check and already supports the full range of key types.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...ice/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java
Show resolved
Hide resolved
...ice/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java
Show resolved
Hide resolved
...ice/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java
Outdated
Show resolved
Hide resolved
...-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java
Outdated
Show resolved
Hide resolved
...-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java
Show resolved
Hide resolved
...nager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java
Show resolved
Hide resolved
...nager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java
Show resolved
Hide resolved
...el/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java
Show resolved
Hide resolved
...e/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java
Show resolved
Hide resolved
kernel/keys-migrator/src/main/java/io/mosip/kernel/migrate/impl/BaseKeysMigrator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java`:
- Around line 617-618: The comparison in SignatureUtil (method returning
AlgorithmIdentifiers.EDDSA) mixes equalsIgnoreCase for
KeymanagerConstant.ED25519_KEY_TYPE with a case-sensitive equals for
KeymanagerConstant.EDDSA_KEY_TYPE; make the EDDSA check case-insensitive as well
(e.g., use equalsIgnoreCase for KeymanagerConstant.EDDSA_KEY_TYPE) or normalize
the incoming algorithm string and compare consistently so both ED25519_KEY_TYPE
and EDDSA_KEY_TYPE are matched regardless of case.
♻️ Duplicate comments (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (1)
612-616: Potential ClassCastException when casting algorithm parameters.The direct cast at Line 614 assumes the parameters are always an
ASN1ObjectIdentifier, but for EC keys with implicit parameters or certain encodings, this may benullor a different ASN.1 type.🛡️ Suggested defensive check
if (KeymanagerConstant.EC_KEY_TYPE.equalsIgnoreCase(algorithm)) { SubjectPublicKeyInfo subjectPublicKeyInfo = SubjectPublicKeyInfo.getInstance(publicKey.getEncoded()); - ASN1ObjectIdentifier curveOid = (ASN1ObjectIdentifier) subjectPublicKeyInfo.getAlgorithm().getParameters(); - - return mapCurveOidToCurveName(curveOid.getId()); + Object params = subjectPublicKeyInfo.getAlgorithm().getParameters(); + if (params instanceof ASN1ObjectIdentifier curveOid) { + return mapCurveOidToCurveName(curveOid.getId()); + } + throw new io.mosip.kernel.core.exception.NoSuchAlgorithmException( + KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorCode(), + KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorMessage()); }
🧹 Nitpick comments (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (1)
632-638: Consider caching enum names for efficient lookup.
Arrays.stream(KeyReferenceIdConsts.values()).anyMatch(...)iterates over the enum on every call. For frequently called methods, consider using a staticSet<String>for O(1) lookup.♻️ Suggested optimization
Add a static set at class level:
private static final Set<String> KEY_REFERENCE_ID_NAMES = Arrays.stream(KeyReferenceIdConsts.values()) .map(Enum::name) .collect(Collectors.toUnmodifiableSet());Then update the method:
public static String getAlgorithmString(X509Certificate x509Certificate, String refId) { - if (isDataValid(refId) && - (Arrays.stream(KeyReferenceIdConsts.values()).anyMatch(rId -> rId.name().equals(refId)))) + if (isDataValid(refId) && KEY_REFERENCE_ID_NAMES.contains(refId)) return refId; else return getJwtSignAlgorithm(x509Certificate); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
KeymanagerConstant(10-75)kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java (1)
KeymanagerConstant(12-263)
⏰ 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
🔇 Additional comments (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (1)
623-630: LGTM!The curve OID mapping correctly handles P-256 and secp256k1 curves with appropriate error handling for unsupported curves.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...el/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java
Show resolved
Hide resolved
Signed-off-by: nagendra0721 <[email protected]>
Signed-off-by: nagendra0721 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java`:
- Around line 306-314: The test currently swallows all exceptions when calling
keymanagerService.generateSymmetricKey(requestDto) inside
KeymanagerControllerTest; change the catch to only handle the known infra
exception (e.g., KeystoreProcessingException) or use JUnit assumptions to skip
the test when that specific infra exception occurs, and let any other exceptions
propagate (or explicitly fail the test) so unexpected regressions don't pass
silently; keep the original assertions on SymmetricKeyGenerateResponseDto and
status when no exception is thrown.
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.java`:
- Around line 692-712: The test currently catches all Exceptions (catch
(Exception e)) which masks real failures; change the catch to only handle the
environment-dependent KeymanagerServiceException (or the specific exception
thrown by generateMasterKey/generateECSignKey/getCertificate) and in that catch
call JUnit's Assume.assumeNoException("HSM/keystore not available in test", e)
so the test is skipped when HSM is unavailable; remove the generic catch so
other exceptions bubble and fail the test. Locate this in
KeymanagerServiceImplTest around the block invoking service.generateMasterKey,
service.getCertificate, service.generateECSignKey and updateKeyExpiry and
replace the broad catch with the specific exception handling using
Assume.assumeNoException.
- Around line 422-437: The test currently swallows all exceptions with a generic
catch and Assert.assertNotNull(e); instead, remove the broad catch(Exception)
and either (A) catch the specific KeystoreProcessingException (or the concrete
exception thrown when HSM/keystore is unavailable) and call
Assume.assumeTrue("HSM unavailable", false) to skip the test, or (B) let
unexpected exceptions propagate (or call Assert.fail(e.getMessage())) so the
test fails; update KeymanagerServiceImplTest to call
service.generateSymmetricKey(requestDto) directly and only handle the specific
keystore-related exception rather than catching Exception.
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java`:
- Around line 366-378: The test is swallowing a NullPointerException which
indicates ZKCryptoManagerServiceImpl.zkReEncryptRandomKey lets
Objects.requireNonNull(kyAlias) surface; update zkReEncryptRandomKey in
ZKCryptoManagerServiceImpl to explicitly check for null encRandomKey and/or
kyAlias and throw a ZKCryptoException with a clear message before calling
Objects.requireNonNull so null safety is handled in-domain, then update
ZKCryptoManagerServiceTest (the try/catch around zkReEncryptRandomKey) to remove
the NullPointerException catch and assert only that a ZKCryptoException is
thrown with the expected message.
♻️ Duplicate comments (3)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java (1)
401-412: Same NPE handling concern astestZkReEncryptRandomKeyNoMatchingThumbprint.This test follows the same pattern of accepting
NullPointerExceptionas valid. The same production code fix would address both cases.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java (2)
104-105: Duplicate configuration field.
certificateSignRefIDon line 105 duplicatessignRefIdon line 92 - both use the same@Value("${mosip.sign-certificate-refid:SIGN}")annotation. This creates confusion and maintenance burden.♻️ Proposed fix - use the existing signRefId field
- `@Value`("${mosip.sign-certificate-refid:SIGN}") - private String certificateSignRefID; // Then update line 459 to use signRefId instead of certificateSignRefID - && refId.get().equals(certificateSignRefID)) || + && refId.get().equals(signRefId)) ||
443-457: Potential NoSuchElementException on Optional.get() before presence check.Line 446 calls
refId.get()unconditionally, but line 450 then checks!refId.isPresent(). IfrefIdis empty, line 446 throwsNoSuchElementExceptionbefore the check executes.🐛 Proposed fix
public Object[] getEncryptedPrivateKey(String appId, Optional<String> refId, String certThumbprint) { + String referenceId = refId.orElse(KeymanagerConstant.EMPTY); LocalDateTime localDateTime = DateUtils.getUTCCurrentDateTime(); - Map<String, List<KeyAlias>> keyAliasMap = dbHelper.getKeyAliases(appId, refId.get(), localDateTime); - KeyAlias keyAlias = keyAliasMap.get(KeymanagerConstant.CURRENTKEYALIAS).getFirst(); + Map<String, List<KeyAlias>> keyAliasMap = dbHelper.getKeyAliases(appId, referenceId, localDateTime); + List<KeyAlias> currentKeyAliasList = keyAliasMap.get(KeymanagerConstant.CURRENTKEYALIAS); + if (currentKeyAliasList == null || currentKeyAliasList.isEmpty()) { + throw new NoUniqueAliasException(KeymanagerErrorConstant.NO_UNIQUE_ALIAS.getErrorCode(), + KeymanagerErrorConstant.NO_UNIQUE_ALIAS.getErrorMessage()); + } + KeyAlias keyAlias = currentKeyAliasList.getFirst(); String ksAlias = keyAlias.getAlias(); - if (!refId.isPresent() || refId.get().trim().isEmpty()) { + if (referenceId.trim().isEmpty()) {
🧹 Nitpick comments (3)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java (1)
535-551: Keep a minimal response assertion to prevent false positives.
Line 548 now only checks HTTP 200; the test will pass even if the body is empty or contains errors. Consider keeping a lightweight response assertion (e.g.,$.responseexists) to preserve signal.✅ Suggested assertion
mockMvc.perform(post("/generateSymmetricKey") .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(request))) - .andExpect(status().isOk()); + .andExpect(status().isOk()) + .andExpect(jsonPath("$.response").exists());kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java (2)
109-110: Unused mock:ecCryptomanagerServiceis declared but never used.This mock is injected but no test methods stub or verify interactions with it. If
ZKCryptoManagerServiceImplnow depends onEcCryptomanagerServicefor ECC flows, consider adding tests that exercise those code paths. Otherwise, remove this unused mock.
1548-1563: Good helper method for reducing test duplication, with a minor redundancy.The helper consolidates mock setup nicely. However, line 1553 (
when(mockCertificate.getPublicKey())...) duplicates the stub already configured insetUp()at line 143. This is harmless but unnecessary.♻️ Optional cleanup
private void setupZkReEncryptRandomKeyMocks(KeyAlias keyAlias) throws Exception { // Mock keyAlias and keyStore lookups when(keyAliasRepository.findById(anyString())).thenReturn(Optional.of(keyAlias)); when(keyStoreRepository.findByAlias(anyString())).thenReturn(createKeyStoreOptional()); when(keymanagerUtil.convertToCertificate(anyString())).thenReturn(mockCertificate); - when(mockCertificate.getPublicKey()).thenReturn(keyPair.getPublic()); when(cryptomanagerUtil.getEncryptedPrivateKey(anyString(), any(), anyString())) .thenReturn(new Object[]{keyPair.getPrivate()}); // Use lenient() for stubs that may not be used in all tests that call this helper
...rc/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java
Show resolved
Hide resolved
.../src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.java
Show resolved
Hide resolved
.../src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.java
Show resolved
Hide resolved
...r-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.