Skip to content

Extracted public key data in getPublicKey API [MOSIP-22993] #1284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
GOKULRAJ136 wants to merge 5 commits into
base: release-1.3.x
Choose a base branch
from
Open

Extracted public key data in getPublicKey API [MOSIP-22993] #1284

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e28bc9
Extracted public key data in getPublicKey API [MOSIP-22993]
GOKULRAJ136 Nov 14, 2025
cbde736
Merge branch 'release-1.3.x' into MOSIP-22993-13X
GOKULRAJ136 Nov 14, 2025
c14c2f9
Merge branch 'release-1.3.x' into MOSIP-22993-13X
GOKULRAJ136 Nov 15, 2025
6c7a7d6
Merge branch 'release-1.3.x' into MOSIP-22993-13X
GOKULRAJ136 Nov 17, 2025
8579d20
Merge branch 'release-1.3.x' into MOSIP-22993-13X
GOKULRAJ136 Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 47 additions & 10 deletions ...ice/src/main/java/io/mosip/kernel/syncdata/service/impl/SyncConfigDetailsServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package io.mosip.kernel.syncdata.service.impl;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.mosip.kernel.clientcrypto.dto.TpmCryptoRequestDto;
import io.mosip.kernel.clientcrypto.dto.TpmCryptoResponseDto;
Expand All @@ -9,6 +9,7 @@
import io.mosip.kernel.core.exception.ServiceError;
import io.mosip.kernel.core.http.ResponseWrapper;
import io.mosip.kernel.core.util.CryptoUtil;
import io.mosip.kernel.core.util.DateUtils2;
import io.mosip.kernel.core.util.HMACUtils2;
import io.mosip.kernel.syncdata.constant.MasterDataErrorCode;
import io.mosip.kernel.syncdata.constant.SyncConfigDetailsErrorCode;
Expand Down Expand Up @@ -39,9 +40,13 @@
import org.springframework.web.client.RestTemplate;
import org.springframework.web.util.UriComponentsBuilder;

import java.io.IOException;
import java.io.ByteArrayInputStream;
import java.io.StringReader;
import java.nio.charset.StandardCharsets;
import java.security.PublicKey;
import java.security.cert.CertificateFactory;
import java.security.cert.X509Certificate;
import java.time.OffsetDateTime;
import java.util.*;
import java.util.Map.Entry;

Expand All @@ -62,6 +67,8 @@ public class SyncConfigDetailsServiceImpl implements SyncConfigDetailsService {

private static final Logger LOGGER = LoggerFactory.getLogger(SyncConfigDetailsServiceImpl.class);
private static final String SLASH = "/";
private static final String BEGIN_KEY = "-----BEGIN PUBLIC KEY-----";
private static final String END_KEY = "-----END PUBLIC KEY-----";

@Autowired
private RestTemplate restTemplate;
Expand Down Expand Up @@ -236,14 +243,31 @@ public PublicKeyResponse<String> getPublicKey(String applicationId, String timeS
}

try {
publicKeyResponseMapped = objectMapper.readValue(publicKeyResponseEntity.getBody(),
new TypeReference<ResponseWrapper<PublicKeyResponse<String>>>() {
});

publicKeyResponseMapped.getResponse().setProfile(environment.getActiveProfiles()[0]);
LOGGER.debug("Public key fetched successfully for applicationId: {}", applicationId);
return publicKeyResponseMapped.getResponse();
} catch (IOException | NullPointerException e) {
JsonNode root = objectMapper.readTree(publicKeyResponseEntity.getBody());
String certificatePem = root.path("response").path("certificate").asText();
String issuedAt = root.path("response").path("issuedAt").asText();
String expiryAt = root.path("response").path("expiryAt").asText();

CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509");
ByteArrayInputStream certStream = new ByteArrayInputStream(certificatePem.getBytes(StandardCharsets.UTF_8));
X509Certificate certificate = (X509Certificate) certificateFactory.generateCertificate(certStream);
PublicKey publicKey = certificate.getPublicKey();

String publicKeyPEM = convertToPEM(publicKey);

OffsetDateTime issued = OffsetDateTime.parse(issuedAt);
OffsetDateTime expiry = OffsetDateTime.parse(expiryAt);

PublicKeyResponse<String> response = new PublicKeyResponse<>();
response.setPublicKey(publicKeyPEM);
response.setLastSyncTime(DateUtils2.getUTCCurrentDateTimeString());
response.setIssuedAt(issued.toLocalDateTime());
response.setExpiryAt(expiry.toLocalDateTime());
response.setProfile(environment.getActiveProfiles()[0]);

return response;

} catch (Exception e) {
Comment on lines 245 to +270
Copy link

@coderabbitai coderabbitai bot Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Handle missing response fields and active profiles more defensively

The new parsing flow is directionally good, but a few robustness points are worth tightening:

  • environment.getActiveProfiles()[0] (Line 266) will throw ArrayIndexOutOfBoundsException if no active profiles are set (which is valid in Spring). Consider a safe fallback, e.g. checking length and defaulting to "default" or a configured value.
  • certificate, issuedAt, and expiryAt are taken via asText() (Lines 247–249), so missing/blank fields only surface later as generic parse errors (certificate factory or OffsetDateTime.parse). It would be clearer to explicitly validate these fields (null/empty/format) and throw a more specific exception (e.g. SyncInvalidArgumentException with details) before attempting parsing.
  • In the catch block (Lines 270–274) you log only e.getMessage(). Using LOGGER.error("Failed to parse public key response", e); would retain the stack trace and make production debugging easier.
  • Behavioural change: lastSyncTime and profile are now derived locally (DateUtils2.getUTCCurrentDateTimeString() and environment profile) instead of being taken from the upstream response. Please double‑check that this changed semantics is acceptable for existing consumers of getPublicKey.

Please verify these changes against existing registration‑client flows (especially ones relying on profile and lastSyncTime) and adjust the error handling as needed.

I need to verify the specific concerns in this review comment by examining the actual code. Let me gather detailed information from the repository.


---

🏁 Script executed:

```shell
#!/bin/bash
# Retrieve the full method implementation around lines 245-270
fd -e java "SyncConfigDetailsServiceImpl" --exec cat {} \; | head -400 | tail -160

Length of output: 7165

🏁 Script executed:

#!/bin/bash
# Search for getActiveProfiles usage in the codebase to understand safety patterns
rg -nC5 "getActiveProfiles" --type=java

Length of output: 5302

🏁 Script executed:

#!/bin/bash
# Look for the convertToPEM method and BEGIN_KEY/END_KEY constants
fd -e java "SyncConfigDetailsServiceImpl" --exec cat {} \; | grep -A 20 "convertToPEM\|BEGIN_KEY"

Length of output: 2077

🏁 Script executed:

#!/bin/bash
# Search for calls to getPublicKey to understand downstream consumers
rg -nP '\bgetPublicKey\s*\(' --type=java -C3

Length of output: 43012

Let me verify one more critical aspect—whether the behavioral change for profile and lastSyncTime represents a breaking change:


---

🏁 Script executed:

```shell
#!/bin/bash
# Check if profile was previously populated from response or if this is a new addition
rg -nC8 "setProfile|getProfile" --type=java admin/kernel-syncdata-service/src/main/java/io/mosip/kernel/syncdata/

Length of output: 2924

🏁 Script executed:

#!/bin/bash
# Inspect full getPublicKey method to see what was there before (context lines)
fd -e java "SyncConfigDetailsServiceImpl" --exec cat {} \; | grep -B50 "try {" | grep -A50 "getPublicKey"

Length of output: 6990

🏁 Script executed:

#!/bin/bash
# Check the test setup to confirm if profile was being set from response before
cat admin/kernel-syncdata-service/src/test/java/io/mosip/kernel/syncdata/test/service/SyncConfigDetailsServiceImplTest.java | grep -A 30 "public void getPublicKey"

Length of output: 4388

Add defensive null-check for getActiveProfiles() and improve error logging

Your implementation correctly extracts certificate details and derives the public key, but two issues need attention:

  1. Line 266: environment.getActiveProfiles()[0] lacks safety check. If no active profiles are set (valid in Spring), this throws ArrayIndexOutOfBoundsException. Other parts of the codebase use getActiveProfiles().length == 0 ? getDefaultProfiles() : getActiveProfiles() (see admin-service/AuditUtil.java:201). Apply the same pattern here or provide a fallback (e.g., "default").

  2. Line 271: Logging discards stack trace. Change LOGGER.error("Failed to parse public key response: {}", e.getMessage()) to LOGGER.error("Failed to parse public key response", e) to preserve the full stack trace for production debugging.

  3. lastSyncTime and profile derivation: These are now set locally instead of from the response. Note that SyncDataController line 136 overwrites lastSyncTime anyway, so the service-level value is not used; however, verify that clients have no other dependencies on the profile field being immutable or response-sourced.

🤖 Prompt for AI Agents 
In
admin/kernel-syncdata-service/src/main/java/io/mosip/kernel/syncdata/service/impl/SyncConfigDetailsServiceImpl.java
around lines 245 to 270, add a defensive check for
environment.getActiveProfiles() before indexing [0] (use the pattern used in
admin-service/AuditUtil.java:201: use getActiveProfiles().length == 0 ?
getDefaultProfiles() : getActiveProfiles() or fall back to "default") and assign
profile from that safe value; change the error log inside the catch to
LOGGER.error("Failed to parse public key response", e) so the full stacktrace is
logged; also confirm whether lastSyncTime/profile should remain set here or left
for SyncDataController (update or document accordingly).

LOGGER.error("Failed to parse public key response: {}", e.getMessage());
throw new SyncDataServiceException(SyncConfigDetailsErrorCode.SYNC_IO_EXCEPTION.getErrorCode(),
SyncConfigDetailsErrorCode.SYNC_IO_EXCEPTION.getErrorMessage(), e);
Expand Down Expand Up @@ -345,4 +369,17 @@ private String getEncryptedData(String data, Machine machine) {
SyncConfigDetailsErrorCode.SYNC_SERIALIZATION_ERROR.getErrorMessage());
}
}

private String convertToPEM(PublicKey publicKey) {
String encoded = CryptoUtil.encodeBase64String(publicKey.getEncoded());
StringBuilder pemBuilder = new StringBuilder();
pemBuilder.append(BEGIN_KEY).append("\n");
for (int i = 0; i < encoded.length(); i += 64) {
int endIndex = Math.min(i + 64, encoded.length());
pemBuilder.append(encoded, i, endIndex).append("\n");
}
pemBuilder.append(END_KEY);
return pemBuilder.toString();
}

}