Open
Conversation
WalkthroughRefactors tenant keystore generation to populate keystores with two key entries (RSA and EC) via a new TenantKeyPairUtil, adds TenantKeyPairConstants, removes in-class certificate generation helpers, makes signature-algorithm method instance-scoped, and bumps the Carbon kernel version. Changes
Sequence Diagram(s)sequenceDiagram
participant KeyStoreGenerator as KeyStoreGenerator
participant TenantKeyPairUtil as TenantKeyPairUtil
participant KeyStore as KeyStore
participant CryptoProvider as Crypto/BC
KeyStoreGenerator->>TenantKeyPairUtil: addKeyEntry(tenantDomain, ksPassword, KeyStore, aliasRSA, "RSA", sigAlg)
TenantKeyPairUtil->>CryptoProvider: generate RSA KeyPair
CryptoProvider-->>TenantKeyPairUtil: RSA KeyPair
TenantKeyPairUtil->>TenantKeyPairUtil: create self-signed X.509 cert (RSA)
TenantKeyPairUtil->>KeyStore: setPrivateKeyEntry(aliasRSA, privateKey, ksPassword, certChain)
KeyStore-->>TenantKeyPairUtil: ack
KeyStoreGenerator->>TenantKeyPairUtil: addKeyEntry(tenantDomain, ksPassword, KeyStore, aliasEC, "EC", "SHA256withECDSA")
TenantKeyPairUtil->>CryptoProvider: generate EC KeyPair (secp256r1)
CryptoProvider-->>TenantKeyPairUtil: EC KeyPair
TenantKeyPairUtil->>TenantKeyPairUtil: create self-signed X.509 cert (EC)
TenantKeyPairUtil->>KeyStore: setPrivateKeyEntry(aliasEC, privateKey, ksPassword, certChain)
KeyStore-->>TenantKeyPairUtil: ack
KeyStoreGenerator->>KeyStore: persist keystore to disk
KeyStore-->>KeyStoreGenerator: persist result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.java
Show resolved
Hide resolved
...n.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java
Show resolved
Hide resolved
...n.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 2 | ||
| #### Log Improvement Suggestion No: 3 | ||
| #### Log Improvement Suggestion No: 4 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.java (1)
202-205: Remove dead code:generatePubKeyFileNameAppender()is not called anywhere.The private method at lines 202-205 is defined but never invoked within the codebase. It should be removed as part of code cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.java` around lines 202 - 205, The private method generatePubKeyFileNameAppender() is dead code and should be deleted from the KeyStoreGenerator class; remove the entire method definition and, after removal, also remove any now-unused imports or references such as UUIDGenerator if they are no longer used elsewhere in the class to keep imports clean.components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java (4)
87-87: Remove redundant null check.
kpgis guaranteed to be non-null at this point—all code paths either assign it or throwIllegalArgumentException. TheObjects.requireNonNull()call is unnecessary.♻️ Suggested fix
- KeyPair keyPair = Objects.requireNonNull(kpg).generateKeyPair(); + KeyPair keyPair = kpg.generateKeyPair();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` at line 87, Remove the redundant null-check wrapper on the KeyPairGenerator: replace the Objects.requireNonNull(kpg).generateKeyPair() invocation with a direct call to kpg.generateKeyPair() in TenantKeyPairUtil (the variable kpg is already validated or an exception is thrown earlier), ensuring no behavioral change but removing the unnecessary Objects.requireNonNull call.
112-112: Consider increasing serial number entropy.A 32-bit serial number provides limited uniqueness (~4 billion values). For better collision resistance across many tenants over time, consider using at least 64 bits (or 128 bits to align with CA/Browser Forum recommendations for certificate serial numbers).
♻️ Suggested fix
- BigInteger serialNumber = new BigInteger(32, new SecureRandom()); + BigInteger serialNumber = new BigInteger(128, new SecureRandom());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` at line 112, The serial number generation in TenantKeyPairUtil uses new BigInteger(32, new SecureRandom()) which yields only 32 bits of entropy; update the serialNumber creation to use at least 64 bits (preferably 128 bits to follow CA/B Forum guidance) by replacing the 32 with 64 or 128 in the BigInteger constructor so the variable serialNumber has higher collision resistance.
74-76: Clarify the purpose ofCryptoUtil.getDefaultCryptoUtil()call.The return value is discarded. If this is intended to initialize the crypto provider as a side effect, add a comment explaining this. Otherwise, remove the unused call.
♻️ Suggested clarification
try { + // Initialize the default crypto utility provider (side effect). CryptoUtil.getDefaultCryptoUtil(); KeyPairGenerator kpg;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` around lines 74 - 76, The call to CryptoUtil.getDefaultCryptoUtil() in TenantKeyPairUtil is currently discarding its return value; either remove this unused call or explicitly document why it is invoked for its side-effect (e.g., to initialize the crypto provider) so future readers understand intent. Locate the try block in TenantKeyPairUtil where CryptoUtil.getDefaultCryptoUtil() is called and either (a) delete the call if not needed, or (b) keep it and add a one-line comment immediately above explaining the required side-effect initialization, ensuring behavior remains unchanged.
108-109: Extract magic numbers to named constants for clarity.The validity period calculations use magic numbers. Consider extracting these to constants for improved readability and maintainability.
♻️ Suggested improvement
+ private static final long MILLIS_PER_DAY = 1000L * 60 * 60 * 24; + private static final int NOT_BEFORE_DAYS = 30; + private static final int VALIDITY_YEARS = 10; + private static X509Certificate generateCertificate(String tenantDomain, KeyPair keyPair, String algorithmId) throws CertificateException, OperatorCreationException { String commonName = "CN=" + tenantDomain + ", OU=None, O=None, L=None, C=None"; //generate certificate X500Name distinguishedName = new X500Name(commonName); - Date notBefore = new Date(System.currentTimeMillis() - 1000L * 60 * 60 * 24 * 30); - Date notAfter = new Date(System.currentTimeMillis() + (1000L * 60 * 60 * 24 * 365 * 10)); + Date notBefore = new Date(System.currentTimeMillis() - MILLIS_PER_DAY * NOT_BEFORE_DAYS); + Date notAfter = new Date(System.currentTimeMillis() + MILLIS_PER_DAY * 365 * VALIDITY_YEARS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` around lines 108 - 109, The Date calculations for notBefore and notAfter in TenantKeyPairUtil use magic numbers; extract these numeric literals into clearly named constants (e.g., MILLIS_PER_SECOND, MILLIS_PER_MINUTE, MILLIS_PER_HOUR, MILLIS_PER_DAY and then DAYS_30 or YEARS_10_MS) and use them to compute the offsets so the intent (30 days before, 10 years after) is explicit; update the code that sets notBefore and notAfter to reference those constants and ensure they are declared as private static final long constants in the TenantKeyPairUtil class.components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.java (1)
23-38: Add private constructor to prevent instantiation.This is a constants-only utility class. Adding a private constructor prevents accidental instantiation and clarifies intent.
♻️ Suggested fix
public class TenantKeyPairConstants { + private TenantKeyPairConstants() { + // Prevent instantiation + } + // Constants required for EC key pair generation for existing tenants for backward compatibility🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.java` around lines 23 - 38, Add a private no-arg constructor to the TenantKeyPairConstants class to prevent instantiation: locate the class TenantKeyPairConstants and add a private constructor (e.g., private TenantKeyPairConstants() { throw new AssertionError("Utility class"); }) so the constants-only utility class cannot be instantiated and intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.java`:
- Around line 31-32: In TenantKeyPairConstants, mark the weak constants RSA_MD5
and RSA_SHA1 as deprecated and add Javadoc that explains they are retained only
for backward compatibility and recommend using SHA-256+ (e.g., RSA_SHA256)
instead; update any code paths that select default/signature algorithms (search
for usages of RSA_MD5, RSA_SHA1 and any default selection logic) to ensure new
tenant keystores default to RSA_SHA256 or stronger, and add a short migration
note in the Javadoc pointing to how to opt into legacy algorithms if absolutely
required.
---
Nitpick comments:
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.java`:
- Around line 202-205: The private method generatePubKeyFileNameAppender() is
dead code and should be deleted from the KeyStoreGenerator class; remove the
entire method definition and, after removal, also remove any now-unused imports
or references such as UUIDGenerator if they are no longer used elsewhere in the
class to keep imports clean.
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.java`:
- Around line 23-38: Add a private no-arg constructor to the
TenantKeyPairConstants class to prevent instantiation: locate the class
TenantKeyPairConstants and add a private constructor (e.g., private
TenantKeyPairConstants() { throw new AssertionError("Utility class"); }) so the
constants-only utility class cannot be instantiated and intent is clear.
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java`:
- Line 87: Remove the redundant null-check wrapper on the KeyPairGenerator:
replace the Objects.requireNonNull(kpg).generateKeyPair() invocation with a
direct call to kpg.generateKeyPair() in TenantKeyPairUtil (the variable kpg is
already validated or an exception is thrown earlier), ensuring no behavioral
change but removing the unnecessary Objects.requireNonNull call.
- Line 112: The serial number generation in TenantKeyPairUtil uses new
BigInteger(32, new SecureRandom()) which yields only 32 bits of entropy; update
the serialNumber creation to use at least 64 bits (preferably 128 bits to follow
CA/B Forum guidance) by replacing the 32 with 64 or 128 in the BigInteger
constructor so the variable serialNumber has higher collision resistance.
- Around line 74-76: The call to CryptoUtil.getDefaultCryptoUtil() in
TenantKeyPairUtil is currently discarding its return value; either remove this
unused call or explicitly document why it is invoked for its side-effect (e.g.,
to initialize the crypto provider) so future readers understand intent. Locate
the try block in TenantKeyPairUtil where CryptoUtil.getDefaultCryptoUtil() is
called and either (a) delete the call if not needed, or (b) keep it and add a
one-line comment immediately above explaining the required side-effect
initialization, ensuring behavior remains unchanged.
- Around line 108-109: The Date calculations for notBefore and notAfter in
TenantKeyPairUtil use magic numbers; extract these numeric literals into clearly
named constants (e.g., MILLIS_PER_SECOND, MILLIS_PER_MINUTE, MILLIS_PER_HOUR,
MILLIS_PER_DAY and then DAYS_30 or YEARS_10_MS) and use them to compute the
offsets so the intent (30 days before, 10 years after) is explicit; update the
code that sets notBefore and notAfter to reference those constants and ensure
they are declared as private static final long constants in the
TenantKeyPairUtil class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e99d603-52ec-4fdb-a9ab-a3d11eb479b7
📒 Files selected for processing (4)
components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.javacomponents/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.javacomponents/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.javapom.xml
...ant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java (3)
112-113: Consider extracting certificate validity constants.The magic numbers for certificate validity (30 days backdated, 10 years forward) could be extracted to named constants for improved readability and easier future modification.
♻️ Suggested constants
private static final long CERT_NOT_BEFORE_DAYS = 30; private static final long CERT_VALIDITY_YEARS = 10;Then use:
- Date notBefore = new Date(System.currentTimeMillis() - 1000L * 60 * 60 * 24 * 30); - Date notAfter = new Date(System.currentTimeMillis() + (1000L * 60 * 60 * 24 * 365 * 10)); + Date notBefore = new Date(System.currentTimeMillis() - 1000L * 60 * 60 * 24 * CERT_NOT_BEFORE_DAYS); + Date notAfter = new Date(System.currentTimeMillis() + (1000L * 60 * 60 * 24 * 365 * CERT_VALIDITY_YEARS));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` around lines 112 - 113, Extract the hard-coded validity durations in TenantKeyPairUtil (the notBefore and notAfter calculations) into named constants, e.g. CERT_NOT_BEFORE_DAYS and CERT_VALIDITY_YEARS, and replace the inline magic numbers with expressions using those constants (compute milliseconds from days/years, preferably via TimeUnit or clearly documented multipliers) so the notBefore and notAfter assignments read from the constants instead of literal numbers; keep the semantics (30 days backdated, 10 years forward) but centralize the values for readability and future change.
80-80: Consider adding a comment explaining the side-effect initialization.
CryptoUtil.getDefaultCryptoUtil()is called but its return value is discarded. This appears to be intentional to ensure the JCE provider is registered before key generation. A brief inline comment would clarify this intent for future maintainers.📝 Suggested clarification
try { - CryptoUtil.getDefaultCryptoUtil(); + // Initialize CryptoUtil to ensure JCE provider is registered + CryptoUtil.getDefaultCryptoUtil(); KeyPairGenerator kpg;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` at line 80, Add an inline comment in TenantKeyPairUtil next to the call to CryptoUtil.getDefaultCryptoUtil() explaining that the call is intentionally used for its side-effect (registering the JCE provider) and that its return value is discarded; reference the CryptoUtil.getDefaultCryptoUtil() invocation so future maintainers understand it’s for initialization prior to key generation rather than an accidental unused call.
92-92: Redundant null check.
Objects.requireNonNull(kpg)is unnecessary here. The variablekpgis guaranteed to be non-null after the if-else block completes successfully—either it's assigned in one of the branches, or anIllegalArgumentExceptionis thrown. You can callgenerateKeyPair()directly onkpg.♻️ Proposed simplification
- KeyPair keyPair = Objects.requireNonNull(kpg).generateKeyPair(); + KeyPair keyPair = kpg.generateKeyPair();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` at line 92, Remove the redundant null-check wrapper and call generateKeyPair() directly on kpg: replace the expression Objects.requireNonNull(kpg).generateKeyPair() in TenantKeyPairUtil with kpg.generateKeyPair(), ensuring you reference the existing variable kpg and keep the surrounding logic 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
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java`:
- Around line 74-77: The logging guard in TenantKeyPairUtil uses
LOG.isDebugEnabled() but calls LOG.info(), so change the call to LOG.debug(...)
(or alternatively change the guard to LOG.isInfoEnabled() if info-level was
intended); specifically update the block using LOG.isDebugEnabled() and
LOG.info(...) so that the guard and the logging method match (e.g., replace
LOG.info with LOG.debug) to ensure the statement executes when the corresponding
level is enabled.
---
Nitpick comments:
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java`:
- Around line 112-113: Extract the hard-coded validity durations in
TenantKeyPairUtil (the notBefore and notAfter calculations) into named
constants, e.g. CERT_NOT_BEFORE_DAYS and CERT_VALIDITY_YEARS, and replace the
inline magic numbers with expressions using those constants (compute
milliseconds from days/years, preferably via TimeUnit or clearly documented
multipliers) so the notBefore and notAfter assignments read from the constants
instead of literal numbers; keep the semantics (30 days backdated, 10 years
forward) but centralize the values for readability and future change.
- Line 80: Add an inline comment in TenantKeyPairUtil next to the call to
CryptoUtil.getDefaultCryptoUtil() explaining that the call is intentionally used
for its side-effect (registering the JCE provider) and that its return value is
discarded; reference the CryptoUtil.getDefaultCryptoUtil() invocation so future
maintainers understand it’s for initialization prior to key generation rather
than an accidental unused call.
- Line 92: Remove the redundant null-check wrapper and call generateKeyPair()
directly on kpg: replace the expression
Objects.requireNonNull(kpg).generateKeyPair() in TenantKeyPairUtil with
kpg.generateKeyPair(), ensuring you reference the existing variable kpg and keep
the surrounding logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9e25c36-a0ba-442e-b5a1-67315d5a8a2e
📒 Files selected for processing (1)
components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java
...n.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java
Show resolved
Hide resolved
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.
PR Description
Introduces support for generating EC based key pairs for tenant keystores and refactors the tenant keystore provisioning logic.
Key improvements include:
TenantKeyPairUtilto centralize key pair generation and certificate creation logicTenantKeyPairConstantsto define supported algorithms and signature constantsKeyStoreGeneratorto use the new utility and generate both RSA and EC key entriesKeyStoreUtil.getTenantKeyAlias()to resolve the EC key alias for tenantsWith this change, tenant keystores will contain both RSA and EC key entries, enabling improved support for modern cryptographic algorithms while keeping the keystore generation logic modular and reusable.
Summary by CodeRabbit
New Features
Refactor
Chores