MOSIP-36428: ecc algorithms encryption support#525
Open
nagendra0721 wants to merge 12 commits intomosip:developfrom
Open
MOSIP-36428: ecc algorithms encryption support#525nagendra0721 wants to merge 12 commits intomosip:developfrom
nagendra0721 wants to merge 12 commits intomosip:developfrom
Conversation
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
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:
WalkthroughAdds ECC (secp256r1/secp256k1) and X25519 support across the keymanager: new EcCryptomanagerService + impl, ECC-aware encrypt/decrypt and JWE/JWT branching, key generation/keystore updates for EC/X25519/Ed25519, new constants/error codes, controller/service API additions, tests, and a Keys Algorithm Migrator module. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CryptoSvc as CryptomanagerService
participant EC as EcCryptomanagerService
participant HSM as KeyManager/HSM
participant DB as Database
Note over Client,HSM: EC Encryption Flow
Client->>CryptoSvc: encrypt(data, recipientCert/publicKey)
activate CryptoSvc
CryptoSvc->>CryptoSvc: detect algorithm = ECC
CryptoSvc->>EC: asymmetricEcEncrypt(publicKey, data, curveName)
activate EC
EC->>EC: generate ephemeral key pair
EC->>EC: perform ECDH/XDH key agreement
EC->>EC: HKDF-SHA256 derive AES key
EC->>EC: AES-GCM encrypt with IV (+AAD)
EC-->>CryptoSvc: encryptedPayload (ciphertext + iv + ephemeralPubKey)
deactivate EC
CryptoSvc->>HSM: retrieve certificate thumbprint/metadata
CryptoSvc->>CryptoSvc: prepend header (algorithm, thumbprint)
CryptoSvc-->>Client: return finalEncryptedPayload
deactivate CryptoSvc
Note over Client,DB: EC Decryption Flow
Client->>CryptoSvc: decrypt(encryptedPayload)
activate CryptoSvc
CryptoSvc->>CryptoSvc: extract header -> algorithm = ECC, thumbprint
CryptoSvc->>HSM: retrieve private key by thumbprint
CryptoSvc->>EC: asymmetricEcDecrypt(privateKey, encryptedPayload, curveName)
activate EC
EC->>EC: extract ephemeralPubKey and IV
EC->>EC: ECDH/XDH with privateKey and ephemeralPubKey
EC->>EC: HKDF derive AES key
EC->>EC: AES-GCM decrypt -> plaintext
EC-->>CryptoSvc: plaintext
deactivate EC
CryptoSvc-->>Client: return plaintext
deactivate CryptoSvc
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: 12
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 (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java (1)
137-162:⚠️ Potential issue | 🔴 CriticalSAN-path private overload still uses the passed-in
signAlgorithmdirectly — ECC/Ed25519 certificate signing with SANs will use the wrong algorithm.Line 143 still reads
new JcaContentSignerBuilder(signAlgorithm). The non-SAN overload was correctly updated (line 119), but this overload was not. For EC or Ed25519 keys with Subject Alternative Names the caller-suppliedsignAlgorithm(often"SHA256withRSA"by default) will be used, causing anOperatorCreationExceptionor a mis-signed certificate.🐛 Proposed fix
- ContentSigner certContentSigner = new JcaContentSignerBuilder(signAlgorithm).setProvider(providerName).build(signPrivateKey); + ContentSigner certContentSigner = new JcaContentSignerBuilder(getSignatureAlgorithm(signPrivateKey)).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 137 - 162, The generateX509Certificate overload is still constructing the ContentSigner with the caller-supplied signAlgorithm (new JcaContentSignerBuilder(signAlgorithm)), which breaks ECC/Ed25519 signing when SANs are present; change it to choose the signing algorithm based on the signing key type instead of blindly using signAlgorithm: inside generateX509Certificate detect the signPrivateKey type (e.g., ECPrivateKey → use an ECDSA digest like "SHA256withECDSA", EdDSA/Ed25519 → "Ed25519", RSA → fall back to the provided signAlgorithm or an RSA default), then pass that resolved algorithm to JcaContentSignerBuilder.setProvider(providerName). Ensure this mirrors the approach used in the non-SAN overload so ECC/Ed25519 keys are signed with the correct algorithm.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keygenerator/bouncycastle/KeyGenerator.java (1)
47-48:⚠️ Potential issue | 🟡 MinorRemove unused
asymmetricKeyAlgorithmfield and its@Valueinjection.The field at line 48 is injected but never used in this class. The
getAsymmetricKey()method hardcodesKeymanagerConstant.RSAinstead, making the configuration property dead code. Remove the field declaration and@Valueannotation.🤖 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 47 - 48, Remove the unused injected field asymmetricKeyAlgorithm and its `@Value` annotation from the KeyGenerator class: locate the field declaration "private String asymmetricKeyAlgorithm" annotated with `@Value` and delete it, since getAsymmetricKey() currently uses KeymanagerConstant.RSA directly; ensure no other references to asymmetricKeyAlgorithm remain (search for the symbol) and run a quick compile to confirm no missing usages.
🟠 Major comments (22)
kernel/keys-algorithm-migrator/src/main/resources/application-local.properties-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorAvoid committing HSM credentials in plaintext config
Line 4 hardcodes a sensitive value (
keystore-pass=pin). Even in local profiles, this normalizes insecure secret handling and risks accidental reuse in non-local environments. Move this to environment/config-server secret injection.Suggested change
-mosip.kernel.keymanager.hsm.keystore-pass=pin +mosip.kernel.keymanager.hsm.keystore-pass=${MOSIP_HSM_KEYSTORE_PASS}🤖 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/application-local.properties` at line 4, The property mosip.kernel.keymanager.hsm.keystore-pass must not be hardcoded; remove the literal "pin" and replace it with a secret reference (e.g. an environment/config-server placeholder) so the keystore password is injected at runtime rather than stored in source. Update any code or configuration that reads mosip.kernel.keymanager.hsm.keystore-pass to accept the injected secret (environment variable or config-server secret) and ensure local runs use a developer-only secret store or .env excluded from VCS. Ensure no plaintext value remains for mosip.kernel.keymanager.hsm.keystore-pass in the repo.kernel/keys-algorithm-migrator/src/main/resources/bootstrap.properties-16-17 (1)
16-17:⚠️ Potential issue | 🟠 MajorReduce management endpoint exposure and health detail leakage
Lines 16–17 expose dangerous operational endpoints (
refresh,restart) and detailed health information with no authorization requirement. Spring Boot Actuator documentation explicitly warns thatrestartis disabled by default and should only be enabled with strong authentication and network isolation. Exposingrefreshrequires equivalent controls.
- Remove
refreshandrestartfrommanagement.endpoints.web.exposure.include(expose onlyinfoandhealth)- Change
management.endpoint.health.show-detailstowhen-authorizedto prevent unauthenticated leakage of health component detailsSuggested fix
- management.endpoint.health.show-details=always - management.endpoints.web.exposure.include=info,health,refresh,restart + management.endpoint.health.show-details=when-authorized + management.endpoints.web.exposure.include=info,health🤖 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 16 - 17, Update the Actuator configuration to avoid exposing sensitive endpoints and leaking health details: set management.endpoint.health.show-details to when-authorized (instead of always) and remove refresh and restart from management.endpoints.web.exposure.include so it only lists info and health; modify the properties named management.endpoint.health.show-details and management.endpoints.web.exposure.include accordingly and ensure any required endpoint access is protected via authentication/authorization.kernel/keys-algorithm-migrator/src/main/resources/bootstrap.properties-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorExternalize active profile instead of hardcoding
localin committed configLine 7 pins the application to local profile, creating deployment risk. If this jar runs in shared/dev/prod environments without proper CLI or environment variable overrides, it will always activate the local profile. Follow Spring Boot best practices: remove the hardcoded setting and set
spring.profiles.activeat deploy time via command-line args (e.g.,--spring.profiles.active=prod) or environment variables (higher precedence than config files).Suggested change
- spring.profiles.active=local + # Keep profile externalized (env/command-line), e.g. SPRING_PROFILES_ACTIVE🤖 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` at line 7, Remove the hardcoded profile by deleting or neutralizing the spring.profiles.active=local entry in bootstrap.properties so the app no longer forces the "local" profile at runtime; rely on external overrides instead (use --spring.profiles.active=<env> or the SPRING_PROFILES_ACTIVE env var at deploy time) and update any deployment docs or scripts that previously depended on that embedded setting.kernel/keys-algorithm-migrator/configure_start.sh-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorZIP path override is wired to a different env var name.
On Line 7, the script reads
zip_file_path, but this PR’s Dockerfile setshsm_zip_file_path. That makes runtime override brittle and can silently fall back to default path.Proposed fix
- [ -z "$zip_file_path" ] && zip_path="$DEFAULT_ZIP_PATH" || zip_path="$zip_file_path" + zip_path="${zip_file_path:-${hsm_zip_file_path:-$DEFAULT_ZIP_PATH}}"🤖 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` at line 7, The env var check in configure_start.sh uses zip_file_path but the Dockerfile sets hsm_zip_file_path, causing overrides to be ignored; update the conditional that assigns zip_path (currently referencing zip_file_path) to instead check hsm_zip_file_path (falling back to DEFAULT_ZIP_PATH if empty) so runtime overrides work correctly, and keep the zip_path variable name unchanged.kernel/keys-algorithm-migrator/configure_start.sh-18-24 (1)
18-24:⚠️ Potential issue | 🟠 MajorDownloaded installer is executed without integrity verification.
The script runs
install.shfrom a freshly downloaded ZIP without verifying artifact integrity. That creates a direct supply-chain execution risk.Proposed fix
wget "$artifactory_url_env/$zip_path" echo "Downloaded $artifactory_url_env/$zip_path" -unzip $FILE_NAME +if [ -n "${hsm_zip_sha256:-}" ]; then + echo "${hsm_zip_sha256} ${FILE_NAME}" | sha256sum -c - +fi + +unzip "$FILE_NAME" echo "Attempting to install" 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 18 - 24, The script currently downloads and immediately unzips/runs the installer (wget -> unzip -> cd $DIR_NAME && ./install.sh) without integrity checks; update the logic to fetch and verify the artifact before execution by downloading a checksum or signature alongside the ZIP (e.g. $zip_path.sha256 or $zip_path.asc), verify the downloaded FILE_NAME using sha256sum --check or gpg --verify, abort and log an error via non-zero exit if verification fails, only then unzip and run the installer in $DIR_NAME; reference FILE_NAME, DIR_NAME and install.sh when implementing the verification step and ensure failures prevent execution.kernel/keys-algorithm-migrator/Dockerfile-96-96 (1)
96-96:⚠️ Potential issue | 🟠 MajorExec-form CMD passes literal
${...}placeholders instead of expanding environment variables.Line 96 uses JSON-array CMD form, which does not invoke a shell. Environment variables like
${loader_path_env}are passed literally to the Java process instead of being expanded, breaking runtime configuration.Wrap the command with
sh -cto enable shell variable expansion:Proposed fix
-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 ["sh","-c","exec 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 CMD currently uses the exec/JSON form so placeholders like ${loader_path_env}, ${spring_config_label_env}, ${spring_config_name_env}, ${active_profile_env}, and ${spring_config_url_env} are passed literally to the JVM; change the Dockerfile CMD invocation for keys-algorithm-migrator.jar to run via a shell so environment variables are expanded (e.g., wrap the entire command with sh -c "<java ... ./keys-algorithm-migrator.jar>"), ensuring the same JVM options and -D properties are preserved and referenced by their existing names.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/util/SignatureUtil.java-612-616 (1)
612-616:⚠️ Potential issue | 🟠 MajorPotential
ClassCastExceptionif EC key uses explicit parameters instead of a named curve.Line 614 casts
subjectPublicKeyInfo.getAlgorithm().getParameters()directly toASN1ObjectIdentifier. If the EC key encodes explicit curve parameters (not a named curve OID),.getParameters()will return anASN1Sequence, causing aClassCastExceptionat runtime.Consider adding a type check or catching the cast failure.
🛡️ Proposed defensive fix
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)) { + throw new io.mosip.kernel.core.exception.NoSuchAlgorithmException( + KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorCode(), + KeymanagerErrorConstant.NOT_SUPPORTED_CURVE_VALUE.getErrorMessage()); + } + return mapCurveOidToCurveName(((ASN1ObjectIdentifier) params).getId());🤖 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/util/SignatureUtil.java` around lines 612 - 616, The code in SignatureUtil that handles EC public keys casts subjectPublicKeyInfo.getAlgorithm().getParameters() to ASN1ObjectIdentifier directly (used in the EC branch before calling mapCurveOidToCurveName), which can throw ClassCastException for keys with explicit EC parameters; modify the EC key handling in the method to first check the parameter object's type (e.g., instanceof ASN1ObjectIdentifier vs ASN1Sequence), and if it's an ASN1ObjectIdentifier extract the OID and call mapCurveOidToCurveName(curveOid.getId()), otherwise handle the explicit-parameters case by either parsing the ASN1Sequence to derive the curve name or throw a clear, descriptive exception/log (including the raw parameters) so callers can handle unsupported explicit-parameter keys.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymigrate/service/impl/KeyMigratorServiceImpl.java-401-416 (1)
401-416:⚠️ Potential issue | 🟠 MajorECB mode remains a cryptographic risk in both RSA and EC branches.
Line [401] and Line [414] use ECB-mode transformation, which lacks semantic security and integrity protection for repeated/structured inputs. For key wrapping/migration, move to an authenticated scheme (e.g., AES-GCM with IV/tag management, or a standard key-wrap construction).
🤖 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/keymigrate/service/impl/KeyMigratorServiceImpl.java` around lines 401 - 416, The code is using aesECBTransformation with Cipher.getInstance(...) and cipher.init(...)/cipher.doFinal(...) (seen around aesECBTransformation, zkMasterKey, Cipher.getInstance, cipher.init, cipher.doFinal in both the RSA and EC branches where ecCrypto.asymmetricEcDecrypt is used) which is insecure; replace ECB-based encryption with an authenticated key-wrap or AEAD mode (e.g., AES-GCM or RFC3394/AES Key Wrap): update the aesECBTransformation constant to an AEAD/key-wrap transformation, instantiate Cipher accordingly, generate and use a secure IV/nonce (or the key-wrap API) when encrypting the secretDataBytes, include the resulting IV/nonce and authentication tag (or wrapped output) alongside the ciphertext for storage/transfer, and update the corresponding decryption logic to extract IV/tag and perform authenticated verification before returning the plaintext; apply this change in both branches where Cipher.getInstance(...) / cipher.init(...)/cipher.doFinal(...) are called and ensure zkMasterKey usage matches the chosen AEAD/key-wrap API.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java-306-314 (1)
306-314:⚠️ Potential issue | 🟠 MajorDo not swallow all exceptions in
testGenerateSymmetricKeyForce.Line [310] catches
Exceptionand then only checks non-null, so the test passes even on unexpected failures.✅ Narrow the catch to expected infra failures only
- try { - SymmetricKeyGenerateResponseDto response = keymanagerService.generateSymmetricKey(requestDto); - Assert.assertNotNull(response); - Assert.assertEquals("Generation Success", response.getStatus()); - } catch (Exception e) { - // If KeystoreProcessing exception occurs, it might be due to HSM/keystore not being available in test - // In that case, we should still verify the method can be called - Assert.assertNotNull(e); - } + try { + SymmetricKeyGenerateResponseDto response = keymanagerService.generateSymmetricKey(requestDto); + Assert.assertNotNull(response); + Assert.assertEquals("Generation Success", response.getStatus()); + } catch (io.mosip.kernel.core.keymanager.exception.KeystoreProcessingException e) { + org.junit.Assume.assumeNoException("Keystore unavailable in this test environment", e); + }🤖 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/controller/KeymanagerControllerTest.java` around lines 306 - 314, In testGenerateSymmetricKeyForce inside KeymanagerControllerTest, don't catch java.lang.Exception broadly; instead catch only the expected infra-specific exceptions (e.g., KeystoreProcessingException or the HSM/keystore unavailability exception your code throws) around the call to keymanagerService.generateSymmetricKey(requestDto), assert details on that specific exception if caught, and let any other unexpected exceptions propagate (remove the generic catch or rethrow them) so test failures surface real bugs.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.java-424-439 (1)
424-439:⚠️ Potential issue | 🟠 MajorAvoid broad exception swallowing in
testGenerateSymmetricKey.Line [435] makes the test pass on any exception, which can hide real defects.
✅ Suggested tightening
- try { + try { SymmetricKeyGenerateResponseDto result = service.generateSymmetricKey(requestDto); Assert.assertEquals("Generation Success", result.getStatus()); @@ requestDto.setForce(false); SymmetricKeyGenerateResponseDto result2 = service.generateSymmetricKey(requestDto); Assert.assertEquals("Key Exists.", result2.getStatus()); - } catch (Exception e) { - // If KeystoreProcessing exception occurs, it might be due to HSM/keystore not being available in test - // In that case, we should still verify the method can be called - Assert.assertNotNull(e); + } catch (io.mosip.kernel.core.keymanager.exception.KeystoreProcessingException e) { + org.junit.Assume.assumeNoException("Keystore unavailable in this test environment", e); }🤖 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, The test testGenerateSymmetricKey currently swallows all Exceptions around calls to service.generateSymmetricKey; narrow the handling so real failures surface: remove the broad catch and let the test method declare throws Exception (or alternatively catch only the specific expected exception type such as KeystoreProcessingException) and in that specific catch assert the known expected behavior, but for any other exception rethrow or call Assert.fail(...) so unexpected exceptions fail the test; update references in KeymanagerServiceImplTest to replace the generic catch(Exception e) with the specific handling around service.generateSymmetricKey.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.java-694-714 (1)
694-714:⚠️ Potential issue | 🟠 Major
testGenerateKeyPairInHSMcurrently passes even on unexpected failures.Line [710] catches all exceptions, so this test can silently succeed during functional breakage.
✅ Suggested tightening
- } catch (Exception e) { - // If KeystoreProcessing exception occurs, it might be due to HSM/keystore not being available in test - // In that case, we should still verify the method can be called - Assert.assertNotNull(e); + } catch (io.mosip.kernel.core.keymanager.exception.KeystoreProcessingException e) { + org.junit.Assume.assumeNoException("Keystore unavailable in this test environment", e); }🤖 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 694 - 714, The test testGenerateKeyPairInHSM currently swallows all exceptions in a broad catch(Exception) which hides real failures; remove the generic try/catch (or replace it with a catch only for the expected KeystoreProcessingException), let unexpected exceptions propagate so the test fails, and if you must catch expected HSM/keystore errors, catch KeystoreProcessingException explicitly and assert its type/message; update the block around calls to service.generateMasterKey, service.getCertificate, service.generateECSignKey and updateKeyExpiry accordingly so only expected HSM-related exceptions are handled and all other exceptions are rethrown or allowed to fail the test.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java-127-130 (1)
127-130:⚠️ Potential issue | 🟠 MajorPotential
NullPointerException—providerNameis not null-checked before.equals("BC").If
providerNameisnull(e.g., not configured), calling.equals("BC")on it throws an NPE. Invert the comparison to use the string literal as the receiver.🛡️ Proposed fix (apply to both occurrences)
- 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, providerName is compared with "BC" using providerName.equals("BC") which can NPE if providerName is null; in CertificateUtility update both occurrences where JcaX509CertificateConverter is constructed (the branches returning new JcaX509CertificateConverter().setProvider(providerName).getCertificate(certHolder) vs new JcaX509CertificateConverter().getCertificate(certHolder)) to either invert the equals check to "BC".equals(providerName) or perform an explicit null-check on providerName before calling equals, so the logic safely selects setProvider(providerName) only when providerName is non-null and equals "BC".kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/MigrateKeysAlgorithmApplication.java-14-16 (2)
14-16:⚠️ Potential issue | 🟠 Major
scanBasePackageswith trailing.*is not a supported glob pattern — required beans may not be discovered.
scanBasePackagesallows wildcard string filtering, however the standard and well-documented form uses a plain package name such as"io.mosip.kernel.keygenerator", which automatically scans the configuration class package and all its sub-packages. Appending.*is an unusual pattern; the official API describesscanBasePackagessimply as "Base packages to scan for annotated components", withscanBasePackageClasses()as a type-safe alternative — there is no documented glob expansion inscanBasePackagesitself. Passing"io.mosip.kernel.keygenerator.*"would look for a package literally named with that suffix, which doesn't exist, causing beans inio.mosip.kernel.keygenerator.bouncycastleand sibling packages to be silently skipped and the migrator to fail at startup due to missing dependencies.Remove the trailing
.*from every entry:🐛 Proposed fix
-@SpringBootApplication(scanBasePackages = { "io.mosip.kernel.keygenerator.*", "io.mosip.kernel.keymanagerservice.*", - "io.mosip.kernel.keymanager.*", "io.mosip.kernel.crypto.*", "io.mosip.kernel.cryptomanager.*", - "io.mosip.kernel.migratealgorithm.*" }) +@SpringBootApplication(scanBasePackages = { "io.mosip.kernel.keygenerator", "io.mosip.kernel.keymanagerservice", + "io.mosip.kernel.keymanager", "io.mosip.kernel.crypto", "io.mosip.kernel.cryptomanager", + "io.mosip.kernel.migratealgorithm" })🤖 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 14 - 16, The scanBasePackages entries in the `@SpringBootApplication` on MigrateKeysAlgorithmApplication are using invalid trailing ".*" patterns which prevents component scanning from finding beans; remove the ".*" suffix from each package string (e.g., change "io.mosip.kernel.keygenerator.*" to "io.mosip.kernel.keygenerator") so Spring scans those base packages and their subpackages correctly; update all entries in the scanBasePackages array accordingly (or consider using scanBasePackageClasses for type-safe scanning).
14-16:⚠️ Potential issue | 🟠 Major
scanBasePackagesuses invalid pattern syntax — correct to standard package names.The
scanBasePackagesattribute accepts either plain package names or Ant-style patterns (e.g.,"io.mosip.kernel.**"). The current syntax"io.mosip.kernel.keygenerator.*"(single dot and asterisk) is not recognized as a valid pattern. Since Spring automatically recurses into sub-packages when given a plain package name, removing the.*suffix is the simplest fix.🐛 Proposed fix
-@SpringBootApplication(scanBasePackages = { "io.mosip.kernel.keygenerator.*", "io.mosip.kernel.keymanagerservice.*", - "io.mosip.kernel.keymanager.*", "io.mosip.kernel.crypto.*", "io.mosip.kernel.cryptomanager.*", - "io.mosip.kernel.migratealgorithm.*" }) +@SpringBootApplication(scanBasePackages = { "io.mosip.kernel.keygenerator", "io.mosip.kernel.keymanagerservice", + "io.mosip.kernel.keymanager", "io.mosip.kernel.crypto", "io.mosip.kernel.cryptomanager", + "io.mosip.kernel.migratealgorithm" })🤖 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 14 - 16, The scanBasePackages value in the `@SpringBootApplication` annotation on MigrateKeysAlgorithmApplication uses invalid pattern syntax like "io.mosip.kernel.keygenerator.*"; update the scanBasePackages attribute to use plain package names (e.g., "io.mosip.kernel.keygenerator", "io.mosip.kernel.keymanagerservice", "io.mosip.kernel.keymanager", "io.mosip.kernel.crypto", "io.mosip.kernel.cryptomanager", "io.mosip.kernel.migratealgorithm") so Spring can properly scan subpackages; locate the annotation on the MigrateKeysAlgorithmApplication class and replace each "package.*" entry with the corresponding plain package name.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java-366-377 (1)
366-377:⚠️ Potential issue | 🟠 MajorTests should not accept
NullPointerExceptionas a valid outcome.Line 372–Line 377 and Line 407–Line 412 currently allow NPE as a “pass” condition. This hides regressions and normalizes undefined behavior.
✅ Suggested fix
- } catch (ZKCryptoException e) { - // Expected - no matching thumbprint found, encRandomKey will be null - assertNotNull(e); - } catch (NullPointerException e) { - // NPE can occur if Objects.requireNonNull(kyAlias) is called with null - // This happens because the code accesses kyAlias before checking if encRandomKey is null - // This is acceptable as it indicates the expected failure path - assertNotNull(e); - } + } catch (ZKCryptoException e) { + assertNotNull(e); + } @@ - } catch (ZKCryptoException e) { - // Expected - empty keyAliases list, no match found, encRandomKey will be null - assertNotNull(e); - } catch (NullPointerException e) { - // NPE can occur if Objects.requireNonNull(kyAlias) is called with null - // This happens because the code accesses kyAlias before checking if encRandomKey is null - // This is acceptable as it indicates the expected failure path - assertNotNull(e); - } + } catch (ZKCryptoException e) { + assertNotNull(e); + }Also applies to: 401-412
🤖 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/zkcryptoservice/test/ZKCryptoManagerServiceTest.java` around lines 366 - 377, The test currently treats NullPointerException as an acceptable outcome; remove the catch for NullPointerException in ZKCryptoManagerServiceTest so NPEs no longer pass silently—either let NPE bubble (failing the test) or replace the NullPointerException catch with a fail() that reports an unexpected NPE; keep the existing expectation that zkReEncryptRandomKey(encryptedKey) should throw ZKCryptoException and only catch/assert that exception.kernel/keys-algorithm-migrator/src/main/java/io/mosip/kernel/migratealgorithm/impl/ComponentKeysAlgorithmMigrator.java-119-124 (1)
119-124:⚠️ Potential issue | 🟠 MajorGuard against null
keyAliasListbefore invalidation.Line 121 assumes
keyAliasListis non-null; if repository lookup returns noCURRENTKEYALIAS, migration fails with NPE and stops processing.✅ Suggested fix
private void invalidateKeyAlias(String appId, String refId, List<KeyAlias> keyAliasList, LocalDateTime timestamp) { + if (keyAliasList == null || keyAliasList.isEmpty()) { + LOGGER.info("No current key aliases to invalidate for AppId: " + appId + ", RefId: " + refId); + return; + } LocalDateTime expireTime = timestamp.minusMinutes(1L); keyAliasList.forEach(alias -> { dbHelper.storeKeyInAlias(appId, alias.getKeyGenerationTime(), refId, alias.getAlias(), expireTime, alias.getCertThumbprint(), alias.getUniqueIdentifier()); }); }🤖 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 119 - 124, The method invalidateKeyAlias currently assumes keyAliasList is non-null and will NPE if the repository returns no CURRENTKEYALIAS; update invalidateKeyAlias to guard against null/empty lists by returning early when keyAliasList == null || keyAliasList.isEmpty(), so the dbHelper.storeKeyInAlias loop is only executed when there are entries to invalidate; keep the existing expireTime computation and forEach logic unchanged otherwise.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java-618-622 (1)
618-622:⚠️ Potential issue | 🟠 MajorX25519 cannot be used for PKCS#10 CSR signing—remove it from signing algorithm paths.
Lines 618-622 and 641-643 incorrectly route X25519 (a key-agreement algorithm) to Ed25519 signing logic. Per RFC 8410, X25519 is limited to key agreement and cannot sign. The
generateCSR()method has a defensive check that blocks X25519, but thebuildResponseObject()CSR path (line 820-826 in KeymanagerServiceImpl) callsgetCSR()without validation, leaving a vulnerable code path.💡 Suggested fix
Add X25519 validation in
buildResponseObject()CSR path before callinggetCSR():if (responseObjectType.toUpperCase().equals(KeymanagerConstant.REQUEST_TYPE_CSR)) { LOGGER.info(KeymanagerConstant.SESSIONID, KeymanagerConstant.APPLICATIONID, appId, "Getting Key CSR for application ID: " + appId + ", RefId: " + refId); Optional<io.mosip.kernel.keymanagerservice.entity.KeyStore> keyFromDBStore = dbHelper.getKeyStoreFromDB(keyAlias); Object[] keyDetailsArr = getKeyDetails(keyFromDBStore, keyAlias); if (Objects.isNull(x509Cert)){ x509Cert = (X509Certificate) keyDetailsArr[1]; } + + if (x509Cert.getPublicKey().getAlgorithm().equals(KeymanagerConstant.X25519_KEY_TYPE)) { + throw new KeymanagerServiceException(KeymanagerErrorConstant.X25519_KEY_CSR_NOT_SUPPORTED.getErrorCode(), + KeymanagerErrorConstant.X25519_KEY_CSR_NOT_SUPPORTED.getErrorMessage()); + } + PublicKey publicKey = x509Cert.getPublicKey(); PrivateKey privateKey = (PrivateKey) keyDetailsArr[0]; KeyPairGenerateResponseDto responseDto = new KeyPairGenerateResponseDto();Additionally, remove X25519 from the signing algorithm selection in KeymanagerUtil:
// In getCSR() method (lines 618-622): - if (privateKeyAlgo.equals(KeymanagerConstant.ED25519_KEY_TYPE) || - privateKeyAlgo.equals(KeymanagerConstant.X25519_KEY_TYPE) || - keyAlgorithm.equals(KeymanagerConstant.X25519_KEY_TYPE)) { + if (privateKeyAlgo.equals(KeymanagerConstant.ED25519_KEY_TYPE)) { contentSigner = new JcaContentSignerBuilder(edSignAlgorithm).build(privateKey); // In getSignatureAlgorithm() method (lines 641-643): - else if (keyAlgorithm.equals(KeymanagerConstant.ED25519_KEY_TYPE) || - keyAlgorithm.equals(KeymanagerConstant.ED25519_ALG_OID) || - keyAlgorithm.equals(KeymanagerConstant.EDDSA_KEY_TYPE) || - keyAlgorithm.equals(KeymanagerConstant.X25519_KEY_TYPE)) + else if (keyAlgorithm.equals(KeymanagerConstant.ED25519_KEY_TYPE) || + keyAlgorithm.equals(KeymanagerConstant.ED25519_ALG_OID) || + keyAlgorithm.equals(KeymanagerConstant.EDDSA_KEY_TYPE)) return edSignAlgorithm;🤖 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/util/KeymanagerUtil.java` around lines 618 - 622, Remove X25519 from the signing-path in KeymanagerUtil by ensuring the condition that picks the Ed25519 signer (the block that checks privateKey.getAlgorithm()/privateKeyAlgo and uses JcaContentSignerBuilder(edSignAlgorithm)) does not treat KeymanagerConstant.X25519_KEY_TYPE as a signing key; only Ed25519 should map to edSignAlgorithm. Also add an explicit validation in the CSR-building path in KeymanagerServiceImpl (the method buildResponseObject()) before calling getCSR(): check the keyAlgorithm/privateKeyAlgo against KeymanagerConstant.X25519_KEY_TYPE and throw or return an error (same behavior as generateCSR()) if X25519 is detected so getCSR() is never invoked for X25519 keys. Ensure references to getCSR(), generateCSR(), KeymanagerUtil, and KeymanagerConstant.X25519_KEY_TYPE are used to locate the changes.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/SignatureServiceImpl.java-677-679 (1)
677-679:⚠️ Potential issue | 🟠 MajorDo not use
PrivateKey.getAlgorithm()as the JOSE signature algorithm.At Line 677 and Line 740, fallback uses key algorithm names (
RSA/EC/Ed25519), butSignatureProviderEnumresolves JOSE identifiers (PS256,ES256,ES256K,EdDSA). This can incorrectly fall back to PS256 for EC/Ed keys and fail signing.💡 Proposed fix
- String signAlgorithm = SignatureUtil.isDataValid(signatureReq.getSignAlgorithm()) - ? signatureReq.getSignAlgorithm() : certificateResponse.getCertificateEntry().getPrivateKey().getAlgorithm(); + String signAlgorithm = SignatureUtil.isDataValid(signatureReq.getSignAlgorithm()) + ? signatureReq.getSignAlgorithm() + : SignatureUtil.getJwtSignAlgorithm(certificateResponse.getCertificateEntry().getChain()[0]); @@ - String signatureAlgorithm = SignatureUtil.isDataValid(signatureReq.getSignAlgorithm()) - ? signatureReq.getSignAlgorithm() - : certificateResponse.getCertificateEntry().getPrivateKey().getAlgorithm(); + String signatureAlgorithm = SignatureUtil.isDataValid(signatureReq.getSignAlgorithm()) + ? signatureReq.getSignAlgorithm() + : SignatureUtil.getJwtSignAlgorithm(certificateResponse.getCertificateEntry().getChain()[0]);Also applies to: 740-743
🤖 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 677 - 679, The code falls back to using PrivateKey.getAlgorithm() (via certificateResponse.getCertificateEntry().getPrivateKey()) as the JOSE signature algorithm when signatureReq.getSignAlgorithm() is absent, which mixes key algorithm names (RSA/EC/Ed25519) with JOSE identifiers; update the fallback in SignatureServiceImpl so that if signatureReq.getSignAlgorithm() is invalid you map the PrivateKey type and curve to an appropriate JOSE algorithm (e.g., RSA->PS256/RS256 per your policy, EC with secp256r1->ES256, secp256k1->ES256K, Ed25519->EdDSA) using SignatureProviderEnum resolution logic rather than using privateKey.getAlgorithm() directly; apply the same change to the other fallback block around lines 740-743 and ensure you call SignatureProviderEnum (or a new helper) to produce a valid JOSE identifier for signing.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java-323-325 (1)
323-325:⚠️ Potential issue | 🟠 MajorUse the passed provider instead of hardcoding
"BC"for key reconstruction.Line 324 ignores the
providerargument and always uses BC. This can fail in deployments where BC is not registered or where provider consistency with keystore is required.💡 Proposed fix
- KeyFactory keyFactory = KeyFactory.getInstance(algo, BC_PROVIDER); + String keyFactoryProvider = (provider == null || provider.isBlank()) ? BC_PROVIDER : provider; + KeyFactory keyFactory = KeyFactory.getInstance(algo, keyFactoryProvider);🤖 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 323 - 325, The code is hardcoding the BouncyCastle provider when reconstructing the key; change KeyFactory.getInstance(algo, BC_PROVIDER) to use the method parameter/provider instance instead (e.g., KeyFactory.getInstance(algo, provider)) so the passed-in provider argument is used for key reconstruction in EcCryptomanagerServiceImpl (around the KeyFactory creation that precedes X509EncodedKeySpec ephemeralPublicKeyBytes).kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java-1503-1516 (1)
1503-1516:⚠️ Potential issue | 🟠 MajorRoute
RSA_2048_SIGNconsistently ingetCertificateV2.
getCertificate(...)treatsRSA_2048_SIGNas HSM-backed, butgetCertificateV2currently misses that in the HSM branch and may route to DB-store retrieval instead. This creates API inconsistency and wrong source selection for RSA signing keys.💡 Proposed fix
} 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()) - && ed25519SupportFlag)) { + && ed25519SupportFlag) || + (refId.isPresent() && refId.get().equals(KeyReferenceIdConsts.RSA_2048_SIGN.name()))) {🤖 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 1503 - 1516, getCertificateV2 incorrectly omits treating RSA_2048_SIGN as an HSM-backed signing key and thus can fall through to DB retrieval; modify the HSM-branch condition in getCertificateV2 (the same branch that calls getCertificateFromHSM) to also check for the RSA_2048_SIGN key reference (or the appropriate KeyReferenceIdConsts/RSA_2048_SIGN constant and certificateSignRefID like getCertificate does), so when appId equals signApplicationid and refId matches RSA_2048_SIGN (or refId.get().equals(corresponding enum name)) it routes to getCertificateFromHSM rather than getRSACertificateFromDBStore.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/EcCryptomanagerServiceImpl.java-153-160 (1)
153-160:⚠️ Potential issue | 🟠 MajorValidate payload bounds before IV/ciphertext extraction.
Line 158 assumes
encryptedData.length >= GCM_NONCE_LENGTH. Malformed input can throw runtimeArrayIndexOutOfBoundsExceptionand bypass your mapped crypto exceptions.💡 Proposed fix
int splitterIndex = 0; splitterIndex = CryptoUtil.getSplitterIndex(data, splitterIndex, keySplitter); + if (splitterIndex <= 0 || splitterIndex >= data.length) { + throw new InvalidKeyException( + SecurityExceptionCodeConstant.MOSIP_INVALID_KEY_EXCEPTION.getErrorCode(), + SecurityExceptionCodeConstant.MOSIP_INVALID_KEY_EXCEPTION.getErrorMessage()); + } @@ byte[] encryptedData = Arrays.copyOfRange(data, 0, splitterIndex); byte[] ephemeralPublicKeyBytes = Arrays.copyOfRange(data, splitterIndex + keySplitterBytes.length, data.length); + if (encryptedData.length <= CryptomanagerConstant.GCM_NONCE_LENGTH) { + throw new InvalidKeyException( + SecurityExceptionCodeConstant.MOSIP_INVALID_KEY_EXCEPTION.getErrorCode(), + SecurityExceptionCodeConstant.MOSIP_INVALID_KEY_EXCEPTION.getErrorMessage()); + }🤖 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 153 - 160, In EcCryptomanagerServiceImpl, before extracting IV and ciphertext from encryptedData (using CryptomanagerConstant.GCM_NONCE_LENGTH), validate that encryptedData.length >= GCM_NONCE_LENGTH + 1 (or at least >= GCM_NONCE_LENGTH) and that splitterIndex and keySplitterBytes boundaries produced a non-empty encryptedData; if the check fails, throw the mapped crypto exception used elsewhere (rather than letting ArrayIndexOutOfBoundsException escape). Update the logic around variables encryptedData, iv and cipherText to perform this bounds check and raise the proper domain exception with a clear message.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java-479-480 (1)
479-480:⚠️ Potential issue | 🟠 MajorAvoid logging the full DB key-store entity.
Line 479 logs
dbKeyStore.toString(), which can expose key/certificate metadata in logs. Prefer logging only minimal identifiers (alias/app/ref).💡 Proposed fix
- LOGGER.error(KeymanagerConstant.SESSIONID, KeymanagerConstant.KEYFROMDB, dbKeyStore.toString(), + LOGGER.error(KeymanagerConstant.SESSIONID, KeymanagerConstant.KEYFROMDB, dbKeyStore.getAlias(), "Key in DBStore does not exist for this alias. Throwing exception");🤖 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 479 - 480, The LOGGER.error call in CryptomanagerUtils is logging dbKeyStore.toString(), which may expose sensitive key/certificate metadata; update the call to remove the full entity and instead log only minimal non-sensitive identifiers (for example use dbKeyStore.getAlias(), dbKeyStore.getApplicationId()/getAppCode() and dbKeyStore.getReferenceId()/getRef() or the actual identifier accessor names in the DB entity) alongside KeymanagerConstant.SESSIONID and KeymanagerConstant.KEYFROMDB; ensure no key material or full toString() output is included and keep the message descriptive (e.g., "Key in DBStore does not exist for alias") while retaining existing exception flow in CryptomanagerUtils.
🟡 Minor comments (5)
kernel/keys-algorithm-migrator/pom.xml-200-200 (1)
200-200:⚠️ Potential issue | 🟡 MinorFix SCM connection URL typo.
Line 200 uses
mosip/keyanager.git; this should bemosip/keymanager.gitto keep SCM metadata valid.🔧 Proposed 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` at line 200, Update the SCM connection URL in the pom's <connection> element: replace the incorrect repository name "mosip/keyanager.git" with the correct "mosip/keymanager.git" so the SCM metadata reads "scm:git:git://github.com/mosip/keymanager.git".kernel/keys-algorithm-migrator/configure_start.sh-9-19 (1)
9-19:⚠️ Potential issue | 🟡 MinorFail fast if Artifactory URL is missing.
artifactory_url_envis consumed without validation. Add an explicit pre-check so startup fails with a clear message instead of an opaque wget error.Proposed fix
+ : "${artifactory_url_env:?artifactory_url_env must be set}" + echo "Download the client from $artifactory_url_env"🤖 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 9 - 19, The script uses artifactory_url_env without validation, so add a pre-check before any use (before the echo "Download the client..." and before building the wget URL) that verifies artifactory_url_env is set and non-empty; if it's missing, print a clear error to stderr and exit with a non-zero status so startup fails fast. Reference the environment variable artifactory_url_env and the wget invocation (wget "$artifactory_url_env/$zip_path") when adding the check; keep the existing FILE_NAME and DIR_NAME handling unchanged but ensure the check runs first.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/signature/service/impl/CoseSignatureServiceImpl.java-478-482 (1)
478-482:⚠️ Potential issue | 🟡 MinorMissing spaces in log messages produce unreadable output.
The string concatenation
"for appId:" + applicationId + "and refId:" + referenceIdwill produce output likefor appId:KERNELand refId:SIGN— note no space before "and" and after the appId value.🔧 Proposed fix
LOGGER.info(SignatureConstant.SESSIONID, SignatureConstant.COSE_SIGN, SignatureConstant.BLANK, - "Getting Signature Certificate. for appId:" + applicationId + "and refId:" + referenceId); + "Getting Signature Certificate. for appId: " + applicationId + " and refId: " + referenceId); SignatureCertificate certificateResponse = keymanagerService.getSignatureCertificate(applicationId, Optional.of(referenceId), timestamp); LOGGER.info(SignatureConstant.SESSIONID, SignatureConstant.COSE_SIGN, SignatureConstant.BLANK, - "Signature Certificate Obtained. for appId:" + applicationId + "and refId:" + referenceId); + "Signature Certificate Obtained. for appId: " + applicationId + " and refId: " + 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/signature/service/impl/CoseSignatureServiceImpl.java` around lines 478 - 482, The log messages in CoseSignatureServiceImpl using LOGGER.info concatenate applicationId and referenceId without spaces; update the two LOGGER.info calls around getSignatureCertificate (referencing LOGGER, SignatureConstant.SESSIONID, SignatureConstant.COSE_SIGN, applicationId, referenceId, and keymanagerService.getSignatureCertificate) to include proper spacing/punctuation (e.g., "for appId: " and " and refId: ") so the output reads correctly; ensure both the pre-call and post-call messages are fixed consistently.kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/controller/KeymanagerControllerTest.java-548-552 (1)
548-552:⚠️ Potential issue | 🟡 MinorReinstate a response-body assertion for this endpoint test.
Line [551] only checks HTTP status now; this can miss functional regressions when response wraps errors with 200 status.
🧪 Suggested assertion strengthening
mockMvc.perform(post("/generateSymmetricKey") .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(request))) - .andExpect(status().isOk()); + .andExpect(status().isOk()) + .andExpect(jsonPath("$.response").exists());🤖 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/controller/KeymanagerControllerTest.java` around lines 548 - 552, The test KeymanagerControllerTest currently only asserts HTTP 200 for the mockMvc.perform(post("/generateSymmetricKey")... ) call which can miss error payloads wrapped in 200; update the assertion to validate the response body (e.g., assert non-empty JSON and expected fields) by adding content assertions on the mockMvc result (use objectMapper.writeValueAsString(request) to locate the request and then assert response content type and jsonPath or string content for the expected key properties) so the test verifies both status and the actual returned payload.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerErrorCode.java-72-72 (1)
72-72:⚠️ Potential issue | 🟡 Minor
%Sin the error message format string uppercases the substituted value — likely unintentional.
String.formattreats%Sas uppercase-%s, which will render the public-key algorithm name in ALL CAPS in the error message. Use%sunless the uppercase rendering is intentional.🐛 Proposed fix
- JWE_ENCRYPTION_NOT_SUPPORTED("KER-CRY-017", "JWE encryption is not supported for the provided (%S) public key."), + JWE_ENCRYPTION_NOT_SUPPORTED("KER-CRY-017", "JWE encryption is not supported for the provided (%s) public 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/java/io/mosip/kernel/cryptomanager/constant/CryptomanagerErrorCode.java` at line 72, The error message for the enum constant JWE_ENCRYPTION_NOT_SUPPORTED in CryptomanagerErrorCode uses "%S" which forces uppercase via String.format; change the format specifier to "%s" in the JWE_ENCRYPTION_NOT_SUPPORTED definition so the substituted public key algorithm name is rendered with its original casing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
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/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/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/kernel-keymanager-service/src/main/resources/application-local.propertieskernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/cryptomanager/test/integration/CryptographicServiceIntegrationTest.javakernel/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/controller/KeymanagerControllerTest.javakernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymanagerservice/test/service/KeymanagerServiceImplTest.javakernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/keymigrate/test/KeyMigratorServiceTest.javakernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.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/impl/ComponentKeysAlgorithmMigrator.javakernel/keys-algorithm-migrator/src/main/resources/application-local.propertieskernel/keys-algorithm-migrator/src/main/resources/bootstrap.propertieskernel/keys-migrator/src/main/java/io/mosip/kernel/migrate/impl/BaseKeysMigrator.javakernel/pom.xml
...rvice/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java
Outdated
Show resolved
Hide resolved
...rvice/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.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/keymanager/hsm/impl/pkcs/PKCS12KeyStoreImpl.java
Show resolved
Hide resolved
...keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java
Show resolved
Hide resolved
...vice/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java
Show resolved
Hide resolved
kernel/keys-algorithm-migrator/src/main/resources/application-local.properties
Show resolved
Hide resolved
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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/keymanagerservice/util/KeymanagerUtil.java (1)
494-495:⚠️ Potential issue | 🔴 CriticalFix null-check guard to avoid NPE in SAN parsing.
Both guards use
&&and dereferencealtValuesMapwhen it is null. This will fail at runtime.🔧 Proposed fix
- if (altValuesMap == null && altValuesMap.isEmpty()) { + if (altValuesMap == null || altValuesMap.isEmpty()) { return certParams; }Also applies to: 564-565
🤖 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/util/KeymanagerUtil.java` around lines 494 - 495, The null-check guard in KeymanagerUtil is wrong: it currently uses "altValuesMap == null && altValuesMap.isEmpty()" which dereferences altValuesMap when null and can throw NPE; change both occurrences to use a short-circuiting check (altValuesMap == null || altValuesMap.isEmpty()) so the method (the SAN parsing logic in KeymanagerUtil referencing altValuesMap) returns early safely. Ensure both places mentioned (the two guards around SAN parsing) are updated to the || form.
♻️ Duplicate comments (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java (1)
261-263:⚠️ Potential issue | 🔴 CriticalDerive ECC algorithm from the certificate public key, not global config.
Using
ecCurveNamehere can produce header/algorithm mismatch when the certificate key differs from configured default.🔧 Proposed fix
- String algName = ecCurveName.equals(KeymanagerConstant.ED25519_KEY_TYPE) ? KeymanagerConstant.X25519_KEY_TYPE : ecCurveName; + String algName = keymanagerUtil.getEcCurveName(publicKey); + if (KeymanagerConstant.ED25519_KEY_TYPE.equalsIgnoreCase(algName)) { + algName = KeymanagerConstant.X25519_KEY_TYPE; + } byte[] encryptedData = ecCryptomanagerService.asymmetricEcEncrypt(publicKey, cryptomanagerUtil.decodeBase64Data(cryptoRequestDto.getData()), null, aad, algName);🤖 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 261 - 263, The code is using the global ecCurveName to set algName which can mismatch the certificate's actual public key; replace usage of ecCurveName when computing algName for the asymmetric EC encrypt call. Inspect the provided publicKey (the certificate public key) to derive the curve/algorithm name (e.g., inspect publicKey.getAlgorithm(), key parameters or curve OID) and map ED25519 to X25519 as before (KeymanagerConstant.ED25519_KEY_TYPE -> KeymanagerConstant.X25519_KEY_TYPE) to produce algName, then call ecCryptomanagerService.asymmetricEcEncrypt(publicKey, cryptomanagerUtil.decodeBase64Data(cryptoRequestDto.getData()), null, aad, algName) and keep the concatByteArrays(aad, encryptedData) step unchanged.
🤖 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/cryptomanager/service/impl/CryptomanagerServiceImpl.java`:
- Around line 270-272: The encrypted payload currently places concatedData (from
cryptomanagerUtil.concatCertThumbprint) before the splitter when calling
CryptoUtil.combineByteArray in CryptomanagerServiceImpl, but the ECC decrypt
parser expects thumbprint/data to be located after the splitter; change the
ordering so the splitter and headerBytes are combined first and concatedData is
appended after the splitter (i.e., invert the arguments to
CryptoUtil.combineByteArray or reorder inputs) so finalEncKeyBytes matches the
decrypt layout, then continue to encode with CryptoUtil.encodeToURLSafeBase64
and set via cryptoResponseDto.setData.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java`:
- Around line 760-771: The code repeatedly loads SIGN trust certificates inside
the per-trust-cert loop causing redundant repository/service calls; move the
sign-cert loading logic out of that loop: call
keyAliasRepository.findByApplicationIdAndReferenceId(signApplicationid,
certificateSignRefID) once, map each signCert to its CertificateDataResponseDto
via keymanagerService.getAllCertificates(signCert.getApplicationId(),
Optional.ofNullable(signCert.getReferenceId())), convert each cert using
convertToCertificate(...) and populate intermediateTrust (or a new collection)
before entering the trustCert loop, then reuse that preloaded collection inside
the loop instead of calling findByApplicationIdAndReferenceId/getAllCertificates
repeatedly.
---
Outside diff comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java`:
- Around line 494-495: The null-check guard in KeymanagerUtil is wrong: it
currently uses "altValuesMap == null && altValuesMap.isEmpty()" which
dereferences altValuesMap when null and can throw NPE; change both occurrences
to use a short-circuiting check (altValuesMap == null || altValuesMap.isEmpty())
so the method (the SAN parsing logic in KeymanagerUtil referencing altValuesMap)
returns early safely. Ensure both places mentioned (the two guards around SAN
parsing) are updated to the || form.
---
Duplicate comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java`:
- Around line 261-263: The code is using the global ecCurveName to set algName
which can mismatch the certificate's actual public key; replace usage of
ecCurveName when computing algName for the asymmetric EC encrypt call. Inspect
the provided publicKey (the certificate public key) to derive the
curve/algorithm name (e.g., inspect publicKey.getAlgorithm(), key parameters or
curve OID) and map ED25519 to X25519 as before
(KeymanagerConstant.ED25519_KEY_TYPE -> KeymanagerConstant.X25519_KEY_TYPE) to
produce algName, then call ecCryptomanagerService.asymmetricEcEncrypt(publicKey,
cryptomanagerUtil.decodeBase64Data(cryptoRequestDto.getData()), null, aad,
algName) and keep the concatByteArrays(aad, encryptedData) step unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.javakernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java
...rvice/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java
Outdated
Show resolved
Hide resolved
...-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/util/KeymanagerUtil.java
Show resolved
Hide resolved
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java (1)
271-273:⚠️ Potential issue | 🔴 CriticalECC payload layout is still inverted against the decrypt parser.
Line 272 places
concatedDatabefore the splitter, but decrypt expects the ECC header/algorithm bytes before the splitter and thumbprint+ciphertext after it. This causes incorrect algorithm detection/decryption path selection.🔧 Proposed fix
- byte[] finalEncKeyBytes = CryptoUtil.combineByteArray(concatedData, headerBytes, keySplitter); + byte[] finalEncKeyBytes = CryptoUtil.combineByteArray(headerBytes, concatedData, keySplitter);🤖 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 271 - 273, The payload bytes are being assembled in the wrong order: replace the current combination that puts concatedData before headerBytes (the call to CryptoUtil.combineByteArray(concatedData, headerBytes, keySplitter)) with an order matching the decrypt parser (headerBytes first, then keySplitter, then certThumbprint+ciphertext). Update the call in CryptomanagerServiceImpl where concatedData is created by concatCertThumbprint to instead call CryptoUtil.combineByteArray(headerBytes, keySplitter, concatedData) so finalEncKeyBytes (and the subsequent CryptoUtil.encodeToURLSafeBase64/cryptoResponseDto.setData) produce the expected ECC header+splitter+thumbprint+ciphertext layout.kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java (1)
450-460:⚠️ Potential issue | 🔴 Critical
NullPointerException/NoSuchElementExceptionwhen no thumbprint matches — DB operations still precede theencRandomKeynull-guard.The fix proposed in the previous review has not been applied. Lines 450–453 execute unconditionally after the loop:
- Line 451:
Objects.requireNonNull(kyAlias)throwsNullPointerException(with no diagnostic message) when no alias matched.- Line 452:
dbKeyStore.get()throwsNoSuchElementExceptionif the alias isn't in the store.The null-check at line 455 (intended to guard both) never gets a chance to fire. Move the DB lookups and certificate parsing to after the
encRandomKeyguard, as outlined in the prior review.🤖 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/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java` around lines 450 - 460, The DB lookups and certificate parsing (calls to keyStoreRepository.findByAlias(kyAlias), keyAliasRepository.findById(Objects.requireNonNull(kyAlias)), using dbKeyStore.get(), reading certificateData and calling keymanagerUtil.convertToCertificate) must be moved so they run only after the null/absence guard on encRandomKey; currently they execute unconditionally and can throw NullPointerException/NoSuchElementException before the encRandomKey check. Change the flow in ZKCryptoManagerServiceImpl: first check if encRandomKey is null and throw the ZKCryptoException as currently written, then perform keyStoreRepository.findByAlias(kyAlias) and keyAliasRepository.findById(kyAlias) and parse certificateData; when unwrapping Optionals avoid .get() and Objects.requireNonNull—use Optional.orElseThrow with a clear exception or handle the empty case to produce meaningful errors.
🧹 Nitpick comments (1)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java (1)
465-471: MoveSymmetricKeyRequestDtoconstruction into the RSA branch only.
symmetricKeyRequestDtois constructed unconditionally but is only consumed in the RSA path (line 469). The EC branch (lines 470–471) doesn't use it. Move it inside the RSA branch to avoid unnecessary allocation and to clarify control flow.♻️ Proposed refactor
- SymmetricKeyRequestDto symmetricKeyRequestDto = new SymmetricKeyRequestDto(pubKeyApplicationId, - localDateTimeStamp, pubKeyReferenceId, encRandomKey, true); - String randomKey = x509Cert.getPublicKey().getAlgorithm().equalsIgnoreCase(KeymanagerConstant.RSA) - ? keyManagerService.decryptSymmetricKey(symmetricKeyRequestDto).getSymmetricKey() + ? keyManagerService.decryptSymmetricKey(new SymmetricKeyRequestDto(pubKeyApplicationId, + localDateTimeStamp, pubKeyReferenceId, encRandomKey, true)).getSymmetricKey() : CryptoUtil.encodeToURLSafeBase64(ecCryptomanagerService.asymmetricEcDecrypt(privateKey, encRandomKeyBytes, null, keymanagerUtil.getEcCurveName(x509Cert.getPublicKey())));🤖 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/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java` around lines 465 - 471, Move construction of SymmetricKeyRequestDto into the RSA branch so it’s only created when needed: check x509Cert.getPublicKey().getAlgorithm().equalsIgnoreCase(KeymanagerConstant.RSA) and inside that branch instantiate SymmetricKeyRequestDto then call keyManagerService.decryptSymmetricKey(...).getSymmetricKey(); keep the EC branch using ecCryptomanagerService.asymmetricEcDecrypt(privateKey, encRandomKeyBytes, null, keymanagerUtil.getEcCurveName(...)) unchanged. This avoids unnecessary allocation and makes the control flow around SymmetricKeyRequestDto, keyManagerService.decryptSymmetricKey, and ecCryptomanagerService.asymmetricEcDecrypt explicit.
🤖 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/cryptomanager/util/CryptomanagerUtils.java`:
- Around line 460-467: Trim and case-normalize referenceId before routing:
replace direct uses of refId.get() with a normalized value (e.g., String
referenceId = refId.get().trim().toUpperCase(Locale.ROOT)) and pass that
normalized referenceId into isSignatureKeyRefId(appId, referenceId) and
getKeyFromHSM(appId, referenceId); apply the same trimming/case-folding before
any DB routing/lookup calls (the other occurrence that currently calls
isSignatureKeyRefId/getKeyFromHSM or performs DB lookup should use the same
normalized referenceId).
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java`:
- Around line 154-156: In ZKCryptoManagerServiceImpl (the warmup loop), replace
the broad "catch (Throwable e)" with a narrower "catch (Exception e)" so JVM
Errors (e.g., OutOfMemoryError, StackOverflowError) are not swallowed;
optionally add a separate "catch (Error e)" that rethrows the Error (or logs and
rethrows) to ensure fatal JVM errors propagate rather than being ignored.
- Around line 462-464: The code calls keyAliasRepository.findById(kyAlias) and
then uses keyAliasObj.get() without checking presence, which can throw
NoSuchElementException; update ZKCryptoManagerServiceImpl to check
keyAliasObj.isPresent() (or use orElseThrow with a clear exception) before
accessing getApplicationId/getReferenceId, and if absent throw or return a
controlled error (e.g., IllegalStateException or a custom exception) with a
descriptive message; ensure the subsequent call to
cryptomanagerUtil.getPrivateKeyForDecryption still uses the safely-obtained
keyAlias values and that the error path is logged or propagated consistently.
---
Duplicate comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/service/impl/CryptomanagerServiceImpl.java`:
- Around line 271-273: The payload bytes are being assembled in the wrong order:
replace the current combination that puts concatedData before headerBytes (the
call to CryptoUtil.combineByteArray(concatedData, headerBytes, keySplitter))
with an order matching the decrypt parser (headerBytes first, then keySplitter,
then certThumbprint+ciphertext). Update the call in CryptomanagerServiceImpl
where concatedData is created by concatCertThumbprint to instead call
CryptoUtil.combineByteArray(headerBytes, keySplitter, concatedData) so
finalEncKeyBytes (and the subsequent
CryptoUtil.encodeToURLSafeBase64/cryptoResponseDto.setData) produce the expected
ECC header+splitter+thumbprint+ciphertext layout.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java`:
- Around line 450-460: The DB lookups and certificate parsing (calls to
keyStoreRepository.findByAlias(kyAlias),
keyAliasRepository.findById(Objects.requireNonNull(kyAlias)), using
dbKeyStore.get(), reading certificateData and calling
keymanagerUtil.convertToCertificate) must be moved so they run only after the
null/absence guard on encRandomKey; currently they execute unconditionally and
can throw NullPointerException/NoSuchElementException before the encRandomKey
check. Change the flow in ZKCryptoManagerServiceImpl: first check if
encRandomKey is null and throw the ZKCryptoException as currently written, then
perform keyStoreRepository.findByAlias(kyAlias) and
keyAliasRepository.findById(kyAlias) and parse certificateData; when unwrapping
Optionals avoid .get() and Objects.requireNonNull—use Optional.orElseThrow with
a clear exception or handle the empty case to produce meaningful errors.
---
Nitpick comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java`:
- Around line 465-471: Move construction of SymmetricKeyRequestDto into the RSA
branch so it’s only created when needed: check
x509Cert.getPublicKey().getAlgorithm().equalsIgnoreCase(KeymanagerConstant.RSA)
and inside that branch instantiate SymmetricKeyRequestDto then call
keyManagerService.decryptSymmetricKey(...).getSymmetricKey(); keep the EC branch
using ecCryptomanagerService.asymmetricEcDecrypt(privateKey, encRandomKeyBytes,
null, keymanagerUtil.getEcCurveName(...)) unchanged. This avoids unnecessary
allocation and makes the control flow around SymmetricKeyRequestDto,
keyManagerService.decryptSymmetricKey, and
ecCryptomanagerService.asymmetricEcDecrypt explicit.
ℹ️ 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/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/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java
...-keymanager-service/src/main/java/io/mosip/kernel/cryptomanager/util/CryptomanagerUtils.java
Show resolved
Hide resolved
...e/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java
Show resolved
Hide resolved
...e/src/main/java/io/mosip/kernel/zkcryptoservice/service/impl/ZKCryptoManagerServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java (2)
1524-1528: Consider foldingCURRENTKEYALIASinitialization intocreateKeyAliasMapWithKeyAliases.Several tests repeat
pubKeyAliasMap.put(KeymanagerConstant.CURRENTKEYALIAS, Collections.emptyList()). Centralizing that in the helper will reduce duplication and keep fixture shape consistent.🤖 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/zkcryptoservice/test/ZKCryptoManagerServiceTest.java` around lines 1524 - 1528, The helper createKeyAliasMapWithKeyAliases currently only sets KeymanagerConstant.KEYALIAS; update it to also populate KeymanagerConstant.CURRENTKEYALIAS with Collections.emptyList() so callers no longer need to repeatedly call pubKeyAliasMap.put(KeymanagerConstant.CURRENTKEYALIAS, Collections.emptyList()). Modify createKeyAliasMapWithKeyAliases to add keyAliasMap.put(KeymanagerConstant.CURRENTKEYALIAS, Collections.emptyList()) and then remove the duplicated pubKeyAliasMap.put(...) lines from tests that build pubKeyAliasMap.
106-110: ECC branch intent is present, but not explicitly asserted.You introduced
ecCryptomanagerServiceand force an EC curve in setup, but these tests don’t explicitly verify ECC-service interaction. Add at least one assertion-driven test that proves the ECC re-encryption path invokesecCryptomanagerServicefor EC keys.Also applies to: 1548-1563
🤖 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/zkcryptoservice/test/ZKCryptoManagerServiceTest.java` around lines 106 - 110, Add a new assertion-driven test that, with the EC branch forced in setup, invokes the ZKCryptoManagerService re-encryption flow and verifies the EC-specific service is used: call the same method under test that triggers re-encryption on ZKCryptoManagerService (the method exercised by existing tests), then use Mockito.verify(ecCryptomanagerService, times(1)) to assert the expected EC method (e.g., reEncrypt/reEncryptKey or the EC-specific operation used in your code) was called for EC keys; keep KeyAliasRepository mocked as before and assert no unintended calls to non-EC services if applicable.
🤖 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/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java`:
- Around line 366-377: Remove the catch block that treats NullPointerException
as an acceptable outcome in ZKCryptoManagerServiceTest and its other similar
negative-path test blocks; keep only the expect-ZKCryptoException flow for
zkReEncryptRandomKey so NPEs surface as test failures. Specifically, in the test
method(s) calling zkReEncryptRandomKey(encryptedKey) remove the catch
(NullPointerException e) branch (and its assertNotNull) so the test only catches
ZKCryptoException, and ensure any setup uses encRandomKey/kyAlias so the failure
path triggers the intended ZKCryptoException rather than masking bugs; apply the
same change to the analogous test block(s) mentioned in the comment.
---
Nitpick comments:
In
`@kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java`:
- Around line 1524-1528: The helper createKeyAliasMapWithKeyAliases currently
only sets KeymanagerConstant.KEYALIAS; update it to also populate
KeymanagerConstant.CURRENTKEYALIAS with Collections.emptyList() so callers no
longer need to repeatedly call
pubKeyAliasMap.put(KeymanagerConstant.CURRENTKEYALIAS, Collections.emptyList()).
Modify createKeyAliasMapWithKeyAliases to add
keyAliasMap.put(KeymanagerConstant.CURRENTKEYALIAS, Collections.emptyList()) and
then remove the duplicated pubKeyAliasMap.put(...) lines from tests that build
pubKeyAliasMap.
- Around line 106-110: Add a new assertion-driven test that, with the EC branch
forced in setup, invokes the ZKCryptoManagerService re-encryption flow and
verifies the EC-specific service is used: call the same method under test that
triggers re-encryption on ZKCryptoManagerService (the method exercised by
existing tests), then use Mockito.verify(ecCryptomanagerService, times(1)) to
assert the expected EC method (e.g., reEncrypt/reEncryptKey or the EC-specific
operation used in your code) was called for EC keys; keep KeyAliasRepository
mocked as before and assert no unintended calls to non-EC services if
applicable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kernel/kernel-keymanager-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java
...r-service/src/test/java/io/mosip/kernel/zkcryptoservice/test/ZKCryptoManagerServiceTest.java
Show resolved
Hide resolved
nagendra0721
changed the title
Signed-off-by: nagendra0721 <nagendra0718@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
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/keymanagerservice/service/impl/KeymanagerServiceImpl.java (1)
932-934:⚠️ Potential issue | 🟠 MajorFix incorrect logical operator to prevent NoSuchElementException.
Line 932 uses
||(OR) which causes aNoSuchElementExceptionwhenrefId.isPresent()isfalse. The second condition!refId.get().trim().isEmpty()will execute and callget()on an empty Optional. Change the operator to&&(AND):Suggested fix
- if ((refId.isPresent() || !refId.get().trim().isEmpty()) && (!appId.equalsIgnoreCase(signApplicationid) && + if ((refId.isPresent() && !refId.get().trim().isEmpty()) && (!appId.equalsIgnoreCase(signApplicationid) && !refId.get().equals(certificateSignRefID))) { keymanagerUtil.destoryKey(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/keymanagerservice/service/impl/KeymanagerServiceImpl.java` around lines 932 - 934, The if condition in KeymanagerServiceImpl uses || between refId.isPresent() and !refId.get().trim().isEmpty(), which can call refId.get() when the Optional is empty; replace the OR with AND so you only call refId.get() when present (i.e., use refId.isPresent() && !refId.get().trim().isEmpty()), or alternatively use a safe Optional check like refId.map(String::trim).filter(s -> !s.isEmpty()).isPresent() before referencing refId.get() in the subsequent checks involving appId, signApplicationid and certificateSignRefID (the block that calls keymanagerUtil.destoryKey(signPrivateKey)).
🧹 Nitpick comments (3)
kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java (3)
376-383: DuplicatekeyGenerator.getEd25519KeyPair()call can be extracted.Lines 378 and 381 both call
keyGenerator.getEd25519KeyPair()identically. The keypair generation can be moved before the conditional to reduce duplication.♻️ Suggested refactor
if (referenceId.equals(KeyReferenceIdConsts.ED25519_SIGN.name()) && isSignKey) { + keypair = keyGenerator.getEd25519KeyPair(); if (eccCurve.equals(KeymanagerConstant.ED25519_KEY_TYPE)) { - keypair = keyGenerator.getEd25519KeyPair(); certInfo = getCertificateFromHSM(applicationId, timeStamp, KeymanagerConstant.X25519_ENC_KEY_REF_ID); } else { - keypair = keyGenerator.getEd25519KeyPair(); certInfo = getCertificateFromHSM(applicationId, timeStamp, KeymanagerConstant.EMPTY); }🤖 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 376 - 383, In KeymanagerServiceImpl, remove the duplicate keyGenerator.getEd25519KeyPair() calls by invoking keyGenerator.getEd25519KeyPair() once before the eccCurve check in the block that handles referenceId.equals(KeyReferenceIdConsts.ED25519_SIGN.name()) && isSignKey; assign the resulting keypair to the existing keypair variable, then keep the conditional only to set certInfo to KeymanagerConstant.X25519_ENC_KEY_REF_ID when eccCurve.equals(KeymanagerConstant.ED25519_KEY_TYPE) or to KeymanagerConstant.EMPTY otherwise (preserving the getCertificateFromHSM(applicationId, timeStamp, ...) calls); this refactor touches the KeymanagerServiceImpl method containing the shown block and uses keyGenerator, keypair, eccCurve, certInfo, getCertificateFromHSM, and KeyReferenceIdConsts/KeymanagerConstant identifiers.
1478-1485: Exception handling loses original error context.Catching
KeymanagerServiceExceptionand re-throwing a new one discards the original error message and stack trace. This makes debugging harder and may mask unrelated validation failures.♻️ Suggested improvement
try { ecKeyPairGenRequestValidator.validate(objectType, request); } catch (KeymanagerServiceException e) { LOGGER.error(KeymanagerConstant.SESSIONID, this.getClass().getSimpleName(), KeymanagerConstant.VALIDATE, - "Reference Id not supported for the provided application Id for RSA Sign Keys."); + "Reference Id not supported for the provided application Id for RSA Sign Keys. Original: " + e.getMessage()); throw new KeymanagerServiceException(KeymanagerErrorConstant.RSA_SIGN_REFERENCE_ID_NOT_SUPPORTED.getErrorCode(), - KeymanagerErrorConstant.RSA_SIGN_REFERENCE_ID_NOT_SUPPORTED.getErrorMessage()); + KeymanagerErrorConstant.RSA_SIGN_REFERENCE_ID_NOT_SUPPORTED.getErrorMessage(), e); }🤖 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 1478 - 1485, The catch block in KeymanagerServiceImpl around ecKeyPairGenRequestValidator.validate(objectType, request) currently throws a new KeymanagerServiceException and loses the original exception context; update the catch to include the caught exception as the cause when rethrowing (or pass its message), and include the original exception in the LOGGER.error call so the stack trace is preserved (i.e., log e and throw new KeymanagerServiceException(..., e)); keep the same error codes/messages from KeymanagerErrorConstant.RSA_SIGN_REFERENCE_ID_NOT_SUPPORTED but attach the original KeymanagerServiceException as the cause.
1536-1687: Significant code duplication withgetCertificateFromDBStore.This method duplicates most of
getCertificateFromDBStorelogic (~150 lines). Consider extracting common logic into a shared helper method with algorithm-specific callbacks or parameters to reduce maintenance burden.🤖 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 1536 - 1687, getRSACertificateFromDBStore duplicates most of getCertificateFromDBStore; extract the shared flow (fetching keyAlias map via dbHelper.getKeyAliases, checking currentKeyAlias, reading KeyStore entry, creating alias, generating KeyPair via keyGenerator.getAsymmetricKey, fetching HSM cert via getCertificateFromHSM, computing expiry via dbHelper.getExpiryPolicy, encrypting private key with keymanagerUtil.encryptKey, selecting sign key with keyStore.getAsymmetricKey, building CertificateParameters using sanHelper/getCertificateParameters*, generating cert via CertificateUtility.generateX509Certificate, storing via dbHelper.storeKeyInDBStore and dbHelper.storeKeyInAlias, and destroying private key) into a single helper (e.g., createAndStoreKeypair or buildCertificateForAlgorithm) that accepts algorithm-specific inputs/callbacks (how to pick certInfo for X25519/Ed25519, sign algorithm selection, and any SAN/common-name differences). Replace algorithm-branching code in getRSACertificateFromDBStore and getCertificateFromDBStore with calls to that helper, passing appropriate lambdas or parameters for certificate selection, sign key alias selection, and signAlgorithm.
🤖 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/keymanager/hsm/constant/KeymanagerErrorCode.java`:
- Around line 29-31: The enum KeymanagerErrorCode has a stray trailing comma
before the semicolon and an inconsistent constant name: change the enum constant
CERTIFICATE_SIGN_NOT_SUPPORT to CERTIFICATE_SIGN_NOT_SUPPORTED (matching
ALGORITHM_NOT_SUPPORTED pattern) and remove the extraneous comma so the
declaration ends with just a semicolon; update all usages/references of
CERTIFICATE_SIGN_NOT_SUPPORT throughout the codebase to the new name.
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java`:
- Around line 1508-1516: The HSM certificate retrieval branch in
getCertificateV2 is missing handling for RSA_2048_SIGN, causing inconsistent
behavior with getCertificate; update the conditional that checks refId in
getCertificateV2 (the block that references appId, signApplicationid, refId,
certificateSignRefID, KeyReferenceIdConsts.EC_SECP256K1_SIGN,
KeyReferenceIdConsts.EC_SECP256R1_SIGN, KeyReferenceIdConsts.ED25519_SIGN and
ed25519SupportFlag) to also accept KeyReferenceIdConsts.RSA_2048_SIGN so that
the code logs the same message and calls getCertificateFromHSM(appId,
localDateTimeStamp, refId.get()) for RSA_2048_SIGN ref IDs.
- Around line 1577-1613: This method is missing a check to block key generation
for the ROOT application ID; add a validation like the existing KERNEL_APP_ID
and PARTNER_APP_ID checks (near currentKeyAlias/keyAliasMap handling in
KeymanagerServiceImpl and analogous to getCertificateFromDBStore), so if
applicationId.equalsIgnoreCase(KeymanagerConstant.ROOT_APP_ID) you log an error
via LOGGER.error (use the same session/application params pattern) and throw a
KeymanagerServiceException with
KeymanagerErrorConstant.GENERATION_NOT_ALLOWED.getErrorCode() and the formatted
GENERATION_NOT_ALLOWED message (use "Root App Id" or similar), ensuring the
ROOT_APP_ID constant and exception flow mirror the other two checks.
- Around line 1583-1598: In KeymanagerServiceImpl (inside the keyAlias.forEach
lambda) you are calling keyFromDBStore.get() without checking the Optional
returned by dbHelper.getKeyStoreFromDB(ksAlias); add an existence check and
handle the empty case instead of letting NoSuchElementException escape — for
example, if keyFromDBStore.isPresent() use get() as now, otherwise throw a
KeymanagerServiceException (with an appropriate KeymanagerErrorConstant/error
message) or handle per business logic; update the lambda to reference
keyFromDBStore.orElseThrow(...) or an explicit isPresent() branch so
masterKeyAlias and privateKeyObj access is safe.
---
Outside diff comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java`:
- Around line 932-934: The if condition in KeymanagerServiceImpl uses || between
refId.isPresent() and !refId.get().trim().isEmpty(), which can call refId.get()
when the Optional is empty; replace the OR with AND so you only call refId.get()
when present (i.e., use refId.isPresent() && !refId.get().trim().isEmpty()), or
alternatively use a safe Optional check like refId.map(String::trim).filter(s ->
!s.isEmpty()).isPresent() before referencing refId.get() in the subsequent
checks involving appId, signApplicationid and certificateSignRefID (the block
that calls keymanagerUtil.destoryKey(signPrivateKey)).
---
Nitpick comments:
In
`@kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java`:
- Around line 376-383: In KeymanagerServiceImpl, remove the duplicate
keyGenerator.getEd25519KeyPair() calls by invoking
keyGenerator.getEd25519KeyPair() once before the eccCurve check in the block
that handles referenceId.equals(KeyReferenceIdConsts.ED25519_SIGN.name()) &&
isSignKey; assign the resulting keypair to the existing keypair variable, then
keep the conditional only to set certInfo to
KeymanagerConstant.X25519_ENC_KEY_REF_ID when
eccCurve.equals(KeymanagerConstant.ED25519_KEY_TYPE) or to
KeymanagerConstant.EMPTY otherwise (preserving the
getCertificateFromHSM(applicationId, timeStamp, ...) calls); this refactor
touches the KeymanagerServiceImpl method containing the shown block and uses
keyGenerator, keypair, eccCurve, certInfo, getCertificateFromHSM, and
KeyReferenceIdConsts/KeymanagerConstant identifiers.
- Around line 1478-1485: The catch block in KeymanagerServiceImpl around
ecKeyPairGenRequestValidator.validate(objectType, request) currently throws a
new KeymanagerServiceException and loses the original exception context; update
the catch to include the caught exception as the cause when rethrowing (or pass
its message), and include the original exception in the LOGGER.error call so the
stack trace is preserved (i.e., log e and throw new
KeymanagerServiceException(..., e)); keep the same error codes/messages from
KeymanagerErrorConstant.RSA_SIGN_REFERENCE_ID_NOT_SUPPORTED but attach the
original KeymanagerServiceException as the cause.
- Around line 1536-1687: getRSACertificateFromDBStore duplicates most of
getCertificateFromDBStore; extract the shared flow (fetching keyAlias map via
dbHelper.getKeyAliases, checking currentKeyAlias, reading KeyStore entry,
creating alias, generating KeyPair via keyGenerator.getAsymmetricKey, fetching
HSM cert via getCertificateFromHSM, computing expiry via
dbHelper.getExpiryPolicy, encrypting private key with keymanagerUtil.encryptKey,
selecting sign key with keyStore.getAsymmetricKey, building
CertificateParameters using sanHelper/getCertificateParameters*, generating cert
via CertificateUtility.generateX509Certificate, storing via
dbHelper.storeKeyInDBStore and dbHelper.storeKeyInAlias, and destroying private
key) into a single helper (e.g., createAndStoreKeypair or
buildCertificateForAlgorithm) that accepts algorithm-specific inputs/callbacks
(how to pick certInfo for X25519/Ed25519, sign algorithm selection, and any
SAN/common-name differences). Replace algorithm-branching code in
getRSACertificateFromDBStore and getCertificateFromDBStore with calls to that
helper, passing appropriate lambdas or parameters for certificate selection,
sign key alias selection, and signAlgorithm.
ℹ️ 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/keymanager/hsm/constant/KeymanagerErrorCode.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/service/impl/KeymanagerServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- kernel/kernel-keymanager-service/src/main/java/io/mosip/kernel/keymanager/hsm/util/CertificateUtility.java
...nager-service/src/main/java/io/mosip/kernel/keymanager/hsm/constant/KeymanagerErrorCode.java
Show resolved
Hide resolved
...vice/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java
Show resolved
Hide resolved
...vice/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.java
Show resolved
Hide resolved
...vice/src/main/java/io/mosip/kernel/keymanagerservice/service/impl/KeymanagerServiceImpl.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>
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.
Summary by CodeRabbit
New Features
API
Errors
Tests