MOSIP-36428: ecc encryption support#514
Conversation
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces elliptic curve cryptography (ECC) support with SECP256R1, SECP256K1, X25519, and Ed25519 algorithms. Adds EC-based asymmetric encryption/decryption services, extends key generation and signing workflows, updates HSM/keystore handling, and creates a keys-algorithm-migrator module for key rotation. Removes legacy license files and expands role-based access control. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CryptomanagerService
participant EcCryptomanagerService
participant KeymanagerUtil
participant Database
Note over Client,Database: EC Encryption Flow
Client->>CryptomanagerService: encrypt(PublicKey, data)
CryptomanagerService->>KeymanagerUtil: detect key algorithm
alt RSA Key
CryptomanagerService->>CryptomanagerService: RSA encrypt path
else EC Key
CryptomanagerService->>EcCryptomanagerService: asymmetricEcEncrypt(PublicKey, data, curveName)
EcCryptomanagerService->>EcCryptomanagerService: generate ephemeral KeyPair
EcCryptomanagerService->>EcCryptomanagerService: perform key agreement
EcCryptomanagerService->>EcCryptomanagerService: HKDF derive AES-GCM key
EcCryptomanagerService->>EcCryptomanagerService: encrypt data with AES-GCM
EcCryptomanagerService-->>CryptomanagerService: ciphertext + ephemeralPublicKey
end
CryptomanagerService->>Database: retrieve certificate
CryptomanagerService-->>Client: encrypted payload + header + thumbprint
sequenceDiagram
participant Client
participant CryptomanagerService
participant EcCryptomanagerService
participant CryptomanagerUtil
participant KeyStore
Note over Client,KeyStore: EC Decryption Flow
Client->>CryptomanagerService: decrypt(PrivateKey, data)
CryptomanagerService->>CryptomanagerUtil: getAlgorithmNameFromHeader(data)
alt RSA Algorithm
CryptomanagerService->>CryptomanagerService: RSA decrypt path
else EC Algorithm
CryptomanagerService->>KeyStore: retrievePrivateKeyByCertThumbprint(thumbprint)
CryptomanagerService->>EcCryptomanagerService: asymmetricEcDecrypt(PrivateKey, data, curveName)
EcCryptomanagerService->>EcCryptomanagerService: extract ephemeralPublicKey from payload
EcCryptomanagerService->>EcCryptomanagerService: perform key agreement
EcCryptomanagerService->>EcCryptomanagerService: HKDF derive same AES-GCM key
EcCryptomanagerService->>EcCryptomanagerService: decrypt ciphertext
EcCryptomanagerService-->>CryptomanagerService: plaintext
end
CryptomanagerService-->>Client: decrypted data
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
Outdated
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 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
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
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (1)
113-135:⚠️ Potential issue | 🟠 Major
signAlgorithmparameter is accepted but silently ignored at Line 119.The overload at Line 113 accepts
signAlgorithmas a parameter, but Line 119 re-derives the algorithm viagetSignatureAlgorithm(signPrivateKey), making the parameter dead code. Meanwhile, the sibling overload at Line 137 does use thesignAlgorithmparameter (Line 143). This inconsistency is confusing and error-prone — callers at Lines 105 and 108 pass the result ofgetSignatureAlgorithm(signPrivateKey)assignAlgorithm, only for it to be discarded.Either honor the parameter (use
signAlgorithmat Line 119 instead of re-deriving) or remove it from the method signature and always derive internally.♻️ Proposed fix — honor the parameter consistently
- ContentSigner certContentSigner = new JcaContentSignerBuilder(getSignatureAlgorithm(signPrivateKey)).setProvider(providerName).build(signPrivateKey); + ContentSigner certContentSigner = new JcaContentSignerBuilder(signAlgorithm).setProvider(providerName).build(signPrivateKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java` around lines 113 - 135, The generateX509Certificate method accepts a signAlgorithm parameter but currently ignores it by calling getSignatureAlgorithm(signPrivateKey) when building the ContentSigner; update the ContentSigner creation in generateX509Certificate to use the provided signAlgorithm (i.e., new JcaContentSignerBuilder(signAlgorithm).setProvider(providerName).build(signPrivateKey)) so it matches the sibling overload, or alternatively remove the signAlgorithm parameter from this method signature and callers so the algorithm is consistently derived inside the method—ensure the change is applied to the ContentSigner creation path in generateX509Certificate.
♻️ Duplicate comments (6)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (1)
296-309:⚠️ Potential issue | 🔴 CriticalX25519 cannot be used for signing — returning it as a signature algorithm will fail at runtime.
This was flagged in a previous review and remains unresolved. X25519 is a key-agreement (Diffie-Hellman) algorithm.
JcaContentSignerBuilder("X25519")will throwNoSuchAlgorithmException. Either throw an explicit error or require a separate signing key for X25519 certificates.🐛 Proposed fix
- else if (keyAlgorithm.equals(KeymanagerConstant.X25519_KEY_TYPE)) - return KeymanagerConstant.X25519_KEY_TYPE; + else if (keyAlgorithm.equals(KeymanagerConstant.X25519_KEY_TYPE) || + keyAlgorithm.equals(KeymanagerConstant.XDH_ALGORITHM)) + throw new KeystoreProcessingException(KeymanagerErrorCode.CERTIFICATE_PROCESSING_ERROR.getErrorCode(), + "X25519 keys cannot be used for signing. Use a signing key (RSA, EC, or Ed25519) instead.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java` around lines 296 - 309, getSignatureAlgorithm currently returns X25519 for X25519 keys which is incorrect because X25519 is not a signing algorithm; update getSignatureAlgorithm so that when privateKey.getAlgorithm() equals KeymanagerConstant.X25519_KEY_TYPE it does NOT return X25519 but instead throws a clear runtime exception (e.g., IllegalArgumentException or UnsupportedOperationException) with a message stating that X25519 cannot be used for signing and a signing-capable key is required; keep existing branches for EC/ED25519/RSA unchanged and ensure the exception mentions getSignatureAlgorithm and KeymanagerConstant.X25519_KEY_TYPE so callers can detect and handle the unsupported key type.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.java (1)
424-439:⚠️ Potential issue | 🟡 MinorAvoid catching generic Exception in tests.
Catching
Exceptionmakes these tests pass even on real failures. Prefer catching the specific keystore/HSM exception and skipping viaAssume, or let unexpected exceptions fail the test.Also applies to: 694-714
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.java` around lines 424 - 439, Replace the broad catch(Exception) in KeymanagerServiceImplTest around calls to service.generateSymmetricKey(requestDto) with a specific keystore/HSM-related exception (e.g., KeystoreProcessingException) and either rethrow unexpected exceptions so the test fails or use JUnit Assume to skip when the keystore is unavailable; update the catch blocks that wrap generateSymmetricKey (and the duplicate block around lines 694-714) to catch KeystoreProcessingException and call Assume.assumeNoException(e) or Assume.assumeTrue(false) with a clear message so only genuine keystore availability issues are skipped while other failures surface.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java (1)
471-472:⚠️ Potential issue | 🟠 MajorBlock X25519 from X.509 signing paths.
generateAndStoreAsymKeyalways issues X.509 certificates usingsignAlgorithm. X25519 is a key‑agreement algorithm, so trying to sign certificates with it will fail or produce invalid certificates. Consider rejecting X25519 in the certificate-generation path or using a separate signing key.Is X25519 a valid signature algorithm for X.509 certificates (JCA/JCE)? If not, what is the recommended handling?🛑 Possible guard
- else if (io.mosip.kernel.keymanagerservice.constant.KeymanagerConstant.X25519_KEY_TYPE.equals(keyType)) - return generateX25519KeyPair(); + else if (io.mosip.kernel.keymanagerservice.constant.KeymanagerConstant.X25519_KEY_TYPE.equals(keyType)) + throw new KeystoreProcessingException(KeymanagerErrorCode.ALGORITHM_NOT_SUPPORTED.getErrorCode(), + KeymanagerErrorCode.ALGORITHM_NOT_SUPPORTED.getErrorMessage());Also applies to: 621-629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java` around lines 471 - 472, The code currently allows X25519 keys to be generated by generateX25519KeyPair and then passed into the X.509 certificate signing path in generateAndStoreAsymKey where signAlgorithm is used; X25519 is a key-agreement algorithm (not a JCA/JCE X.509 signature algorithm) and must not be used to sign certificates. Fix by adding a guard in generateAndStoreAsymKey (and the same logic around the block that calls generateX25519KeyPair) to reject or throw a clear exception when keyType equals KeymanagerConstant.X25519_KEY_TYPE for certificate-generation/signing flows, or alternatively generate X25519 keys only for non-signing uses and ensure certificate creation uses an appropriate signature key (e.g., EC/EdDSA with Ed25519) so signAlgorithm is only applied to valid signature-capable key types.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java (1)
676-678:⚠️ Potential issue | 🟡 MinorStandardize raw‑sign algorithm identifiers with JWS/JWT paths.
Raw sign defaults to
privateKey.getAlgorithm()(e.g., RSA/EC/Ed25519) while JWS/JWT paths use JWT identifiers (RS256/ES256/EdDSA). This inconsistency can confuse providers/clients. Consider usingSignatureUtil.getJwtSignAlgorithm(...)when the request doesn’t specify an algorithm.#!/bin/bash # Check how SignatureProviderEnum and SignatureUtil map algorithms to providers. rg -n "enum SignatureProviderEnum|getSignatureProvider" --type=java -C 2 rg -n "getJwtSignAlgorithm" --type=java -C 2Also applies to: 740-742, 767-767
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java` around lines 676 - 678, The code in SignatureServiceImpl uses privateKey.getAlgorithm() as the default signAlgorithm when signatureReq.getSignAlgorithm() is empty, causing raw algorithm names (e.g., RSA/EC/Ed25519) to be returned instead of JWT/JWS identifiers; update the defaulting logic to call SignatureUtil.getJwtSignAlgorithm(certificateResponse.getCertificateEntry().getPrivateKey().getAlgorithm()) wherever you currently fall back to privateKey.getAlgorithm() (e.g., the assignment using SignatureUtil.isDataValid(signatureReq.getSignAlgorithm()), and the similar spots around the blocks at the other occurrences flagged), keeping the request-specified value when SignatureUtil.isDataValid(...) is true.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java (2)
91-109: Avoid duplicate sign-ref configuration fields.
certificateSignRefIDduplicatessignRefId(same property key). This adds maintenance overhead without adding value.♻️ Suggested cleanup
- `@Value`("${mosip.sign-certificate-refid:SIGN}") - private String certificateSignRefID;And use
signRefIdin the RSA-sign reference checks instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java` around lines 91 - 109, The file defines duplicate configuration fields signRefId and certificateSignRefID both bound to "${mosip.sign-certificate-refid:SIGN}"; remove the redundant certificateSignRefID field and replace any usages of certificateSignRefID (e.g., in RSA-sign reference checks) with the existing signRefId variable (update references in methods/classes that currently read certificateSignRefID to use signRefId instead) so there is a single source of truth for the sign-ref configuration.
443-510:⚠️ Potential issue | 🔴 CriticalLine 446: Optional misuse + empty-list access can throw.
refId.get()is called before checking presence, andgetFirst()is used even when both alias lists can be empty. Both can throw at runtime.🛠️ Suggested fix
- Map<String, List<KeyAlias>> keyAliasMap = dbHelper.getKeyAliases(appId, refId.get(), localDateTime); - List<KeyAlias> curkeyAliasList = keyAliasMap.getOrDefault(KeymanagerConstant.CURRENTKEYALIAS, Collections.emptyList()); - List<KeyAlias> keyAliasList = keyAliasMap.getOrDefault(KeymanagerConstant.KEYALIAS, Collections.emptyList()); - String ksAlias = curkeyAliasList.isEmpty() ? keyAliasList.getFirst().getAlias() : curkeyAliasList.getFirst().getAlias(); + String refValue = refId.map(String::trim).orElse(KeymanagerConstant.EMPTY); + Map<String, List<KeyAlias>> keyAliasMap = dbHelper.getKeyAliases(appId, refValue, localDateTime); + List<KeyAlias> curkeyAliasList = keyAliasMap.getOrDefault(KeymanagerConstant.CURRENTKEYALIAS, Collections.emptyList()); + List<KeyAlias> keyAliasList = keyAliasMap.getOrDefault(KeymanagerConstant.KEYALIAS, Collections.emptyList()); + if (curkeyAliasList.isEmpty() && keyAliasList.isEmpty()) { + throw new NoUniqueAliasException(KeymanagerErrorConstant.NO_UNIQUE_ALIAS.getErrorCode(), + KeymanagerErrorConstant.NO_UNIQUE_ALIAS.getErrorMessage()); + } + String ksAlias = !curkeyAliasList.isEmpty() + ? curkeyAliasList.getFirst().getAlias() + : keyAliasList.getFirst().getAlias(); - if (!refId.isPresent() || refId.get().trim().isEmpty()) { + if (refValue.isEmpty()) { LOGGER.info(KeymanagerConstant.SESSIONID, KeymanagerConstant.EMPTY, KeymanagerConstant.EMPTY, "Not valid reference Id. Getting private key from HSM."); KeyStore.PrivateKeyEntry masterKeyEntry = keyStore.getAsymmetricKey(ksAlias); ... - } else if ((appId.equalsIgnoreCase(signApplicationId) && refId.isPresent() - && refId.get().equals(certificateSignRefID)) || - (refId.isPresent() && refId.get().equals(KeyReferenceIdConsts.EC_SECP256K1_SIGN.name())) || - (refId.isPresent() && refId.get().equals(KeyReferenceIdConsts.EC_SECP256R1_SIGN.name())) || - (refId.isPresent() && refId.get().equals(KeyReferenceIdConsts.ED25519_SIGN.name()) + } else if ((appId.equalsIgnoreCase(signApplicationId) && refValue.equals(signRefId)) || + (refValue.equals(KeyReferenceIdConsts.EC_SECP256K1_SIGN.name())) || + (refValue.equals(KeyReferenceIdConsts.EC_SECP256R1_SIGN.name())) || + (refValue.equals(KeyReferenceIdConsts.ED25519_SIGN.name()) && ed25519SupportFlag)) { ... } else { ... - String referenceId = refId.get(); + String referenceId = refValue; io.mosip.kernel.keymanagerservice.entity.KeyStore dbKeyStore = privateKeyDecryptorHelper.getDBKeyStoreData(certThumbprint, appId, referenceId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java` around lines 443 - 510, getEncryptedPrivateKey misuses Optional and assumes non-empty alias lists: change the call to dbHelper.getKeyAliases to pass refId.orElse(null) (or empty string) so you don't call refId.get() before presence check; then guard selection of ksAlias by checking curkeyAliasList.isEmpty() and keyAliasList.isEmpty() and throw a clear NoUniqueAliasException (or appropriate exception) if both are empty instead of calling keyAliasList.getFirst(); also replace subsequent refId.get() usages with a safe local variable set after verifying refId.isPresent() (e.g., String referenceId = refId.get()) so all Optional access in getEncryptedPrivateKey is protected.
🟠 Major comments (19)
kernel/kernel-keymanager-service/src/main/resources/application-local.properties-54-55 (1)
54-55:⚠️ Potential issue | 🟠 MajorDo not commit DB credentials in local properties.
Use environment variables or a secrets manager; committing credentials is a security and compliance risk.
🔒 Proposed fix
-keymanager_database_username=postgres -keymanager_database_password=0685 +keymanager_database_username=${KEYMANAGER_DATABASE_USERNAME} +keymanager_database_password=${KEYMANAGER_DATABASE_PASSWORD}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local.properties` around lines 54 - 55, Remove the hard-coded credentials for keymanager_database_username and keymanager_database_password and replace them with references to environment-backed properties or a secrets manager; e.g., set those keys to read from environment variables or an externalized secret (use the property names keymanager_database_username and keymanager_database_password as the placeholders) and update application startup/configuration to supply the real values from environment variables or a secrets store instead of committing them here.kernel/kernel-keymanager-service/src/main/resources/application-local.properties-133-134 (1)
133-134:⚠️ Potential issue | 🟠 MajorClient secret must not be stored in plaintext.
Move the IAM client secret to env/secret manager and keep only a reference in this file.
🔒 Proposed fix
-mosip.iam.adapter.clientsecret=2sZwR8TlYVOe9jkL +mosip.iam.adapter.clientsecret=${MOSIP_IAM_ADAPTER_CLIENTSECRET}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local.properties` around lines 133 - 134, Plaintext IAM client secret stored in the property mosip.iam.adapter.clientsecret must be removed; change the hard-coded value to a reference (e.g., environment variable or secret-manager placeholder) and store the real secret in your env/secret manager instead, update any code/config resolution so the application reads mosip.iam.adapter.clientsecret from the secret store or an environment variable, and ensure mosip.iam.adapter.appid remains unchanged; also update deployment/README to document the new secret key name and where to provision it.kernel/kernel-keymanager-service/src/main/resources/application-local.properties-92-93 (1)
92-93:⚠️ Potential issue | 🟠 MajorRemove plaintext salts from committed config.
These look like secrets and should be sourced from env/secret manager, not committed to VCS.
🔒 Proposed fix
-mosip.kernel.tokenid.uin.salt=zHuDEAbmbxiUbUShgy6pwUhKh9DE0EZn9kQDKPPKbWscGajMwf -mosip.kernel.tokenid.partnercode.salt=yS8w5Wb6vhIKdf1msi4LYTJks7mqkbmITk2O63Iq8h0bkRlD0d +mosip.kernel.tokenid.uin.salt=${MOSIP_KERNEL_TOKENID_UIN_SALT} +mosip.kernel.tokenid.partnercode.salt=${MOSIP_KERNEL_TOKENID_PARTNERCODE_SALT}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local.properties` around lines 92 - 93, The committed application-local.properties contains plaintext secrets for mosip.kernel.tokenid.uin.salt and mosip.kernel.tokenid.partnercode.salt; remove those literal values and change the config to read them from environment/secret manager (e.g., replace the hardcoded salts with placeholders and load via environment variables or your secret provider), update any code that references these keys to fallback or fail fast if the env/secret values are missing (look for usage of mosip.kernel.tokenid.uin.salt and mosip.kernel.tokenid.partnercode.salt to ensure runtime configuration is reading from System/env or the secret service), and add guidance in docs or a sample properties file (without secrets) showing the required env variable names.kernel/kernel-keymanager-service/src/main/resources/application-local1.properties-135-135 (1)
135-135:⚠️ Potential issue | 🟠 MajorIAM client secret hardcoded in source code.
mosip.iam.adapter.clientsecret=2sZwR8TlYVOe9jkLis a live credential committed to the repository. It should be externalised to a secrets manager or supplied at runtime via environment variables, and the exposed secret should be rotated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local1.properties` at line 135, The property mosip.iam.adapter.clientsecret is a hardcoded live credential—remove this value from source and stop committing secrets; change the configuration to read the client secret from a secure external source (e.g., environment variable, secrets manager, or vault) instead of the literal value, update the code/configuration that reads mosip.iam.adapter.clientsecret to use the chosen secret provider, rotate the exposed secret immediately, purge the secret from the repo history, and document how to supply the secret at runtime so deployments use the externalized value.kernel/kernel-keymanager-service/src/main/resources/application-local1.properties-94-95 (1)
94-95:⚠️ Potential issue | 🟠 MajorCryptographic salts committed to the repository.
mosip.kernel.tokenid.uin.saltandmosip.kernel.tokenid.partnercode.saltare long-lived secret values whose confidentiality underpins token-ID derivation security. Committing them exposes all token IDs derivable from known UINs. These must be externalised and the values here considered compromised if this branch is ever merged or the history is public.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local1.properties` around lines 94 - 95, The two hard-coded secret salts (mosip.kernel.tokenid.uin.salt and mosip.kernel.tokenid.partnercode.salt) must be removed from source control and externalised: replace these property values with configuration lookups that read from an injected secret source (environment variables, Vault/Secrets Manager, or a secure config provider) and make the application fail-fast if they are missing; treat the current values as compromised and rotate/regenerate new salts, update any systems depending on them, and purge or rewrite repo history to remove the exposed values; also add a template (placeholder properties) and document the required env var/secret names so CI/deployment injects them securely.kernel/kernel-keymanager-service/src/main/resources/application-local1.properties-8-8 (1)
8-8:⚠️ Potential issue | 🟠 MajorPasswords committed to source — even local profiles end up in version-control history.
Both
mosip.kernel.keymanager.hsm.keystore-pass=1629(trivially short) andkeymanager_database_password=0685are embedded in plain text. These should be injected via environment variables or a secrets manager rather than stored in a committed properties file.Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local1.properties` at line 8, Remove the hard-coded secrets from the properties file—delete entries for mosip.kernel.keymanager.hsm.keystore-pass and keymanager_database_password and replace them with environment/secret placeholders (e.g. use property placeholders like ${KEYMANAGER_HSM_KEYSTORE_PASS} and ${KEYMANAGER_DATABASE_PASSWORD} or integrate with the project's secrets manager), update the configuration-loading code (where these properties are read) to fail fast if the env/secret is missing, ensure no default plaintext values remain, and add a brief note to deployment/README and CI to inject those env vars; after fixing, remove the committed secrets from git history using your repo's secret-removal procedure (git filter-repo or BFG) and rotate the exposed credentials.kernel/kernel-keymanager-service/src/main/resources/application-local1.properties-113-114 (1)
113-114:⚠️ Potential issue | 🟠 MajorAES/ECB mode is insecure for key-wrapping and key-derivation operations.
Both
mosip.kernel.zkcrypto.wrap.algorithm-name=AES/ECB/NoPaddingandmosip.kernel.zkcrypto.derive.encrypt.algorithm-name=AES/ECB/PKCS5Paddinguse ECB mode. ECB encrypts each block independently, so identical plaintext blocks produce identical ciphertext, leaking structure. Key-wrapping should useAES/GCM/NoPadding(authenticated) or at minimumAES/CBCwith a fresh random IV.🔧 Suggested replacements
-mosip.kernel.zkcrypto.wrap.algorithm-name=AES/ECB/NoPadding +mosip.kernel.zkcrypto.wrap.algorithm-name=AES/GCM/NoPadding -mosip.kernel.zkcrypto.derive.encrypt.algorithm-name=AES/ECB/PKCS5Padding +mosip.kernel.zkcrypto.derive.encrypt.algorithm-name=AES/GCM/NoPadding🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local1.properties` around lines 113 - 114, The properties mosip.kernel.zkcrypto.wrap.algorithm-name and mosip.kernel.zkcrypto.derive.encrypt.algorithm-name are configured to use insecure AES/ECB; change them to a secure mode (prefer AES/GCM/NoPadding for authenticated encryption, or AES/CBC/PKCS5Padding if GCM is unavailable) and update the surrounding key-wrap/derive code to generate, store and pass a fresh random IV/nonce and to handle GCM authentication tags where applicable (adjust any wrap/unwrap or encrypt/decrypt logic that references these properties to use the IV/nonce and tag correctly).kernel/kernel-keymanager-service/src/main/resources/application-local1.properties-49-49 (1)
49-49:⚠️ Potential issue | 🟠 Major
ES256andsecp256k1are incompatible — choose one approach to align them.
mosip.kernel.crypto.sign-algorithm-name=ES256(line 49) uses ECDSA with P-256 (secp256r1) and SHA-256 per RFC 7518. However, line 206 configuresmosip.kernel.keygenerator.ecc-curve-name=secp256k1, which is a different curve entirely. RFC 8812 explicitly requires that secp256k1 must only be used with theES256Kalgorithm identifier, notES256. This mismatch will either cause signing to fail at runtime or produce JWTs that cannot be verified by standards-compliant verifiers.Fix: Either change the curve to
secp256r1to matchES256, or change the algorithm toES256Kto matchsecp256k1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/resources/application-local1.properties` at line 49, There's a mismatch between mosip.kernel.crypto.sign-algorithm-name and mosip.kernel.keygenerator.ecc-curve-name: ES256 requires curve secp256r1 (P-256) while secp256k1 must use ES256K; update either mosip.kernel.crypto.sign-algorithm-name to ES256K to match mosip.kernel.keygenerator.ecc-curve-name=secp256k1, or change mosip.kernel.keygenerator.ecc-curve-name to secp256r1 to match mosip.kernel.crypto.sign-algorithm-name=ES256 so both properties use a compatible algorithm/curve pair.kernel/keys-algorithm-migrator/configure_start.sh-6-23 (1)
6-23:⚠️ Potential issue | 🟠 MajorFail fast when
artifactory_url_envis unset; quote paths.
Currently the script assumesartifactory_url_envexists (Shellcheck SC2154). If unset,wgethits an invalid URL and the failure message is unclear.✅ Suggested fix
DEFAULT_ZIP_PATH=artifactory/libs-release-local/hsm/client.zip [ -z "$zip_file_path" ] && zip_path="$DEFAULT_ZIP_PATH" || zip_path="$zip_file_path" +if [ -z "${artifactory_url_env:-}" ]; then + echo "artifactory_url_env is required" >&2 + exit 1 +fi + @@ -wget "$artifactory_url_env/$zip_path" +wget "${artifactory_url_env}/${zip_path}" @@ -unzip $FILE_NAME +unzip "${FILE_NAME}" @@ -cd ./$DIR_NAME && ./install.sh +cd "./${DIR_NAME}" && ./install.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/configure_start.sh` around lines 6 - 23, Check that artifactory_url_env is set at the top of the script and exit with a clear error if not (fail fast), and ensure all variable expansions used in commands are quoted; specifically validate artifactory_url_env before using it in the wget call, quote "$artifactory_url_env/$zip_path", quote "$zip_path", "$FILE_NAME" and "$DIR_NAME" in unzip/cd/./install.sh invocations, and use DEFAULT_ZIP_PATH/zip_path logic as-is but referencing the quoted variables when constructing the URL and file operations.kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/config/AppConfig.java-10-12 (1)
10-12:⚠️ Potential issue | 🟠 MajorSet connect/read timeouts on RestTemplate.
Default timeouts are infinite, which can hang migration flows on network issues. This is particularly critical since RestTemplate is used for external HTTP calls in key migration workflows (e.g., token generation, certificate retrieval). UseRestTemplateBuilderwith explicitDuration-based timeouts.✅ Suggested fix
+import java.time.Duration; +import org.springframework.boot.web.client.RestTemplateBuilder; import org.springframework.web.client.RestTemplate; @@ - public RestTemplate restTemplate() { - return new RestTemplate(); - } + public RestTemplate restTemplate(RestTemplateBuilder builder) { + return builder + .setConnectTimeout(Duration.ofSeconds(5)) + .setReadTimeout(Duration.ofSeconds(30)) + .build(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/config/AppConfig.java` around lines 10 - 12, The RestTemplate bean in AppConfig currently returns a plain RestTemplate with infinite timeouts; change the restTemplate() method to use RestTemplateBuilder and set explicit Duration-based timeouts (connect and read) before building and returning the RestTemplate so external HTTP calls (token/certificate retrieval) cannot hang indefinitely; update the AppConfig.restTemplate bean to inject RestTemplateBuilder (or create one) and call its setConnectTimeout(...) and setReadTimeout(...) with appropriate Durations then build().kernel/keys-algorithm-migrator/src/main/resources/bootstrap.properties-9-10 (1)
9-10:⚠️ Potential issue | 🟠 MajorFix Spring Cloud Config URI to include scheme and port.
spring.cloud.config.uri=localhostis missing the required URI scheme and will fail to connect to the config server. Spring Cloud Config requires an absolute URL withhttp://orhttps://scheme.Suggested fix
-spring.cloud.config.uri=localhost +spring.cloud.config.uri=${SPRING_CLOUD_CONFIG_URI:http://localhost:8888}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/src/main/resources/bootstrap.properties` around lines 9 - 10, The spring.cloud.config.uri value is invalid because it lacks a scheme and port; update the property spring.cloud.config.uri to an absolute URL including the scheme and (if applicable) the config server port (e.g., change spring.cloud.config.uri=localhost to spring.cloud.config.uri=http://localhost:8888 or https://your-config-host:port) so Spring Cloud Config can resolve and connect to the server.kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/impl/ComponentKeysAlgorithmMigrator.java-49-59 (1)
49-59:⚠️ Potential issue | 🟠 MajorROOT key is invalidated before the new key is generated — failure during generation leaves the system without a valid ROOT key.
If
generateMasterKey(ROOT_APP_ID, BLANK_REF_ID)at Line 58 throws (it is not wrapped in try-catch here), the ROOT key has already been invalidated at Line 54, leaving the system with no usable ROOT key. This can be a catastrophic failure.Consider reversing the order (generate first, invalidate old after success) or wrapping both operations such that invalidation is rolled back on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/impl/ComponentKeysAlgorithmMigrator.java` around lines 49 - 59, The rotateMasterKeys() flow currently invalidates the ROOT master key via invalidateKeyAlias(...) before calling generateMasterKey(...), which can leave no valid ROOT key if generation fails; change the sequence to call generateMasterKey(ROOT_APP_ID, BLANK_REF_ID) first and only call invalidateKeyAlias(ROOT_APP_ID, BLANK_REF_ID, ...) after generation returns successfully, or wrap the operations in a try/catch that rolls back invalidation (re-instate previous alias/state) if generateMasterKey throws; update rotateMasterKeys() to use the new order or transactional/compensating logic referencing rotateMasterKeys(), invalidateKeyAlias(...), generateMasterKey(...), and the rootKeyAlias list obtained from dbHelper.getKeyAliases(...) so failure never leaves the system without a valid ROOT key.kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/impl/ComponentKeysAlgorithmMigrator.java-62-79 (1)
62-79:⚠️ Potential issue | 🟠 MajorComponent key invalidation-then-generation: failure leaves the component without a valid key.
The
catchat Line 75 logs the error and continues, but by that pointinvalidateKeyAliasat Line 69 has already expired the key. IfgenerateMasterKeyat Line 73 was the call that failed, the component's master key is now invalid with no replacement. Consider deferring invalidation until after successful generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/impl/ComponentKeysAlgorithmMigrator.java` around lines 62 - 79, In ComponentKeysAlgorithmMigrator's loop over componentKeysList the code calls invalidateKeyAlias(...) before generateMasterKey(...), which can leave a component without a valid master if generation fails; change the flow to first call generateMasterKey(appId, refId) and ensure it completes successfully (or returns the new alias/result), then call invalidateKeyAlias(appId, refId, currentKeyAliasList, timestamp) to expire the old key; use the same unique symbols (generateMasterKey, invalidateKeyAlias, dbHelper.getKeyAliases, componentKeysList.forEach) to locate the code, and make sure the catch block only handles failures after both steps or performs a rollback/cleanup if invalidate must run first in some cases.kernel/keys-algorithm-migrator/Dockerfile-85-92 (1)
85-92:⚠️ Potential issue | 🟠 MajorContainer runs as root — add a
USERdirective after setup.Static analysis (Trivy DS-0002) correctly flags that no
USERcommand is present. After creating themosipuser, switch to it. Also, use--no-install-recommendsand clean up apt caches to reduce image size.🔒️ Proposed fix
RUN apt-get -y update \ -&& apt-get install -y unzip sudo adduser \ +&& apt-get install -y --no-install-recommends unzip sudo adduser \ && groupadd -g ${container_user_gid} ${container_user_group} \ && useradd -u ${container_user_uid} -g ${container_user_group} -s /bin/bash -m ${container_user} \ && adduser ${container_user} sudo \ && echo "%sudo ALL=(ALL) NOPASSWD:/home/${container_user}/${hsm_local_dir}/install.sh" >> /etc/sudoers \ && mkdir -p ${loader_path} \ -&& chmod +x configure_start.sh +&& chmod +x configure_start.sh \ +&& apt-get clean \ +&& rm -rf /var/lib/apt/lists/* + +USER ${container_user}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/Dockerfile` around lines 85 - 92, The Dockerfile leaves the container running as root; after creating the ${container_user} user and setting up permissions (groupadd, useradd, adduser, echo to /etc/sudoers, mkdir ${loader_path}, chmod +x configure_start.sh) add a USER directive to switch to that non-root user (e.g. USER ${container_user}). Also change the apt install invocation to use --no-install-recommends and clean up apt caches after package installation (apt-get clean && rm -rf /var/lib/apt/lists/*) to reduce image size.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java-322-332 (1)
322-332:⚠️ Potential issue | 🟠 Major
providerparameter is ignored — hardcodesBC_PROVIDERfor ephemeral public key reconstruction.
getEphemeralPublicKeyaccepts aproviderparameter but always usesBC_PROVIDER("BC") at Line 324. This means the HSM provider (when configured) is bypassed for public key reconstruction, which could cause inconsistencies with other operations that honor the configured provider.🐛 Proposed fix
- KeyFactory keyFactory = KeyFactory.getInstance(algo, BC_PROVIDER); + KeyFactory keyFactory = KeyFactory.getInstance(algo, provider);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java` around lines 322 - 332, The getEphemeralPublicKey method currently ignores the provider parameter and hardcodes BC_PROVIDER; update getEphemeralPublicKey to use the incoming provider when calling KeyFactory.getInstance(algo, provider) and only fall back to BC_PROVIDER if provider is null/blank or unavailable, keeping the existing exception wrapping behavior (refer to getEphemeralPublicKey, BC_PROVIDER and the provider parameter); ensure behavior is consistent with other HSM-configured operations by validating provider before use and using the fallback as a last resort.kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/MigrateKeysAlgorithmApplication.java-25-28 (1)
25-28:⚠️ Potential issue | 🟠 MajorProcess exit code is discarded — container orchestrators won't detect failure.
SpringApplication.exit(run)returns an exit code, but it's not passed toSystem.exit(). In Docker/Kubernetes, this means the container will always report success (exit 0) even if the migration encountered errors.🐛 Proposed fix
public static void main(String[] args) { ConfigurableApplicationContext run = SpringApplication.run(MigrateKeysAlgorithmApplication.class, args); - SpringApplication.exit(run); + System.exit(SpringApplication.exit(run)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/MigrateKeysAlgorithmApplication.java` around lines 25 - 28, The main method in MigrateKeysAlgorithmApplication currently calls SpringApplication.exit(run) but discards its return value; change it to capture the exit code and pass it to System.exit(...) so the process returns the correct status to the container runtime. Locate the main method and the ConfigurableApplicationContext variable run, call SpringApplication.exit(run) into an int (or use the returned ExitCodeGenerator result) and invoke System.exit(exitCode) before returning from main.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java-329-330 (1)
329-330:⚠️ Potential issue | 🟠 MajorMake referenceId comparison null‑safe to avoid NPEs.
referenceId.equals(...)will throw ifreferenceIdis null. UseObjects.equalsor constant-first comparison to keep signing resilient.🔧 Suggested fix
- String algoString = (referenceId.equals(KeymanagerConstant.EMPTY) || referenceId.equals(certificateSignRefID)) ? + String algoString = (KeymanagerConstant.EMPTY.equals(referenceId) || Objects.equals(referenceId, certificateSignRefID)) ? SignatureUtil.getJwtSignAlgorithm(x509Certificate) : SignatureAlgorithmIdentifyEnum.getAlgorithmIdentifier(referenceId);Also applies to: 887-889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java` around lines 329 - 330, The referenceId null-unsafe comparisons in SignatureServiceImpl (used when computing algoString and again around lines 887-889) must be made null-safe: replace expressions like referenceId.equals(KeymanagerConstant.EMPTY) and referenceId.equals(certificateSignRefID) with null-safe checks (e.g., Objects.equals(referenceId, KeymanagerConstant.EMPTY) or KeymanagerConstant.EMPTY.equals(referenceId)) before deciding between SignatureUtil.getJwtSignAlgorithm(x509Certificate) and SignatureAlgorithmIdentifyEnum.getAlgorithmIdentifier(referenceId); apply the same change to the other occurrence(s) to avoid NPEs.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java-268-272 (1)
268-272:⚠️ Potential issue | 🟠 MajorECC encrypt payload layout doesn’t match decrypt expectations.
Decrypt assumes the data after the splitter starts with the thumbprint, but the ECC encrypt path currently puts the header after the splitter. That means the decrypt path will read header bytes as a thumbprint and fail decryption.
🔧 Suggested fix (swap order so header is before splitter)
- byte[] finalEncKeyBytes = CryptoUtil.combineByteArray(concatedData, headerBytes, keySplitter); - cryptoResponseDto.setData(CryptoUtil.encodeToURLSafeBase64(finalEncKeyBytes)); + byte[] finalEncKeyBytes = CryptoUtil.combineByteArray(headerBytes, concatedData, keySplitter); + cryptoResponseDto.setData(CryptoUtil.encodeToURLSafeBase64(finalEncKeyBytes));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java` around lines 268 - 272, The encrypt path currently appends headerBytes after the splitter which mismatches decrypt's expectation that bytes after the splitter begin with the cert thumbprint; fix by putting headerBytes before the splitter so the splitter separates header from (thumbprint + encryptedData). Concretely, change the combine/concatenation order used around cryptomanagerUtil.getHeaderByte(algName), cryptomanagerUtil.concatCertThumbprint(certThumbprint, encryptedDataWithIv) and CryptoUtil.combineByteArray(...) so headerBytes is the first argument (or otherwise ordered) and the concated certThumbprint+encryptedData is the second argument with keySplitter still used, then continue to encode with CryptoUtil.encodeToURLSafeBase64(...) and set via cryptoResponseDto.setData(...).kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java-244-249 (1)
244-249:⚠️ Potential issue | 🟠 MajorLine 244: ECC/RSA selection is inverted for several cases.
With the current condition, RSA config can generate ECC keys when
referenceIdisn’t inKeyReferenceIdConsts, and ECC reference IDs can fall into the RSA branch. This breaks algorithm selection for both default and ECC refIds.🛠️ Suggested fix
- if (!masterKeyAlgorithm.equals(KeymanagerConstant.RSA) || (Arrays.stream(KeyReferenceIdConsts.values()) - .noneMatch((rId) -> rId.name().equals(referenceId)))) { - keyStore.generateAndStoreAsymmetricKey(alias, rootKeyAlias, certParams, eccCurve); - } else { - keyStore.generateAndStoreAsymmetricKey(alias, rootKeyAlias, certParams); - } + boolean isEccRefId = Arrays.stream(KeyReferenceIdConsts.values()) + .anyMatch(rId -> rId.name().equals(referenceId) + && !rId.equals(KeyReferenceIdConsts.RSA_2048_SIGN)); + if (!KeymanagerConstant.RSA.equals(masterKeyAlgorithm) || isEccRefId) { + keyStore.generateAndStoreAsymmetricKey(alias, rootKeyAlias, certParams, eccCurve); + } else { + keyStore.generateAndStoreAsymmetricKey(alias, rootKeyAlias, certParams); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java` around lines 244 - 249, The algorithm selection is inverted: ensure RSA uses the two-argument generate call and ECC uses the four-argument call with eccCurve. Change the condition so RSA branch runs only when masterKeyAlgorithm.equals(KeymanagerConstant.RSA) (call keyStore.generateAndStoreAsymmetricKey(alias, rootKeyAlias, certParams)), and run the ECC branch (keyStore.generateAndStoreAsymmetricKey(alias, rootKeyAlias, certParams, eccCurve)) when masterKeyAlgorithm indicates ECC or when Arrays.stream(KeyReferenceIdConsts.values()).anyMatch(rId -> rId.name().equals(referenceId)); keep a sensible default (RSA) if neither explicitly matches.
🟡 Minor comments (5)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/EcCryptomanagerServeTest.java-36-36 (1)
36-36:⚠️ Potential issue | 🟡 MinorLikely typo in test class name:
EcCryptomanagerServeTest→EcCryptomanagerServiceTest.Missing "ic" in "Service".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/EcCryptomanagerServeTest.java` at line 36, Rename the test class from EcCryptomanagerServeTest to EcCryptomanagerServiceTest: change the public class declaration (EcCryptomanagerServeTest → EcCryptomanagerServiceTest), update the file name to match the new public class if needed, and update any references/imports or test suite entries that reference EcCryptomanagerServeTest so they point to EcCryptomanagerServiceTest.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java-127-130 (1)
127-130:⚠️ Potential issue | 🟡 MinorNPE risk if
providerNameis null.
providerName.equals("BC")will throwNullPointerExceptionifproviderNameis evernull. Use"BC".equals(providerName)for null-safe comparison.🛡️ Proposed fix
- if (providerName.equals("BC")) + if ("BC".equals(providerName))Also applies to: 154-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java` around lines 127 - 130, In CertificateUtility, avoid NullPointerException by making the providerName comparison null-safe: replace usages like providerName.equals("BC") in the logic that constructs the JcaX509CertificateConverter (the branches that call new JcaX509CertificateConverter().setProvider(providerName).getCertificate(certHolder) vs the no-provider variant) with a null-safe check such as "BC".equals(providerName) (or explicitly check providerName != null && providerName.equals("BC")); apply the same change to the other identical block that handles provider selection (the second occurrence around the code that also chooses between setProvider(providerName) and the no-arg converter).kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureAlgorithmIdentifyEnum.java-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove unused import
ECCurvesfrom line 4.The
ECCurvesclass is imported but never referenced anywhere in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureAlgorithmIdentifyEnum.java` at line 4, Remove the unused import ECCurves from the top of SignatureAlgorithmIdentifyEnum.java; locate the import statement "import io.mosip.kernel.keymanagerservice.constant.ECCurves;" in the SignatureAlgorithmIdentifyEnum file and delete it so the enum no longer contains an unused dependency (ensure no other references to ECCurves exist in methods or constants like those in SignatureAlgorithmIdentifyEnum).kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/KeyGenerator.java-94-98 (1)
94-98:⚠️ Potential issue | 🟡 MinorRemove unused
asymmetricKeyAlgorithmfield from KeyGenerator.javaThe
@Valueproperty at line 48 is no longer referenced within this class—line 95 hardcodesKeymanagerConstant.RSAinstead of using the field. Remove this dead property to keep the code clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/KeyGenerator.java` around lines 94 - 98, Remove the unused `@Value-injected` field asymmetricKeyAlgorithm from the KeyGenerator class: delete the field declaration (the `@Value` annotation and the asymmetricKeyAlgorithm variable) since getAsymmetricKey() hardcodes KeymanagerConstant.RSA; also remove any now-unused imports (e.g., org.springframework.beans.factory.annotation.Value) to keep the class clean and compile without warnings. Ensure getAsymmetricKey() and other methods (like getSecureRandom()) remain unchanged and compile after the field removal.kernel/keys-algorithm-migrator/pom.xml-199-202 (1)
199-202:⚠️ Potential issue | 🟡 MinorFix SCM connection URL typo to avoid broken release metadata.
The SCM connection string points to
keyanager(missing “m”), which breaks generated source links and release tooling.🔧 Suggested fix
- <connection>scm:git:git://github.com/mosip/keyanager.git</connection> + <connection>scm:git:git://github.com/mosip/keymanager.git</connection>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/keys-algorithm-migrator/pom.xml` around lines 199 - 202, The SCM connection URL in the pom.xml under the <scm> block contains a typo ("keyanager") that breaks release/source links; update the <connection> element value to use the correct repository name "keymanager" (and also confirm the <developerConnection> and <url> values are consistent) so generated release metadata and source links resolve correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerErrorCode.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/keymanager/hsm/impl/pkcs/PKCS11KeyStoreImpl.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/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/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/signature/constant/SignatureAlgorithmIdentifyEnum.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureErrorCode.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/dto/CoseSignRequestDto.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/dto/CoseSignVerifyRequestDto.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/resources/application-local.propertieskernel/kernel-keymanager-service/src/main/resources/application-local1.propertieskernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/EcCryptomanagerServeTest.javakernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.javakernel/keys-algorithm-migrator/Dockerfilekernel/keys-algorithm-migrator/README.mdkernel/keys-algorithm-migrator/configure_start.shkernel/keys-algorithm-migrator/pom.xmlkernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/MigrateKeysAlgorithmApplication.javakernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/config/AppConfig.javakernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/impl/ComponentKeysAlgorithmMigrator.javakernel/keys-algorithm-migrator/src/main/resources/application-local.propertieskernel/keys-algorithm-migrator/src/main/resources/bootstrap.propertieskernel/pom.xml
✅ Files skipped from review due to trivial changes (1)
- kernel/keys-algorithm-migrator/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerConstant.java
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/constant/KeymanagerErrorConstant.java
| mosip.kernel.keymanager.service.cose.privatekey=MC4CAQAwBQYDK2VwBCIEIBSRNSG9zqqQSmGWiHuI6vA3GkW6wMMFuxiupMX87JmP | ||
| mosip.kernel.keymanager.service.cose.publickey=MCowBQYDK2VwAyEAPvDN-FB2f50m3si16mEJF07X-Yn5yhyEC6jPE0D3aOE |
There was a problem hiding this comment.
Private key should not be embedded in properties.
Embedding private keys in repo config is a severe security risk. Load from HSM/keystore or secret store and reference it.
🔒 Proposed fix
-mosip.kernel.keymanager.service.cose.privatekey=MC4CAQAwBQYDK2VwBCIEIBSRNSG9zqqQSmGWiHuI6vA3GkW6wMMFuxiupMX87JmP
-mosip.kernel.keymanager.service.cose.publickey=MCowBQYDK2VwAyEAPvDN-FB2f50m3si16mEJF07X-Yn5yhyEC6jPE0D3aOE
+mosip.kernel.keymanager.service.cose.privatekey=${MOSIP_KERNEL_KEYMANAGER_COSE_PRIVATEKEY}
+mosip.kernel.keymanager.service.cose.publickey=${MOSIP_KERNEL_KEYMANAGER_COSE_PUBLICKEY}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mosip.kernel.keymanager.service.cose.privatekey=MC4CAQAwBQYDK2VwBCIEIBSRNSG9zqqQSmGWiHuI6vA3GkW6wMMFuxiupMX87JmP | |
| mosip.kernel.keymanager.service.cose.publickey=MCowBQYDK2VwAyEAPvDN-FB2f50m3si16mEJF07X-Yn5yhyEC6jPE0D3aOE | |
| mosip.kernel.keymanager.service.cose.privatekey=${MOSIP_KERNEL_KEYMANAGER_COSE_PRIVATEKEY} | |
| mosip.kernel.keymanager.service.cose.publickey=${MOSIP_KERNEL_KEYMANAGER_COSE_PUBLICKEY} |
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 193-193: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kernel/kernel-keymanager-service/src/main/resources/application-local.properties`
around lines 193 - 194, The properties
mosip.kernel.keymanager.service.cose.privatekey and
mosip.kernel.keymanager.service.cose.publickey should not contain raw key
material; remove these hard-coded values from application-local.properties and
change the code that reads these properties (lookup locations that read those
keys) to fetch keys from a secure store (HSM, keystore, or secret manager) at
runtime instead. Replace direct property usage with a secure retrieval method
(e.g., KeyStore/HSM client or vault client) and ensure the property keys now
reference only the secret identifier or keystore alias, then update
initialization logic to load the private/public key from the secure store using
that identifier.
| mosip.kernel.keymanager.service.cose.privatekey=MC4CAQAwBQYDK2VwBCIEIBSRNSG9zqqQSmGWiHuI6vA3GkW6wMMFuxiupMX87JmP | ||
| mosip.kernel.keymanager.service.cose.publickey=MCowBQYDK2VwAyEAPvDN-FB2f50m3si16mEJF07X-Yn5yhyEC6jPE0D3aOE |
There was a problem hiding this comment.
🚨 Private key material committed to the repository — rotate immediately.
mosip.kernel.keymanager.service.cose.privatekey contains a PKCS#8-encoded Ed25519 private key committed in plain text. Once a private key is in version-control history it must be treated as compromised regardless of the branch or profile it appears in. This key should be revoked and regenerated, and the value should never be stored in source code — use a secrets manager, environment variable injection, or an HSM reference instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kernel/kernel-keymanager-service/src/main/resources/application-local1.properties`
around lines 197 - 198, The committed private key value for
mosip.kernel.keymanager.service.cose.privatekey (and the paired public key) must
be revoked and rotated immediately; remove the literal key values from source,
replace them with references (e.g., read from an environment variable, a secrets
manager/VAULT path, or an HSM key identifier) and update the
configuration-loading code to fetch the secret from that provider (use the
property names mosip.kernel.keymanager.service.cose.privatekey / .publickey as
the logical keys to map to the secret source). After replacing the values, purge
the exposed secret from version-control history (use git-filter-repo or BFG) and
notify/rotate any systems that trusted the old key. Ensure no plaintext private
keys remain in the repo and add a comment in config-template/example.properties
instructing developers to use the secret provider instead.
|
|
||
| ENTRYPOINT [ "./configure_start.sh" ] | ||
|
|
||
| CMD ["java","-jar", "-Dloader.path=${loader_path_env}", "-Dspring.cloud.config.label=${spring_config_label_env}","-Dspring.cloud.config.name=${spring_config_name_env}", "-Dspring.profiles.active=${active_profile_env}", "-Dspring.cloud.config.uri=${spring_config_url_env}", "./keys-algorithm-migrator.jar"] |
There was a problem hiding this comment.
CMD in exec form does not expand environment variables — application will fail at startup.
Docker's exec form (["cmd", "arg1", ...]) does not perform shell variable substitution. The literal strings ${loader_path_env}, ${spring_config_label_env}, etc. will be passed to java as-is, so Spring Boot will receive nonsensical config values.
Either switch to shell form or explicitly invoke a shell:
🐛 Proposed fix — use shell form
-CMD ["java","-jar", "-Dloader.path=${loader_path_env}", "-Dspring.cloud.config.label=${spring_config_label_env}","-Dspring.cloud.config.name=${spring_config_name_env}", "-Dspring.profiles.active=${active_profile_env}", "-Dspring.cloud.config.uri=${spring_config_url_env}", "./keys-algorithm-migrator.jar"]
+CMD java -jar -Dloader.path=${loader_path_env} -Dspring.cloud.config.label=${spring_config_label_env} -Dspring.cloud.config.name=${spring_config_name_env} -Dspring.profiles.active=${active_profile_env} -Dspring.cloud.config.uri=${spring_config_url_env} ./keys-algorithm-migrator.jar📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CMD ["java","-jar", "-Dloader.path=${loader_path_env}", "-Dspring.cloud.config.label=${spring_config_label_env}","-Dspring.cloud.config.name=${spring_config_name_env}", "-Dspring.profiles.active=${active_profile_env}", "-Dspring.cloud.config.uri=${spring_config_url_env}", "./keys-algorithm-migrator.jar"] | |
| CMD java -jar -Dloader.path=${loader_path_env} -Dspring.cloud.config.label=${spring_config_label_env} -Dspring.cloud.config.name=${spring_config_name_env} -Dspring.profiles.active=${active_profile_env} -Dspring.cloud.config.uri=${spring_config_url_env} ./keys-algorithm-migrator.jar |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kernel/keys-algorithm-migrator/Dockerfile` at line 96, The Dockerfile CMD
currently uses exec form which does not expand environment variables (the array
with "java" and the -D... args), so strings like ${loader_path_env} are passed
literally; change the CMD to a shell-invoked form so variables expand (either
use shell form or wrap the java invocation in sh -c) and ensure the same JVM
args (-Dloader.path, -Dspring.cloud.config.*, -Dspring.profiles.active) and the
./keys-algorithm-migrator.jar target are preserved; update the CMD line in the
Dockerfile where the current exec-form CMD is declared to use a shell invocation
instead.
There was a problem hiding this comment.
Actionable comments posted: 1
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/signature/service/impl/CoseSignatureServiceImpl.java (2)
184-199:⚠️ Potential issue | 🟡 MinorEnsure CWT outputs always include both CWT and COSE tags.
If
includeCoseTag=falsewhileisCwt=true, the method returns untagged COSE bytes, butcwtVerifyexpects an outer CWT tag and inner COSE tag. Consider forcing tags for CWT or rejecting such inputs.🛠️ Suggested adjustment
- if (!includeCoseTag) - return coseItem.encode(); - - CBORTaggedItem sign1Tagged = new CBORTaggedItem(SignatureConstant.COSE_SIGN1_TAG, coseItem); - - if (isCwt) { - CBORTaggedItem cwtTagged = new CBORTaggedItem(SignatureConstant.CWT_SIGN_TAG, sign1Tagged); - return cwtTagged.encode(); - } else { - return sign1Tagged.encode(); - } + if (isCwt) { + CBORTaggedItem sign1Tagged = new CBORTaggedItem(SignatureConstant.COSE_SIGN1_TAG, coseItem); + CBORTaggedItem cwtTagged = new CBORTaggedItem(SignatureConstant.CWT_SIGN_TAG, sign1Tagged); + return cwtTagged.encode(); + } + + if (!includeCoseTag) { + return coseItem.encode(); + } + + CBORTaggedItem sign1Tagged = new CBORTaggedItem(SignatureConstant.COSE_SIGN1_TAG, coseItem); + return sign1Tagged.encode();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/CoseSignatureServiceImpl.java` around lines 184 - 199, The method encodeTaggedCoseSign1 currently skips COSE tagging when includeCoseTag is false which breaks callers that expect a CWT envelope (e.g., cwtVerify). Change encodeTaggedCoseSign1 so that when isCwt is true it always produces both tags: create a CBORTaggedItem for the COSE_SIGN1 (sign1Tagged) regardless of includeCoseTag and then wrap it in the CWT_SIGN_TAG (cwtTagged) before encoding; only skip the outer/inner tag when isCwt is false and includeCoseTag is false, or alternatively throw an IllegalArgumentException if the caller requests isCwt=true but includeCoseTag=false—implement the forced-tag behavior in encodeTaggedCoseSign1 to ensure cwtVerify receives a CWT with an inner COSE tag.
647-665:⚠️ Potential issue | 🟠 MajorGuard against non-tagged CBOR in
parseTaggedCoseSign1.At Line 652, a non‑tagged CBOR item will throw
ClassCastException, which is not handled here and bubbles as a generic verification failure. Add aninstanceofcheck (or catchClassCastException) and map it to the intended invalid‑input error path.🔧 Proposed fix
- cborTaggedItem = (CBORTaggedItem) cborDecoder.next(); + CBORItem item = cborDecoder.next(); + if (!(item instanceof CBORTaggedItem)) { + LOGGER.error(SignatureConstant.SESSIONID, SignatureConstant.COSE_VERIFY, SignatureConstant.BLANK, + "Provided COSE data is not a tagged item."); + throw new RequestException(SignatureErrorCode.INVALID_COSE_SIGN1_INPUT.getErrorCode(), + SignatureErrorCode.INVALID_COSE_SIGN1_INPUT.getErrorMessage()); + } + cborTaggedItem = (CBORTaggedItem) item;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/CoseSignatureServiceImpl.java` around lines 647 - 665, The method parseTaggedCoseSign1 currently casts cborDecoder.next() to CBORTaggedItem without checking type, which can cause a ClassCastException; update the code to first assign the result of cborDecoder.next() to an Object (or a generic CBORItem), verify it is an instance of CBORTaggedItem before casting (or catch ClassCastException), and if it is not, log the same invalid-input error and throw the RequestException using SignatureErrorCode.INVALID_COSE_SIGN1_INPUT (mirroring the existing invalid-tag path); keep existing log messages/context (SESSIONID, COSE_VERIFY) and return the tag content only after the instanceof check passes so parseTaggedCoseSign1 and CBORTaggedItem handling are guarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/dto/CoseSignVerifyRequestDto.java`:
- Around line 59-64: The class CoseSignVerifyRequestDto contains a duplicate
field declaration for isCOSETagIncluded which causes a compile error; remove the
redundant declaration (the one between the Javadoc and `@ApiModelProperty` shown
in the diff) so only a single isCOSETagIncluded field remains in
CoseSignVerifyRequestDto, or if a distinct flag was intended, rename the
duplicate to the correct unique field name and update usages accordingly (search
for isCOSETagIncluded references and adjust to the chosen single field).
---
Outside diff comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/CoseSignatureServiceImpl.java`:
- Around line 184-199: The method encodeTaggedCoseSign1 currently skips COSE
tagging when includeCoseTag is false which breaks callers that expect a CWT
envelope (e.g., cwtVerify). Change encodeTaggedCoseSign1 so that when isCwt is
true it always produces both tags: create a CBORTaggedItem for the COSE_SIGN1
(sign1Tagged) regardless of includeCoseTag and then wrap it in the CWT_SIGN_TAG
(cwtTagged) before encoding; only skip the outer/inner tag when isCwt is false
and includeCoseTag is false, or alternatively throw an IllegalArgumentException
if the caller requests isCwt=true but includeCoseTag=false—implement the
forced-tag behavior in encodeTaggedCoseSign1 to ensure cwtVerify receives a CWT
with an inner COSE tag.
- Around line 647-665: The method parseTaggedCoseSign1 currently casts
cborDecoder.next() to CBORTaggedItem without checking type, which can cause a
ClassCastException; update the code to first assign the result of
cborDecoder.next() to an Object (or a generic CBORItem), verify it is an
instance of CBORTaggedItem before casting (or catch ClassCastException), and if
it is not, log the same invalid-input error and throw the RequestException using
SignatureErrorCode.INVALID_COSE_SIGN1_INPUT (mirroring the existing invalid-tag
path); keep existing log messages/context (SESSIONID, COSE_VERIFY) and return
the tag content only after the instanceof check passes so parseTaggedCoseSign1
and CBORTaggedItem handling are guarded.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureErrorCode.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/dto/CoseSignVerifyRequestDto.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/CoseSignatureServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/constant/SignatureErrorCode.java
|
|
||
| /** | ||
| * Flag to validate the COSE TAG. | ||
| */ | ||
| @ApiModelProperty(notes = "Flag to validate the COSE TAG.", example = "true", required = false) | ||
| private Boolean isCOSETagIncluded; |
There was a problem hiding this comment.
Remove duplicate isCOSETagIncluded field (compile error).
This redeclares an existing field (Line 47) and breaks compilation. Keep a single declaration (or rename if a different flag was intended).
🛠️ Proposed fix (remove duplicate)
- /**
- * Flag to validate the COSE TAG.
- */
- `@ApiModelProperty`(notes = "Flag to validate the COSE TAG.", example = "true", required = false)
- private Boolean isCOSETagIncluded;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Flag to validate the COSE TAG. | |
| */ | |
| @ApiModelProperty(notes = "Flag to validate the COSE TAG.", example = "true", required = false) | |
| private Boolean isCOSETagIncluded; |
🧰 Tools
🪛 GitHub Actions: Maven Package upon a push
[error] 64-64: variable isCOSETagIncluded is already defined in class io.mosip.kernel.signature.dto.CoseSignVerifyRequestDto
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/dto/CoseSignVerifyRequestDto.java`
around lines 59 - 64, The class CoseSignVerifyRequestDto contains a duplicate
field declaration for isCOSETagIncluded which causes a compile error; remove the
redundant declaration (the one between the Javadoc and `@ApiModelProperty` shown
in the diff) so only a single isCOSETagIncluded field remains in
CoseSignVerifyRequestDto, or if a distinct flag was intended, rename the
duplicate to the correct unique field name and update usages accordingly (search
for isCOSETagIncluded references and adjust to the chosen single field).
1 participant
Summary by CodeRabbit