#523: Enable keymanager to support ECC algorithm during encryption and decryption#578
Open
nagendra0721 wants to merge 5 commits into
Open
#523: Enable keymanager to support ECC algorithm during encryption and decryption#578nagendra0721 wants to merge 5 commits into
nagendra0721 wants to merge 5 commits into
Conversation
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
…on and decryption Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
WalkthroughAdds end-to-end EC asymmetric cryptography (ECDH hybrid encryption/decryption with AES-GCM, HKDF key derivation) across the keymanager service. Introduces ChangesEnd-to-end EC cryptography enablement
Sequence Diagram(s)sequenceDiagram
participant Client
participant CryptomanagerServiceImpl
participant CryptomanagerUtils
participant EcCryptoOperationImpl
participant CryptoCore
rect rgba(70, 130, 180, 0.5)
note over Client,CryptoCore: Encrypt Path
Client->>CryptomanagerServiceImpl: encrypt(request)
CryptomanagerServiceImpl->>CryptomanagerUtils: getCertificate(appId, refId)
CryptomanagerUtils-->>CryptomanagerServiceImpl: X509Certificate
alt EC public key
CryptomanagerServiceImpl->>CryptomanagerUtils: getHeaderByte(ecCurveName)
CryptomanagerUtils-->>CryptomanagerServiceImpl: versionHeaderBytes
CryptomanagerServiceImpl->>EcCryptoOperationImpl: asymmetricEcEncrypt(publicKey, aad+data, ecCurveName)
EcCryptoOperationImpl-->>CryptomanagerServiceImpl: encryptedPayload
else RSA public key
CryptomanagerServiceImpl->>CryptoCore: symmetricEncrypt(data, aesKey)
CryptoCore-->>CryptomanagerServiceImpl: encryptedData
CryptomanagerServiceImpl->>CryptoCore: asymmetricEncrypt(aesKey, publicKey)
CryptoCore-->>CryptomanagerServiceImpl: wrappedKey
end
CryptomanagerServiceImpl-->>Client: base64url(thumbprint + header + payload)
end
rect rgba(60, 179, 113, 0.5)
note over Client,CryptoCore: Decrypt Path
Client->>CryptomanagerServiceImpl: decrypt(request)
CryptomanagerServiceImpl->>CryptomanagerUtils: getAlgorithmNameFromHeader(encryptedData)
CryptomanagerUtils-->>CryptomanagerServiceImpl: algorithmName
alt EC header
CryptomanagerServiceImpl->>CryptomanagerUtils: getEncryptedPrivateKey(appId, refId, thumbprint)
CryptomanagerUtils-->>CryptomanagerServiceImpl: privateKey
CryptomanagerServiceImpl->>EcCryptoOperationImpl: asymmetricEcDecrypt(privateKey, payload, aad, curveName)
EcCryptoOperationImpl-->>CryptomanagerServiceImpl: plaintext
else RSA header
CryptomanagerServiceImpl->>CryptoCore: decryptSymmetricKey(wrappedKey, privateKey)
CryptoCore-->>CryptomanagerServiceImpl: aesKey
CryptomanagerServiceImpl->>CryptoCore: symmetricDecrypt(encryptedData, aesKey)
CryptoCore-->>CryptomanagerServiceImpl: plaintext
end
CryptomanagerServiceImpl-->>Client: base64url(plaintext)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
nagendra0721
changed the title
![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 (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java (1)
473-480:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical bug: Case mismatch prevents EC key type detection.
The
ecCurvesListis populated with lowercase values (line 475:.toLowerCase()), but the check on line 479 useskeyType.toUpperCase(). This means thecontains()check will always returnfalsefor EC curves, preventing EC key generation.Example:
ecCurvesList = ["secp256r1", ...]butkeyType.toUpperCase() = "SECP256R1"→ no match.🐛 Proposed fix - normalize both to same case
private KeyPair generateKeyPair(String keyType) { List<String> ecCurvesList = Stream.of(ECCurves.values()).filter(value -> !value.name().equals(ECCurves.ED25519.name())) - .map(value -> value.toString().toLowerCase()) + .map(value -> value.name().toUpperCase()) .collect(Collectors.toList()); if (KeymanagerConstant.RSA_KEY_TYPE.equals(keyType)) return generateRSAKeyPair(); - else if (ecCurvesList.contains(keyType.toUpperCase())) + else if (ecCurvesList.contains(keyType.toUpperCase())) return generateECKeyPair(keyType);Or alternatively, normalize to lowercase consistently:
- else if (ecCurvesList.contains(keyType.toUpperCase())) + else if (ecCurvesList.contains(keyType.toLowerCase()))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java` around lines 473 - 480, In the generateKeyPair method, there is a case mismatch causing EC key detection to fail. The ecCurvesList is populated with lowercase values (via the toLowerCase() call), but the contains() check compares keyType.toUpperCase() against this lowercase list, which will never match. Fix this by normalizing the case consistently: either convert the ecCurvesList to uppercase values and keep the keyType.toUpperCase() comparison, or change the comparison to use keyType.toLowerCase() to match against the lowercase ecCurvesList. Choose one approach and apply it uniformly throughout the condition check.
🧹 Nitpick comments (4)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java (1)
69-73: 💤 Low valueConsider consolidating duplicate RSA signing algorithm constant.
RSA_SIGN_ALGORITHMhas the same value as the existingSIGNATURE_ALGORITHM(line 25). If they serve the same purpose, consolidate them to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java` around lines 69 - 73, The constant RSA_SIGN_ALGORITHM in the KeymanagerConstant class has the same value as the existing SIGNATURE_ALGORITHM constant, creating a duplicate. To fix this, remove the RSA_SIGN_ALGORITHM constant definition and replace all references to RSA_SIGN_ALGORITHM throughout the codebase with SIGNATURE_ALGORITHM to consolidate them and prevent future drift between the two constants.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java (1)
63-63: 💤 Low valueRedundant import.
java.lang.*is implicitly imported in all Java files and does not need to be explicitly imported.🧹 Suggested fix
-import java.lang.*;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java` at line 63, The import statement `import java.lang.*;` is redundant because the java.lang package is implicitly imported in all Java files by default. Remove this explicit import statement from the SignatureUtil.java file to clean up the imports and follow Java conventions.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/util/KeyGeneratorUtils.java (1)
22-22: 💤 Low valueUnused import detected.
The
@Valueimport from Spring is not used in this utility class since all methods are static and there's no Spring bean injection.🧹 Suggested fix
-import org.springframework.beans.factory.annotation.Value;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/util/KeyGeneratorUtils.java` at line 22, The import statement for `@Value` annotation from org.springframework.beans.factory.annotation is not being used in the KeyGeneratorUtils utility class, as there are no instance fields or Spring bean injections in the file. Remove the unused import statement on line 22 to clean up the imports and reduce unnecessary dependencies.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (1)
41-41: 💤 Low valueUnused import.
The
@Valueimport is not used in this static utility class.🧹 Suggested fix
-import org.springframework.beans.factory.annotation.Value;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java` at line 41, Remove the unused import statement for `org.springframework.beans.factory.annotation.Value` from the CertificateUtility class. Since this is a static utility class and does not use Spring's `@Value` annotation for dependency injection anywhere in the code, this import is unnecessary and should be deleted to keep the imports clean and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java`:
- Around line 368-370: The call to cryptomanagerUtil.getEncryptedPrivateKey in
CryptomanagerServiceImpl is passing three arguments
(cryptoRequestDto.getApplicationId(),
Optional.ofNullable(cryptoRequestDto.getReferenceId()), and certThumbprintHex)
but the method signature in CryptomanagerUtils only accepts two parameters.
Either remove the certThumbprintHex argument from the method call if the
thumbprint lookup is handled internally within getEncryptedPrivateKey, or update
the getEncryptedPrivateKey method signature in CryptomanagerUtils to accept the
additional certThumbprintHex parameter to match the intended usage.
- Around line 376-379: The EC decryption branch in the method creates and
populates a CryptomanagerResponseDto object by calling setData with
CryptoUtil.encodeToURLSafeBase64(decryptedData), but the method never returns
this object. Add a return statement for cryptoResponseDto immediately after the
setData call and before the closing braces to ensure the method returns the
populated response object as required by the method signature.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.java`:
- Around line 99-105: Replace the delimiter-based framing using keySplitter with
length-prefixed encoding to eliminate binary collision risks. Instead of
concatenating encryptedDataWithIv, keySplitterBytes, and ephemeralPublicKey with
a text delimiter, prepend the length of each component as a fixed-size integer
prefix (e.g., 4 bytes per length field). During encryption in the
System.arraycopy operations, write component lengths before their data. During
decryption (lines 142-147), read the length prefixes to determine exact
boundaries for each component rather than scanning for the keySplitter index.
This ensures the decrypt path correctly identifies component boundaries
regardless of byte patterns in the ciphertext or IV.
- Around line 125-129: The second destroyKey call on
ephemeralKeyPair.getPublic().getEncoded() in the finally block is missing a null
check guard; wrap this call with an if condition checking ephemeralKeyPair !=
null before dereferencing it, similar to the guard already present for the
private key destruction, to prevent NullPointerException if key-pair generation
fails earlier.
- Around line 66-69: The EcCryptoOperationImpl class has hard-coded algorithm
names that prevent X25519 support. First, remove the unused algorithmName
parameter from the asymmetricEcDecrypt method signature around line 134. Then,
replace the hard-coded algorithm strings throughout the class:
KeyPairGenerator.getInstance(EC_ALGORITHM) at line 67,
KeyAgreement.getInstance(ECDH) around lines 154-160, and
KeyFactory.getInstance(EC_ALGORITHM) need to be made dynamic based on the
curveName parameter. Create a helper method to determine the correct algorithm
(return "XDH" when curveName is X25519, otherwise return "EC" for standard
elliptic curves) and apply this logic consistently across all KeyPairGenerator,
KeyAgreement, and KeyFactory instantiation calls in the encryption and
decryption methods so X25519 routing via VERSION_EC_X25519 headers can function
properly.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java`:
- Around line 458-463: In the getEncryptedPrivateKey method, the Optional
parameter refId is being accessed with refId.get() without first verifying that
it contains a value, which will throw a NoSuchElementException if refId is
empty. You need to check if refId.isPresent() before calling refId.get() on the
parameter, and handle the case where refId is not present appropriately (either
return early, throw a meaningful exception, or use a default value as needed by
the method logic).
- Around line 109-114: The CryptomanagerUtils class has duplicate field
injections that inject the same configuration properties into different
variables, creating maintenance confusion. Remove the duplicate fields
`signApplicationid` and `certificateSignRefID` (lines 111 and 114) and instead
use the already-existing fields `signApplicationId` and `signRefId` throughout
the class. Update any references to the duplicate field names to use the
corresponding existing field names to ensure consistency and reduce code
duplication.
- Around line 533-538: In the try-catch block within the method that generates a
private key using KeyFactory, replace the raw RuntimeException with
KeymanagerServiceException. When catching InvalidKeySpecException or
NoSuchAlgorithmException, instantiate KeymanagerServiceException with a
descriptive error message indicating the key generation failure and pass the
caught exception as the cause using the appropriate constructor. This ensures
consistency with other error handling patterns in the CryptomanagerUtils class
and preserves the error context for proper debugging.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/KeyGenerator.java`:
- Around line 104-107: The getECKeyPair() method passes asymmetricKeyAlgorithm
to KeyGeneratorUtils.getECKeyPairGenerator(), but asymmetricKeyAlgorithm is
typically set to "RSA" which causes an InvalidAlgorithmParameterException when
the RSA generator is initialized with ECGenParameterSpec. Instead of passing
asymmetricKeyAlgorithm, hardcode the string "EC" directly in the getECKeyPair()
method call to KeyGeneratorUtils.getECKeyPairGenerator(), following the same
pattern used by getEd25519KeyPair() which uses its own dedicated algorithm
configuration.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java`:
- Around line 110-115: The private helper methods for generateX509Certificate
(at lines 124 and 147) are ignoring the signAlgorithm parameter that is passed
to them and instead always deriving the algorithm by calling
getSignatureAlgorithm(signPrivateKey). This causes all callers including
PKCS12KeyStoreImpl, PKCS11KeyStoreImpl, KeymanagerServiceImpl, and
PartnerCertificateManagerServiceImpl to have their explicit signature algorithm
specifications silently ignored. Fix this by modifying the private
generateX509Certificate helper methods to use the signAlgorithm parameter
directly instead of always calling getSignatureAlgorithm(signPrivateKey),
ensuring the caller's requested algorithm is respected throughout the call
chain.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/helper/SessionKeyDecryptorHelper.java`:
- Around line 378-383: The KeyFactory is being instantiated with
masterPrivateKey.getAlgorithm(), but masterPrivateKey is only the wrapping key
used for decryption. The decryptedPrivateKey bytes actually belong to the key
stored in dbKeyStore and may use a different algorithm (e.g., EC instead of
RSA). Fix this by extracting the algorithm from the certificate object that was
just created from dbKeyStore.get().getCertificateData(), since the certificate's
public key corresponds to the decrypted private key. Use the certificate's
public key algorithm when creating the KeyFactory instance instead of the master
key's algorithm.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java`:
- Around line 258-262: The masterKeyAlgorithm comparison in the if condition is
case-sensitive, so values configured as `rsa` (lowercase) or with extra
whitespace will not match KeymanagerConstant.RSA and will incorrectly branch to
EC key generation. In the condition that checks `if
(!masterKeyAlgorithm.trim().equals(KeymanagerConstant.RSA))`, normalize
masterKeyAlgorithm to uppercase by converting it to upper case before the
comparison. Apply this same normalization fix to all occurrences of this check
in the KeymanagerServiceImpl class, including the branches at lines 387-389 and
747-751.
- Line 25: Replace the import statement for SignatureUtil in
KeymanagerServiceImpl.java from the SPI interface package
(io.mosip.kernel.core.signatureutil.spi.SignatureUtil) to the implementation
class package (io.mosip.kernel.signature.util.SignatureUtil), since the
convertHexToBase64 method called at lines 1342 and 1362 is only defined in the
implementation class, not in the SPI interface.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java`:
- Around line 809-824: The getEcCurveName method contains an unsafe cast of the
result from subjectPublicKeyInfo.getAlgorithm().getParameters() directly to
ASN1ObjectIdentifier without validating that it is not null or that it is
actually of the correct type. This can cause a ClassCastException or
NullPointerException for certain EC key encodings. Add null checking and type
validation (using instanceof) before performing the cast to
ASN1ObjectIdentifier. If the parameters are null or not of the expected type,
throw an appropriate exception with a descriptive error message.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java`:
- Around line 294-298: The code now supports creating both RSA and EC temporary
migration keys in the key generation logic (lines 294-298), but the
migrateZKKeys method still only uses the RSA decryption path via
cryptoCore.asymmetricDecrypt, making EC temporary certificates unusable for
migration. Update the migrateZKKeys method (and the other location at lines
392-398 mentioned in the comment) to detect whether the temporary certificate is
RSA or EC and call the appropriate asymmetric decryption method for each case,
ensuring both key types are properly supported for unwrapping incoming random
keys during migration.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java`:
- Around line 413-421: Extract the algorithm selection logic from the sign()
method (lines 415-422) into a private helper method that takes referenceId and
x509Certificate as parameters and returns the resolved algorithm string. This
helper should contain the conditional logic that checks for empty or default
reference IDs to derive from the certificate via
SignatureUtil.getJwtSignAlgorithm(), otherwise looks up in
JWT_SIGNATURE_ALGO_IDENT with null and blank checks, defaulting to
RSA_USING_SHA256. Call this new helper method from both the sign() method at
line 414 and from signV2() at line 1102 where it currently calls
JWT_SIGNATURE_ALGO_IDENT.get(referenceId) to ensure consistent algorithm
resolution behavior for default signing references across both code paths.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java`:
- Around line 616-636: The getJwtSignAlgorithm method at line 622 performs an
unsafe direct cast of getParameters() to ASN1ObjectIdentifier without
validation, which can throw a ClassCastException for EC certificates with
non-standard parameter encodings. Add defensive checks before the cast to verify
that getParameters() returns a non-null ASN1ObjectIdentifier instance, and
handle the case where it doesn't by either using a default algorithm or throwing
an appropriate exception with context. This should follow the same pattern used
in KeymanagerUtil.getEcCurveName to ensure consistency.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java`:
- Around line 518-527: After matching the thumbprint to find the correct
kyAlias, ensure the private key recovery in the EC branch uses the matched alias
consistently. Instead of calling cryptomanagerUtil.getEncryptedPrivateKey with
application ID and reference ID extracted from a separate keyAliasRepository
lookup, directly use the matched alias information (kyAlias or the values from
dbKeyStore) to recover the private key. This ensures the EC decryption path uses
the same thumbprint-matched key that was validated, preventing mismatches when
multiple configured references or rotated keys exist.
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/EcCryptoOperationTest.java`:
- Around line 22-25: The test method testAsymmetricEcEncrypt is empty and
contains no test logic or assertions, which means any EC crypto regressions
would go unnoticed. Implement this test to perform a roundtrip encryption and
decryption operation using EC crypto, verifying that data encrypted with EC can
be successfully decrypted and matches the original plaintext. Include
appropriate assertions to validate that the decrypted output equals the original
input, and handle any necessary test setup such as key generation or
initialization of the crypto operation utilities.
---
Outside diff comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java`:
- Around line 473-480: In the generateKeyPair method, there is a case mismatch
causing EC key detection to fail. The ecCurvesList is populated with lowercase
values (via the toLowerCase() call), but the contains() check compares
keyType.toUpperCase() against this lowercase list, which will never match. Fix
this by normalizing the case consistently: either convert the ecCurvesList to
uppercase values and keep the keyType.toUpperCase() comparison, or change the
comparison to use keyType.toLowerCase() to match against the lowercase
ecCurvesList. Choose one approach and apply it uniformly throughout the
condition check.
---
Nitpick comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/util/KeyGeneratorUtils.java`:
- Line 22: The import statement for `@Value` annotation from
org.springframework.beans.factory.annotation is not being used in the
KeyGeneratorUtils utility class, as there are no instance fields or Spring bean
injections in the file. Remove the unused import statement on line 22 to clean
up the imports and reduce unnecessary dependencies.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerConstant.java`:
- Around line 69-73: The constant RSA_SIGN_ALGORITHM in the KeymanagerConstant
class has the same value as the existing SIGNATURE_ALGORITHM constant, creating
a duplicate. To fix this, remove the RSA_SIGN_ALGORITHM constant definition and
replace all references to RSA_SIGN_ALGORITHM throughout the codebase with
SIGNATURE_ALGORITHM to consolidate them and prevent future drift between the two
constants.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java`:
- Line 41: Remove the unused import statement for
`org.springframework.beans.factory.annotation.Value` from the CertificateUtility
class. Since this is a static utility class and does not use Spring's `@Value`
annotation for dependency injection anywhere in the code, this import is
unnecessary and should be deleted to keep the imports clean and maintainable.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java`:
- Line 63: The import statement `import java.lang.*;` is redundant because the
java.lang package is implicitly imported in all Java files by default. Remove
this explicit import statement from the SignatureUtil.java file to clean up the
imports and follow Java conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 179e6284-1c73-4db8-a7ce-2c39f44a3470
📒 Files selected for processing (20)
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/EcCryptoOperation.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/EcCryptoOperationImpl.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/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/helper/SessionKeyDecryptorHelper.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/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/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/EcCryptoOperationTest.java
Comment on lines
+368
to
+370
| String certThumbprintHex = Hex.toHexString(thumbprint).toUpperCase(); | ||
| PrivateKey privateKey = (PrivateKey) cryptomanagerUtil.getEncryptedPrivateKey(cryptoRequestDto.getApplicationId(), | ||
| Optional.ofNullable(cryptoRequestDto.getReferenceId()), certThumbprintHex)[0]; |
There was a problem hiding this comment.
Compilation error: getEncryptedPrivateKey called with wrong argument count.
The method getEncryptedPrivateKey(String, Optional<String>) accepts 2 parameters, but 3 are passed here including certThumbprintHex. This causes the pipeline compilation failure.
Either:
- Remove the unused
certThumbprintHexargument if thumbprint lookup is handled internally, or - Update
CryptomanagerUtils.getEncryptedPrivateKeyto accept the thumbprint parameter
- PrivateKey privateKey = (PrivateKey) cryptomanagerUtil.getEncryptedPrivateKey(cryptoRequestDto.getApplicationId(),
- Optional.ofNullable(cryptoRequestDto.getReferenceId()), certThumbprintHex)[0];
+ PrivateKey privateKey = (PrivateKey) cryptomanagerUtil.getEncryptedPrivateKey(cryptoRequestDto.getApplicationId(),
+ Optional.ofNullable(cryptoRequestDto.getReferenceId()))[0];📝 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.
Suggested change
| String certThumbprintHex = Hex.toHexString(thumbprint).toUpperCase(); | |
| PrivateKey privateKey = (PrivateKey) cryptomanagerUtil.getEncryptedPrivateKey(cryptoRequestDto.getApplicationId(), | |
| Optional.ofNullable(cryptoRequestDto.getReferenceId()), certThumbprintHex)[0]; | |
| String certThumbprintHex = Hex.toHexString(thumbprint).toUpperCase(); | |
| PrivateKey privateKey = (PrivateKey) cryptomanagerUtil.getEncryptedPrivateKey(cryptoRequestDto.getApplicationId(), | |
| Optional.ofNullable(cryptoRequestDto.getReferenceId()))[0]; |
🧰 Tools
🪛 GitHub Actions: Maven Package upon a push / 3_build-kernel _ maven-build.txt
[error] 369-369: Compilation error: method getEncryptedPrivateKey in CryptomanagerUtils cannot be applied to given types. Required (String, Optional) but found (String, Optional, String).
🪛 GitHub Actions: Maven Package upon a push / build-kernel _ maven-build
[error] 369-369: Compilation error: method getEncryptedPrivateKey in CryptomanagerUtils cannot be applied to given types. Required: (String, Optional); found: (String, Optional, String). Actual and formal argument lists differ in length.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java`
around lines 368 - 370, The call to cryptomanagerUtil.getEncryptedPrivateKey in
CryptomanagerServiceImpl is passing three arguments
(cryptoRequestDto.getApplicationId(),
Optional.ofNullable(cryptoRequestDto.getReferenceId()), and certThumbprintHex)
but the method signature in CryptomanagerUtils only accepts two parameters.
Either remove the certThumbprintHex argument from the method call if the
thumbprint lookup is handled internally within getEncryptedPrivateKey, or update
the getEncryptedPrivateKey method signature in CryptomanagerUtils to accept the
additional certThumbprintHex parameter to match the intended usage.
Source: Pipeline failures
Comment on lines
+376
to
379
| CryptomanagerResponseDto cryptoResponseDto = new CryptomanagerResponseDto(); | ||
| cryptoResponseDto.setData(CryptoUtil.encodeToURLSafeBase64(decryptedData)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing return statement in EC decryption branch causes compilation error.
The EC decryption path creates cryptoResponseDto but never returns it. The method signature requires returning CryptomanagerResponseDto.
byte[] decryptedData = ecCryptoOperation.asymmetricEcDecrypt(privateKey, encryptedData, aad, ecCurveName);
CryptomanagerResponseDto cryptoResponseDto = new CryptomanagerResponseDto();
cryptoResponseDto.setData(CryptoUtil.encodeToURLSafeBase64(decryptedData));
+ return cryptoResponseDto;
}
}📝 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.
Suggested change
| CryptomanagerResponseDto cryptoResponseDto = new CryptomanagerResponseDto(); | |
| cryptoResponseDto.setData(CryptoUtil.encodeToURLSafeBase64(decryptedData)); | |
| } | |
| } | |
| CryptomanagerResponseDto cryptoResponseDto = new CryptomanagerResponseDto(); | |
| cryptoResponseDto.setData(CryptoUtil.encodeToURLSafeBase64(decryptedData)); | |
| return cryptoResponseDto; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java`
around lines 376 - 379, The EC decryption branch in the method creates and
populates a CryptomanagerResponseDto object by calling setData with
CryptoUtil.encodeToURLSafeBase64(decryptedData), but the method never returns
this object. Add a return statement for cryptoResponseDto immediately after the
setData call and before the closing braces to ensure the method returns the
populated response object as required by the method signature.
Comment on lines
+66
to
+69
| try { | ||
| KeyPairGenerator ephemeralKeyPairGen = KeyPairGenerator.getInstance(EC_ALGORITHM); | ||
| ECGenParameterSpec ecGenParameterSpec = new ECGenParameterSpec(curveName); | ||
| ephemeralKeyPairGen.initialize(ecGenParameterSpec); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify curve routing and hard-coded algorithm usage
rg -n -C2 'EC_X25519|VERSION_EC_X25519|getHeaderByte' kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager
rg -n -C2 'asymmetricEcDecrypt\(PrivateKey privateKey, byte\[] data, byte\[] aad, String algorithmName\)|\balgorithmName\b' kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.java
rg -n -C2 'KeyPairGenerator\.getInstance\(EC_ALGORITHM\)|KeyAgreement\.getInstance\(ECDH\)|KeyFactory\.getInstance\(EC_ALGORITHM\)' kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.javaRepository: mosip/keymanager
Length of output: 6610
🌐 Web query:
Does Java JCA support X25519 key generation/agreement through KeyPairGenerator.getInstance("EC")+ECGenParameterSpec("X25519"), or should X25519/XDH algorithms be used explicitly?
💡 Result:
In the Java Cryptography Architecture (JCA), X25519 key generation should be performed using the "XDH" algorithm, not the "EC" algorithm [1][2]. While some specialized providers (such as the Android Keystore) may support X25519 through the "EC" algorithm for specific platform integrations [3], standard Java SE implementations require the use of "XDH" to comply with the API introduced in JEP 324 [1]. To generate an X25519 key pair, you should use the following approach: 1. Obtain a KeyPairGenerator instance for "XDH" [1]. 2. Initialize it using a NamedParameterSpec set to "X25519" [1][4]. Example: KeyPairGenerator kpg = KeyPairGenerator.getInstance("XDH"); NamedParameterSpec paramSpec = new NamedParameterSpec("X25519"); kpg.initialize(paramSpec); KeyPair kp = kpg.generateKeyPair; Using KeyPairGenerator.getInstance("X25519") is also a supported shorthand in modern Java versions [1]. The "EC" algorithm and ECGenParameterSpec are intended for traditional Elliptic Curve cryptography (such as NIST curves) and should not be used for X25519 key agreement in standard JCA code [1][2][5].
Citations:
- 1: https://openjdk.org/jeps/324
- 2: https://bugs.openjdk.org/browse/JDK-8189806
- 3: https://android.googlesource.com/platform/cts/+/refs/heads/main/tests/tests/keystore/src/android/keystore/cts/Curve25519Test.java?autodive=0%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F
- 4: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/security/spec/NamedParameterSpec.html
- 5: https://docs.oracle.com/en/java/javase/25/docs/specs/security/standard-names.html
🏁 Script executed:
# Get full implementation of EcCryptoOperationImpl to check if algorithmName is used
head -200 kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.java | tail -150Repository: mosip/keymanager
Length of output: 8083
🏁 Script executed:
# Check if algorithmName is used anywhere in the asymmetricEcDecrypt method
grep -n "algorithmName" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.javaRepository: mosip/keymanager
Length of output: 174
🏁 Script executed:
# Get full file to understand context better
wc -l kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.javaRepository: mosip/keymanager
Length of output: 181
Remove unused algorithmName parameter and fix hard-coded EC/ECDH algorithm usage to support X25519.
The algorithmName parameter is unused in asymmetricEcDecrypt, and more critically, KeyPairGenerator.getInstance(EC_ALGORITHM), KeyAgreement.getInstance(ECDH), and KeyFactory.getInstance(EC_ALGORITHM) are hard-coded throughout the class. X25519 support is broken because Java JCA requires "XDH" algorithm for X25519 key generation and agreement, not "EC". The codebase has X25519 routing infrastructure with VERSION_EC_X25519 headers, but this implementation cannot honor it due to the hard-coded algorithm constraints.
This also applies to lines 134 (method signature), 154-160 (decryption's hard-coded algorithm instantiations), and the corresponding encryption methods that similarly ignore curve selection.
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 66-66: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: KeyPairGenerator.getInstance(EC_ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(desede-is-deprecated-java)
[warning] 66-66: 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)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(use-of-aes-ecb-java)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.java`
around lines 66 - 69, The EcCryptoOperationImpl class has hard-coded algorithm
names that prevent X25519 support. First, remove the unused algorithmName
parameter from the asymmetricEcDecrypt method signature around line 134. Then,
replace the hard-coded algorithm strings throughout the class:
KeyPairGenerator.getInstance(EC_ALGORITHM) at line 67,
KeyAgreement.getInstance(ECDH) around lines 154-160, and
KeyFactory.getInstance(EC_ALGORITHM) need to be made dynamic based on the
curveName parameter. Create a helper method to determine the correct algorithm
(return "XDH" when curveName is X25519, otherwise return "EC" for standard
elliptic curves) and apply this logic consistently across all KeyPairGenerator,
KeyAgreement, and KeyFactory instantiation calls in the encryption and
decryption methods so X25519 routing via VERSION_EC_X25519 headers can function
properly.
Comment on lines
+99
to
+105
| byte[] keySplitterBytes = keySplitter.getBytes(); | ||
| output = new byte[encryptedDataWithIv.length + keySplitterBytes.length + ephemeralPublicKey.length]; | ||
|
|
||
| System.arraycopy(encryptedDataWithIv, 0, output, 0, encryptedDataWithIv.length); | ||
| System.arraycopy(keySplitterBytes, 0, output, encryptedDataWithIv.length, keySplitterBytes.length); | ||
| System.arraycopy(ephemeralPublicKey, 0, output, encryptedDataWithIv.length + keySplitterBytes.length, ephemeralPublicKey.length); | ||
|
|
There was a problem hiding this comment.
Binary payload framing with keySplitter is collision-prone.
The encrypt path inserts a text delimiter into raw binary, and decrypt finds it by index scan. If ciphertext/IV bytes contain the same sequence, parsing breaks nondeterministically (wrong split, invalid key parse, decryption failures).
Protocol-hardening direction
- // output = encryptedDataWithIv + keySplitter + ephemeralPublicKey
+ // output = [4-byte encLen][encDataWithIv][4-byte ephKeyLen][ephPublicKey]
+ // Use explicit length-prefix framing to avoid delimiter collisions in binary payloads.Also applies to: 142-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.java`
around lines 99 - 105, Replace the delimiter-based framing using keySplitter
with length-prefixed encoding to eliminate binary collision risks. Instead of
concatenating encryptedDataWithIv, keySplitterBytes, and ephemeralPublicKey with
a text delimiter, prepend the length of each component as a fixed-size integer
prefix (e.g., 4 bytes per length field). During encryption in the
System.arraycopy operations, write component lengths before their data. During
decryption (lines 142-147), read the length prefixes to determine exact
boundaries for each component rather than scanning for the keySplitter index.
This ensures the decrypt path correctly identifies component boundaries
regardless of byte patterns in the ciphertext or IV.
Comment on lines
+125
to
+129
| if (ephemeralKeyPair != null) { | ||
| destroyKey(ephemeralKeyPair.getPrivate().getEncoded()); | ||
| } | ||
| if (ephemeralKeyPair.getPrivate() != null) destroyKey(ephemeralKeyPair.getPublic().getEncoded()); | ||
| } |
There was a problem hiding this comment.
Avoid NPE in finally when key-pair generation fails.
Line 128 dereferences ephemeralKeyPair without guarding ephemeralKeyPair != null; if curve init/generation fails earlier, this throws NullPointerException and masks the real crypto error.
Proposed fix
- if (ephemeralKeyPair != null) {
- destroyKey(ephemeralKeyPair.getPrivate().getEncoded());
- }
- if (ephemeralKeyPair.getPrivate() != null) destroyKey(ephemeralKeyPair.getPublic().getEncoded());
+ if (ephemeralKeyPair != null) {
+ if (ephemeralKeyPair.getPrivate() != null) {
+ destroyKey(ephemeralKeyPair.getPrivate().getEncoded());
+ }
+ if (ephemeralKeyPair.getPublic() != null) {
+ destroyKey(ephemeralKeyPair.getPublic().getEncoded());
+ }
+ }📝 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.
Suggested change
| if (ephemeralKeyPair != null) { | |
| destroyKey(ephemeralKeyPair.getPrivate().getEncoded()); | |
| } | |
| if (ephemeralKeyPair.getPrivate() != null) destroyKey(ephemeralKeyPair.getPublic().getEncoded()); | |
| } | |
| if (ephemeralKeyPair != null) { | |
| if (ephemeralKeyPair.getPrivate() != null) { | |
| destroyKey(ephemeralKeyPair.getPrivate().getEncoded()); | |
| } | |
| if (ephemeralKeyPair.getPublic() != null) { | |
| destroyKey(ephemeralKeyPair.getPublic().getEncoded()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptoOperationImpl.java`
around lines 125 - 129, The second destroyKey call on
ephemeralKeyPair.getPublic().getEncoded() in the finally block is missing a null
check guard; wrap this call with an if condition checking ephemeralKeyPair !=
null before dereferencing it, similar to the guard already present for the
private key destruction, to prevent NullPointerException if key-pair generation
fails earlier.
Comment on lines
+413
to
+421
| // Alg selection: for default SIGN/empty refID derive from actual cert (handles EC sign certs) | ||
| String algoString; | ||
| if (referenceId.equals(KeymanagerConstant.EMPTY) || referenceId.equals(certificateSignRefID)) { | ||
| algoString = SignatureUtil.getJwtSignAlgorithm(x509Certificate); | ||
| } else { | ||
| algoString = JWT_SIGNATURE_ALGO_IDENT.get(referenceId); | ||
| if (algoString == null || algoString.isBlank()) { | ||
| algoString = AlgorithmIdentifiers.RSA_USING_SHA256; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect JWT algorithm resolution and alg header assignment sites.
rg -n -C3 'JWT_SIGNATURE_ALGO_IDENT|getJwtSignAlgorithm|setAlgorithmHeaderValue|resolveJwtSignAlgorithm'Repository: mosip/keymanager
Length of output: 12965
🏁 Script executed:
#!/bin/bash
# Get certificateSignRefID initialization
rg -n "certificateSignRefID\s*=" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java
# Get the full signV2 method signature and context
rg -n -B5 -A20 'private.*String signV2' kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -60
# Check where signV2 is called from
rg -n "\.signV2\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -20Repository: mosip/keymanager
Length of output: 1304
🏁 Script executed:
#!/bin/bash
# Find where certificateSignRefID is defined/initialized
rg -n -B2 -A2 "certificateSignRefID" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java
# Also check the public methods that call sign vs signV2
rg -n "signV2\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -10Repository: mosip/keymanager
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Check the public method that calls sign/signV2 to see what referenceIds are passed
rg -n -B10 "signV2.*referenceId" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -50
# Check the sign method call to see if it gets the same referenceId
rg -n -B10 'sign.*referenceId' kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | grep -A10 "public.*sign" | head -30
# Get the full method signatures of the public entry points
ast-grep --pattern 'public $_($_) {
$$$
}' kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -100Repository: mosip/keymanager
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Find the public methods that call sign or signV2
rg -n "public.*sign" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -20
# Get the full context of the jwtSign method that calls sign
rg -n -B5 -A30 "private String sign\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -80
# Check what referenceIds are actually passed in when calling sign/signV2
rg -n "\.sign\(|\.signV2\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.javaRepository: mosip/keymanager
Length of output: 2944
🏁 Script executed:
#!/bin/bash
# Find where the private sign method is called
rg -n "\.sign\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | grep -v "//"
# Get the public signv2 method
rg -n -A50 "public SignResponseDto signv2\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -100
# Get the public sign method
rg -n -A50 "public SignatureResponse sign\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -100Repository: mosip/keymanager
Length of output: 6712
🏁 Script executed:
#!/bin/bash
# Find where private signV2 is called with referenceId
rg -n -B10 "String signedData = signV2\|signedData = signV2" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -60
# Also find all calls to private signV2 with context
rg -n "signV2\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | grep -v "public"
# Check the jwtSign method which likely calls private sign
rg -n -B5 -A30 "SignatureResponseDto jwtSign\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -80Repository: mosip/keymanager
Length of output: 2270
🏁 Script executed:
#!/bin/bash
# Get the full jwtSign method to see how signV2 is called
rg -n -A80 "public JWTSignatureResponseDto jwtSign\(" kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | head -150Repository: mosip/keymanager
Length of output: 4922
🏁 Script executed:
#!/bin/bash
# Get context around line 1038 to see what method calls signV2
sed -n '980,1120p' kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java | cat -nRepository: mosip/keymanager
Length of output: 7874
Extract JWT algorithm resolution into a shared helper to fix signV2 producing null alg headers for default signing references.
The sign() method handles default SIGN/empty reference IDs by deriving the algorithm from the certificate (lines 415–416), but signV2() skips this and looks up the raw referenceId in JWT_SIGNATURE_ALGO_IDENT (line 1102). Since the map has no entry for the default SIGN reference, it returns null and passes that to setAlgorithmHeaderValue() (line 1114), which can fail or produce invalid JWTs when using EC signing certificates.
Both methods can receive the same reference IDs—jwtSignV2() defaults to signRefid ("SIGN") just like jwtSign() does—so they should use the same resolver. Extract the algorithm selection logic into a private helper method and call it from both paths to ensure consistent behavior:
Suggested fix
+ private String resolveJwtSignAlgorithm(String referenceId, X509Certificate x509Certificate) {
+ if (KeymanagerConstant.EMPTY.equals(referenceId) || certificateSignRefID.equals(referenceId)) {
+ return SignatureUtil.getJwtSignAlgorithm(x509Certificate);
+ }
+ String algoString = JWT_SIGNATURE_ALGO_IDENT.get(referenceId);
+ return (algoString == null || algoString.isBlank())
+ ? AlgorithmIdentifiers.RSA_USING_SHA256
+ : algoString;
+ }Then in sign() at line 414–422:
- String algoString;
- if (referenceId.equals(KeymanagerConstant.EMPTY) || referenceId.equals(certificateSignRefID)) {
- algoString = SignatureUtil.getJwtSignAlgorithm(x509Certificate);
- } else {
- algoString = JWT_SIGNATURE_ALGO_IDENT.get(referenceId);
- if (algoString == null || algoString.isBlank()) {
- algoString = AlgorithmIdentifiers.RSA_USING_SHA256;
- }
- }
+ String algoString = resolveJwtSignAlgorithm(referenceId, x509Certificate);And in signV2() at line 1102:
- String algoString = JWT_SIGNATURE_ALGO_IDENT.get(referenceId);
+ String algoString = resolveJwtSignAlgorithm(referenceId, x509Certificate);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java`
around lines 413 - 421, Extract the algorithm selection logic from the sign()
method (lines 415-422) into a private helper method that takes referenceId and
x509Certificate as parameters and returns the resolved algorithm string. This
helper should contain the conditional logic that checks for empty or default
reference IDs to derive from the certificate via
SignatureUtil.getJwtSignAlgorithm(), otherwise looks up in
JWT_SIGNATURE_ALGO_IDENT with null and blank checks, defaulting to
RSA_USING_SHA256. Call this new helper method from both the sign() method at
line 414 and from signV2() at line 1102 where it currently calls
JWT_SIGNATURE_ALGO_IDENT.get(referenceId) to ensure consistent algorithm
resolution behavior for default signing references across both code paths.
Comment on lines
+616
to
+636
| public static String getJwtSignAlgorithm(X509Certificate x509Certificate) { | ||
| PublicKey publicKey = x509Certificate.getPublicKey(); | ||
| String algorithm = publicKey.getAlgorithm(); | ||
|
|
||
| if (KeymanagerConstant.EC_KEY_TYPE.equalsIgnoreCase(algorithm)) { | ||
| SubjectPublicKeyInfo subjectPublicKeyInfo = SubjectPublicKeyInfo.getInstance(publicKey.getEncoded()); | ||
| ASN1ObjectIdentifier curveOid = (ASN1ObjectIdentifier) subjectPublicKeyInfo.getAlgorithm().getParameters(); | ||
|
|
||
| return mapCurveOidToCurveName(curveOid.getId()); | ||
| } | ||
| return AlgorithmIdentifiers.RSA_USING_SHA256; | ||
| } | ||
|
|
||
| private static String mapCurveOidToCurveName(String oid) { | ||
| return switch (oid) { | ||
| case KeymanagerConstant.EC_SECP256R1_OID -> AlgorithmIdentifiers.ECDSA_USING_P256_CURVE_AND_SHA256; | ||
| case KeymanagerConstant.EC_SECP256K1_OID -> AlgorithmIdentifiers.ECDSA_USING_SECP256K1_CURVE_AND_SHA256; | ||
| default -> throw new io.mosip.kernel.core.exception.NoSuchAlgorithmException(KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorCode(), | ||
| KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorMessage()); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Unsafe cast may cause ClassCastException for certain EC certificates.
Similar to KeymanagerUtil.getEcCurveName, line 622 directly casts getParameters() to ASN1ObjectIdentifier. This could fail for EC certificates with non-standard parameter encodings.
🛡️ Proposed defensive fix
public static String getJwtSignAlgorithm(X509Certificate x509Certificate) {
PublicKey publicKey = x509Certificate.getPublicKey();
String algorithm = publicKey.getAlgorithm();
if (KeymanagerConstant.EC_KEY_TYPE.equalsIgnoreCase(algorithm)) {
SubjectPublicKeyInfo subjectPublicKeyInfo = SubjectPublicKeyInfo.getInstance(publicKey.getEncoded());
- ASN1ObjectIdentifier curveOid = (ASN1ObjectIdentifier) subjectPublicKeyInfo.getAlgorithm().getParameters();
+ Object params = subjectPublicKeyInfo.getAlgorithm().getParameters();
+ if (!(params instanceof ASN1ObjectIdentifier)) {
+ throw new io.mosip.kernel.core.exception.NoSuchAlgorithmException(
+ KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorCode(),
+ KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorMessage()
+ );
+ }
+ ASN1ObjectIdentifier curveOid = (ASN1ObjectIdentifier) params;
return mapCurveOidToCurveName(curveOid.getId());
}
return AlgorithmIdentifiers.RSA_USING_SHA256;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java`
around lines 616 - 636, The getJwtSignAlgorithm method at line 622 performs an
unsafe direct cast of getParameters() to ASN1ObjectIdentifier without
validation, which can throw a ClassCastException for EC certificates with
non-standard parameter encodings. Add defensive checks before the cast to verify
that getParameters() returns a non-null ASN1ObjectIdentifier instance, and
handle the case where it doesn't by either using a default algorithm or throwing
an appropriate exception with context. This should follow the same pattern used
in KeymanagerUtil.getEcCurveName to ensure consistency.
Comment on lines
+22
to
+25
| @Test | ||
| public void testAsymmetricEcEncrypt() { | ||
|
|
||
| } |
There was a problem hiding this comment.
testAsymmetricEcEncrypt is empty, so EC crypto regressions will pass unnoticed.
This new test currently exercises no behavior or assertions for the newly added EC encrypt/decrypt path.
Proposed minimal roundtrip test
+import java.nio.charset.StandardCharsets;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.spec.ECGenParameterSpec;
+import static org.junit.Assert.assertArrayEquals;
...
`@Test`
public void testAsymmetricEcEncrypt() {
-
+ String curve = "secp256r1";
+ byte[] plain = "ec-roundtrip".getBytes(StandardCharsets.UTF_8);
+ KeyPairGenerator kpg = KeyPairGenerator.getInstance("EC");
+ kpg.initialize(new ECGenParameterSpec(curve));
+ KeyPair kp = kpg.generateKeyPair();
+
+ byte[] encrypted = service.asymmetricEcEncrypt(kp.getPublic(), plain, curve);
+ byte[] decrypted = service.asymmetricEcDecrypt(kp.getPrivate(), encrypted, null, curve);
+ assertArrayEquals(plain, decrypted);
}📝 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.
Suggested change
| @Test | |
| public void testAsymmetricEcEncrypt() { | |
| } | |
| `@Test` | |
| public void testAsymmetricEcEncrypt() { | |
| String curve = "secp256r1"; | |
| byte[] plain = "ec-roundtrip".getBytes(StandardCharsets.UTF_8); | |
| KeyPairGenerator kpg = KeyPairGenerator.getInstance("EC"); | |
| kpg.initialize(new ECGenParameterSpec(curve)); | |
| KeyPair kp = kpg.generateKeyPair(); | |
| byte[] encrypted = service.asymmetricEcEncrypt(kp.getPublic(), plain, curve); | |
| byte[] decrypted = service.asymmetricEcDecrypt(kp.getPrivate(), encrypted, null, curve); | |
| assertArrayEquals(plain, decrypted); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/service/EcCryptoOperationTest.java`
around lines 22 - 25, The test method testAsymmetricEcEncrypt is empty and
contains no test logic or assertions, which means any EC crypto regressions
would go unnoticed. Implement this test to perform a roundtrip encryption and
decryption operation using EC crypto, verifying that data encrypted with EC can
be successfully decrypted and matches the original plaintext. Include
appropriate assertions to validate that the decrypted output equals the original
input, and handle any necessary test setup such as key generation or
initialization of the crypto operation utilities.
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: 1
🧹 Nitpick comments (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java (1)
401-405: ⚖️ Poor tradeoffECB mode usage is flagged by static analysis but is a pre-existing pattern.
The static analyzer correctly flags AES-ECB as weak (same ciphertext for same plaintext blocks, no integrity). However, this follows the existing
ZKCryptoManagerServiceImpl.doFinal()pattern for ZK random key wrapping—the new EC branch simply maintains consistency with the RSA branch and existing codebase.Changing the wrap algorithm would require a data migration strategy for existing wrapped keys. Consider tracking this as technical debt for a future security hardening effort if moving to AES-GCM or AES-KW is feasible.
Also applies to: 414-417
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java` around lines 401 - 405, The AES-ECB cipher initialization in the asymmetricDecrypt flow and the corresponding operation at lines 414-417 is flagged by static analysis as a security weakness. While this follows the existing ZKCryptoManagerServiceImpl.doFinal() pattern and maintains consistency with the RSA branch, add a suppression annotation (such as `@SuppressWarnings` with appropriate static analyzer code) or a clarifying comment above the Cipher.getInstance(aesECBTransformation) calls explaining that ECB mode is used for compatibility with existing wrapped key formats and that this should be addressed in a future security hardening effort when a data migration strategy for re-wrapping existing keys is implemented.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/integration/CryptographicServiceIntegrationTest.java`:
- Line 195: In the mock setup at line 195, the stubbed return value for
cryptomanagerUtil.parseEncryptKeyHeader(Mockito.any()) is set to
"RSA".getBytes(), which does not match the actual contract of the production
method. The production code expects version-header bytes (such as
VERSION_RSA_2048) and later performs exact comparisons against this header.
Replace the literal "RSA".getBytes() with the actual version-header bytes
constant that the production implementation of parseEncryptKeyHeader returns to
ensure the test properly validates the production behavior.
---
Nitpick comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java`:
- Around line 401-405: The AES-ECB cipher initialization in the
asymmetricDecrypt flow and the corresponding operation at lines 414-417 is
flagged by static analysis as a security weakness. While this follows the
existing ZKCryptoManagerServiceImpl.doFinal() pattern and maintains consistency
with the RSA branch, add a suppression annotation (such as `@SuppressWarnings`
with appropriate static analyzer code) or a clarifying comment above the
Cipher.getInstance(aesECBTransformation) calls explaining that ECB mode is used
for compatibility with existing wrapped key formats and that this should be
addressed in a future security hardening effort when a data migration strategy
for re-wrapping existing keys is implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3efa7eb-6eda-4ec1-a52f-9029cd75a55d
📒 Files selected for processing (7)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.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/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.javakernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/integration/CryptographicServiceIntegrationTest.javakernel/keys-migrator/src/main/java/io/mosip/kernel/migrate/impl/BaseKeysMigrator.java
🚧 Files skipped from review as they are similar to previous changes (4)
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java
| SymmetricKeyRequestDto symmetricKeyRequestDto = new SymmetricKeyRequestDto(appid, timeStamp, refid, data, true); | ||
| when(keyManagerService.decryptSymmetricKey(Mockito.any())).thenReturn(symmetricKeyResponseDto); | ||
| when(cryptomanagerUtil.parseEncryptKeyHeader(Mockito.any())).thenReturn("".getBytes()); | ||
| when(cryptomanagerUtil.parseEncryptKeyHeader(Mockito.any())).thenReturn("RSA".getBytes()); |
There was a problem hiding this comment.
Use real RSA header bytes in the mock, not "RSA" literal bytes.
At Line 195, the stubbed value does not match parseEncryptKeyHeader(...) contract. Production code returns version-header bytes (e.g., VERSION_RSA_2048) and later compares against that exact header; "RSA".getBytes() can make this test pass on a non-production path.
Suggested fix
- when(cryptomanagerUtil.parseEncryptKeyHeader(Mockito.any())).thenReturn("RSA".getBytes());
+ when(cryptomanagerUtil.parseEncryptKeyHeader(Mockito.any()))
+ .thenReturn(CryptomanagerConstant.VERSION_RSA_2048);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/integration/CryptographicServiceIntegrationTest.java`
at line 195, In the mock setup at line 195, the stubbed return value for
cryptomanagerUtil.parseEncryptKeyHeader(Mockito.any()) is set to
"RSA".getBytes(), which does not match the actual contract of the production
method. The production code expects version-header bytes (such as
VERSION_RSA_2048) and later performs exact comparisons against this header.
Replace the literal "RSA".getBytes() with the actual version-header bytes
constant that the production implementation of parseEncryptKeyHeader returns to
ensure the test properly validates the production behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[#523]
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores