Add ECDSA support#326
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughOverviewThis pull request adds Elliptic Curve Digital Signature Algorithm (ECDSA) support to the carbon-multitenancy repository, enabling tenant keystore generation to include EC (P-256 / secp256r1) key pairs alongside existing RSA key pairs. Key ChangesNew utility classes
Refactored keystore generation
Dependencies
ImpactTenant keystore management can now generate and store EC (P-256) key material in addition to RSA, using shared constants and a centralized helper to improve maintainability and consistency across key-entry provisioning. WalkthroughThe pull request adds EC (P-256) key pair support to tenant keystore generation. A new Sequence DiagramsequenceDiagram
participant KeyStoreGenerator
participant TenantKeyPairUtil
participant BouncyCastle
participant KeyStore
KeyStoreGenerator->>KeyStore: create/load default keystore
KeyStoreGenerator->>TenantKeyPairUtil: addKeyEntry(tenantDomain, pwd, keyStore, rsaAlias, RSA_KEY_ALG, sigAlg)
TenantKeyPairUtil->>BouncyCastle: generate RSA keypair + self-signed cert
BouncyCastle-->>TenantKeyPairUtil: RSA X509Certificate
TenantKeyPairUtil->>KeyStore: setKeyEntry(rsaAlias, rsaPrivKey, certChain)
TenantKeyPairUtil-->>KeyStoreGenerator: RSA X509Certificate
KeyStoreGenerator->>TenantKeyPairUtil: addKeyEntry(tenantDomain, pwd, keyStore, ecAlias, EC_KEY_ALG, EC_SHA256)
TenantKeyPairUtil->>BouncyCastle: generate EC(secp256r1) keypair + self-signed cert
BouncyCastle-->>TenantKeyPairUtil: EC X509Certificate
TenantKeyPairUtil->>KeyStore: setKeyEntry(ecAlias, ecPrivKey, certChain)
TenantKeyPairUtil-->>KeyStoreGenerator: EC X509Certificate
KeyStoreGenerator->>KeyStore: persist using RSA public certificate
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.java (1)
244-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault signature algorithm should use a stronger hash function.
The fallback to
RSA_MD5at line 253 uses a hash function that is no longer recommended for cryptographic operations. Consider changing the default toRSA_SHA256or another algorithm using SHA-256 or stronger.Proposed fix
private static String getSignatureAlgorithm() { String algorithm = ServerConfiguration.getInstance().getFirstProperty(SIGNING_ALG); // Find in a list of supported signature algorithms. for (String supportedAlgorithm : signatureAlgorithms) { if (supportedAlgorithm.equalsIgnoreCase(algorithm)) { return supportedAlgorithm; } } - return RSA_MD5; + return RSA_SHA256; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.java` around lines 244 - 254, The getSignatureAlgorithm method currently returns RSA_MD5 as the default fallback algorithm, which uses a weak hash function that is cryptographically unsound. Replace the RSA_MD5 constant in the return statement with RSA_SHA256 or another algorithm that uses SHA-256 or stronger to ensure the default signature algorithm meets current security standards and best practices for cryptographic operations.
🧹 Nitpick comments (1)
components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java (1)
75-76: ⚡ Quick winClarify the intent of the initialization call.
The return value of
CryptoUtil.getDefaultCryptoUtil()is discarded. If this is intentional for initializing crypto defaults as a side effect, consider adding a brief comment to clarify. Otherwise, if the result is not needed, this call may be unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairUtil.java` around lines 75 - 76, The call to CryptoUtil.getDefaultCryptoUtil() in the try block discards its return value without explanation. If this call is intended to initialize crypto defaults as a side effect, add a clarifying comment above the method call (in the try block) explaining that initialization is required. If the result is actually needed or used elsewhere, verify that it is being properly captured and utilized. If neither case applies and the call is unnecessary, remove it entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.java`:
- Around line 31-32: The constants RSA_MD5 and RSA_SHA1 in the
TenantKeyPairConstants class use weak hash algorithms (MD5 and SHA1) that are no
longer recommended for cryptographic applications. If these constants are
required for backward compatibility, annotate both RSA_MD5 and RSA_SHA1 with the
`@Deprecated` annotation and add JavaDoc comments explaining why these algorithms
are weak and should not be used for new implementations. If backward
compatibility is not required, consider removing these constants entirely from
the class.
---
Outside diff comments:
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/KeyStoreGenerator.java`:
- Around line 244-254: The getSignatureAlgorithm method currently returns
RSA_MD5 as the default fallback algorithm, which uses a weak hash function that
is cryptographically unsound. Replace the RSA_MD5 constant in the return
statement with RSA_SHA256 or another algorithm that uses SHA-256 or stronger to
ensure the default signature algorithm meets current security standards and best
practices for cryptographic operations.
---
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 75-76: The call to CryptoUtil.getDefaultCryptoUtil() in the try
block discards its return value without explanation. If this call is intended to
initialize crypto defaults as a side effect, add a clarifying comment above the
method call (in the try block) explaining that initialization is required. If
the result is actually needed or used elsewhere, verify that it is being
properly captured and utilized. If neither case applies and the call is
unnecessary, remove it entirely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f7a3f76-1c2d-4f3a-9fda-83bd1e5794c7
📒 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
| public static final String RSA_MD5 = "MD5withRSA"; | ||
| public static final String RSA_SHA1 = "SHA1withRSA"; |
There was a problem hiding this comment.
Consider removing or deprecating weak signature algorithm constants.
MD5withRSA and SHA1withRSA use hash functions that are no longer recommended for new cryptographic applications. Including them as constants may encourage continued use. If backward compatibility is required, consider annotating these with @Deprecated and adding documentation discouraging their use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/tenant-mgt/org.wso2.carbon.tenant.keystore.mgt/src/main/java/org/wso2/carbon/keystore/mgt/util/TenantKeyPairConstants.java`
around lines 31 - 32, The constants RSA_MD5 and RSA_SHA1 in the
TenantKeyPairConstants class use weak hash algorithms (MD5 and SHA1) that are no
longer recommended for cryptographic applications. If these constants are
required for backward compatibility, annotate both RSA_MD5 and RSA_SHA1 with the
`@Deprecated` annotation and add JavaDoc comments explaining why these algorithms
are weak and should not be used for new implementations. If backward
compatibility is not required, consider removing these constants entirely from
the class.
Source: Linters/SAST tools
1 participant
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning