Add tenant EC and EdDSA key alias resolution support#4547
Merged
hwupathum merged 2 commits intowso2:4.12.xfrom Mar 18, 2026
Merged
Add tenant EC and EdDSA key alias resolution support#4547hwupathum merged 2 commits intowso2:4.12.xfrom
hwupathum merged 2 commits intowso2:4.12.xfrom
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java
Outdated
Show resolved
Hide resolved
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java
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: 1 | ||
| #### Log Improvement Suggestion No: 2 |
📝 WalkthroughWalkthroughAdds ECDSA-aware per-tenant key alias handling to KeyStoreUtil: new TENANT_ECDSA_KEY_SUFFIX, a getTenantECKeyAlias(tenantDomain) method, updated getTenantEdKeyAlias(tenantDomain) to handle SUPER_TENANT_DOMAIN_NAME via server primary alias, and a helper getServerPrimaryKeyAlias(); getPrivateKeyAlias now excludes both EDDSA and ECDSA tenant suffixes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
📝 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 |
hwupathum
reviewed
Mar 16, 2026
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java (1)
60-72:⚠️ Potential issue | 🔴 CriticalBehavioral change introduces NPE risk at KeyStoreManager line 1016.
The modified
getPrivateKeyAliasnow excludes aliases ending with_ecor_ed(mapping to EC/ED key types). This is an intentional behavioral change, and most call sites handle the potential null return safely:
- CertProcessor (line 121): Checks
if(alias != null)before using it- KeyStoreManager (line 240): Checks
if(StringUtils.isNotBlank(pvtKeyAlias))before callingsetPrivateKeyAlias- KeyStoreManager (line 1003): Checks
!= nullwhen setting private store flagHowever, KeyStoreManager line 1016 lacks null protection:
X509Certificate publicCert = (X509Certificate) keyStore.getCertificate(keyStoreModel.getPrivateKeyAlias()); metadata.setPublicCert(publicCert.getEncoded());If
getPrivateKeyAlias()returnsnull(for keystores containing only EC/ED-suffixed keys),getCertificate(null)returnsnull, and the subsequentgetEncoded()call throws aNullPointerException.Add a null check before line 1017:
if (publicCert != null) { metadata.setPublicCert(publicCert.getEncoded()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java` around lines 60 - 72, KeyStoreManager currently assumes getPrivateKeyAlias() yields a non-null alias and calls getCertificate(...).getEncoded(), which can NPE when getPrivateKeyAlias() returns null (e.g., only EC/ED aliases present); locate the code around the call X509Certificate publicCert = (X509Certificate) keyStore.getCertificate(keyStoreModel.getPrivateKeyAlias()) and wrap the metadata.setPublicCert(publicCert.getEncoded()) call with a null-check (if publicCert != null) so you only call getEncoded() when a certificate exists, optionally logging or leaving metadata unchanged when publicCert is null.
🧹 Nitpick comments (1)
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java (1)
48-52: Consider exposing key type constants as public API.The
KEY_TYPE_ALIAS_MAPPINGvalidates thatkeyTypemust be either"EC"or"ED", but these constants are private. Callers ofgetTenantKeyAliasmust know these magic strings. Additionally, the PR description mentions "EDDSA" as a key type, but the actual accepted value is "ED".Consider exposing these constants publicly so callers can use them directly:
♻️ Proposed refactor
- private static final String TENANT_EDDSA_KEY = "ED"; - private static final String TENANT_ECDSA_KEY = "EC"; + public static final String KEY_TYPE_EDDSA = "ED"; + public static final String KEY_TYPE_ECDSA = "EC"; private static final Map<String, String> KEY_TYPE_ALIAS_MAPPING = Map.of( - TENANT_ECDSA_KEY, "_ec", - TENANT_EDDSA_KEY, "_ed"); + KEY_TYPE_ECDSA, "_ec", + KEY_TYPE_EDDSA, "_ed");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java` around lines 48 - 52, Expose the key-type identifiers so callers don't rely on magic strings: change TENANT_EDDSA_KEY and TENANT_ECDSA_KEY to public static final (and add JavaDoc clarifying that TENANT_EDDSA_KEY ("ED") denotes EDDSA), keep KEY_TYPE_ALIAS_MAPPING as the internal mapping used by getTenantKeyAlias, and ensure getTenantKeyAlias's callers can reference the public constants instead of hard-coded "ED"/"EC" values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java`:
- Around line 60-72: KeyStoreManager currently assumes getPrivateKeyAlias()
yields a non-null alias and calls getCertificate(...).getEncoded(), which can
NPE when getPrivateKeyAlias() returns null (e.g., only EC/ED aliases present);
locate the code around the call X509Certificate publicCert = (X509Certificate)
keyStore.getCertificate(keyStoreModel.getPrivateKeyAlias()) and wrap the
metadata.setPublicCert(publicCert.getEncoded()) call with a null-check (if
publicCert != null) so you only call getEncoded() when a certificate exists,
optionally logging or leaving metadata unchanged when publicCert is null.
---
Nitpick comments:
In
`@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java`:
- Around line 48-52: Expose the key-type identifiers so callers don't rely on
magic strings: change TENANT_EDDSA_KEY and TENANT_ECDSA_KEY to public static
final (and add JavaDoc clarifying that TENANT_EDDSA_KEY ("ED") denotes EDDSA),
keep KEY_TYPE_ALIAS_MAPPING as the internal mapping used by getTenantKeyAlias,
and ensure getTenantKeyAlias's callers can reference the public constants
instead of hard-coded "ED"/"EC" values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 533d837b-d968-4443-8d07-0e01a29b24bd
📒 Files selected for processing (1)
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java
hwupathum
reviewed
Mar 16, 2026
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java
Outdated
Show resolved
Hide resolved
hwupathum
reviewed
Mar 17, 2026
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java (1)
56-68:⚠️ Potential issue | 🟠 MajorAvoid returning
nullwhen only suffixed key aliases are present.With the new suffix filters, this method can return
nulleven when the keystore has key entries, which is a compatibility risk for callers that expect an alias.Suggested fix
public static String getPrivateKeyAlias(KeyStore store) throws KeyStoreException { - String alias = null; + String fallbackAlias = null; Enumeration<String> enums = store.aliases(); while (enums.hasMoreElements()) { String name = enums.nextElement(); - if (store.isKeyEntry(name) && !name.endsWith(TENANT_EDDSA_KEY_SUFFIX) - && !name.endsWith(TENANT_ECDSA_KEY_SUFFIX)) { - alias = name; - break; + if (!store.isKeyEntry(name)) { + continue; + } + if (fallbackAlias == null) { + fallbackAlias = name; + } + if (!name.endsWith(TENANT_EDDSA_KEY_SUFFIX) + && !name.endsWith(TENANT_ECDSA_KEY_SUFFIX)) { + return name; } } - return alias; + return fallbackAlias; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java` around lines 56 - 68, getPrivateKeyAlias currently returns null if all key entries end with TENANT_EDDSA_KEY_SUFFIX or TENANT_ECDSA_KEY_SUFFIX; change it to fall back to returning the first available key entry if no non-suffixed alias is found. In getPrivateKeyAlias(KeyStore store) keep the initial loop that looks for a non-suffixed alias (checking TENANT_EDDSA_KEY_SUFFIX and TENANT_ECDSA_KEY_SUFFIX), but if alias is still null after that loop, iterate aliases again and return the first name for which store.isKeyEntry(name) is true; preserve the method signature and KeyStoreException behavior.
🧹 Nitpick comments (1)
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java (1)
271-299: Consolidate duplicated tenant-alias construction into one helper.
getTenantEdKeyAliasandgetTenantECKeyAliascurrently duplicate the same control flow. A shared helper reduces drift and can centrally handle edge cases (e.g., avoid double-appending suffixes).Refactor sketch
public static String getTenantEdKeyAlias(String tenantDomain) throws CarbonException { - - if (StringUtils.isBlank(tenantDomain)) { - throw new CarbonException("Tenant domain must not be empty."); - } else if (SUPER_TENANT_DOMAIN_NAME.equals(tenantDomain)) { - return getServerPrimaryKeyAlias() + TENANT_EDDSA_KEY_SUFFIX; - } else { - return tenantDomain + TENANT_EDDSA_KEY_SUFFIX; - } + return getTenantKeyAliasWithSuffix(tenantDomain, TENANT_EDDSA_KEY_SUFFIX); } public static String getTenantECKeyAlias(String tenantDomain) throws CarbonException { - - if (StringUtils.isBlank(tenantDomain)) { - throw new CarbonException("Tenant domain must not be empty."); - } else if (SUPER_TENANT_DOMAIN_NAME.equals(tenantDomain)) { - return getServerPrimaryKeyAlias() + TENANT_ECDSA_KEY_SUFFIX; - } else { - return tenantDomain + TENANT_ECDSA_KEY_SUFFIX; - } + return getTenantKeyAliasWithSuffix(tenantDomain, TENANT_ECDSA_KEY_SUFFIX); } + +private static String getTenantKeyAliasWithSuffix(String tenantDomain, String suffix) throws CarbonException { + if (StringUtils.isBlank(tenantDomain)) { + throw new CarbonException("Tenant domain must not be empty."); + } + String baseAlias = SUPER_TENANT_DOMAIN_NAME.equals(tenantDomain) + ? getServerPrimaryKeyAlias() + : tenantDomain; + return baseAlias.endsWith(suffix) ? baseAlias : baseAlias + suffix; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java` around lines 271 - 299, Both getTenantEdKeyAlias and getTenantECKeyAlias duplicate the same validation and branching; extract their common logic into a private helper (e.g., getTenantKeyAlias(String tenantDomain, String suffix)) that: validates tenantDomain (throws CarbonException if blank), returns getServerPrimaryKeyAlias() + suffix when tenantDomain equals SUPER_TENANT_DOMAIN_NAME, otherwise returns tenantDomain + suffix, and is used by getTenantEdKeyAlias (passing TENANT_EDDSA_KEY_SUFFIX) and getTenantECKeyAlias (passing TENANT_ECDSA_KEY_SUFFIX); ensure the helper prevents double-appending by treating the suffix as always appended only by the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java`:
- Around line 56-68: getPrivateKeyAlias currently returns null if all key
entries end with TENANT_EDDSA_KEY_SUFFIX or TENANT_ECDSA_KEY_SUFFIX; change it
to fall back to returning the first available key entry if no non-suffixed alias
is found. In getPrivateKeyAlias(KeyStore store) keep the initial loop that looks
for a non-suffixed alias (checking TENANT_EDDSA_KEY_SUFFIX and
TENANT_ECDSA_KEY_SUFFIX), but if alias is still null after that loop, iterate
aliases again and return the first name for which store.isKeyEntry(name) is
true; preserve the method signature and KeyStoreException behavior.
---
Nitpick comments:
In
`@core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java`:
- Around line 271-299: Both getTenantEdKeyAlias and getTenantECKeyAlias
duplicate the same validation and branching; extract their common logic into a
private helper (e.g., getTenantKeyAlias(String tenantDomain, String suffix))
that: validates tenantDomain (throws CarbonException if blank), returns
getServerPrimaryKeyAlias() + suffix when tenantDomain equals
SUPER_TENANT_DOMAIN_NAME, otherwise returns tenantDomain + suffix, and is used
by getTenantEdKeyAlias (passing TENANT_EDDSA_KEY_SUFFIX) and getTenantECKeyAlias
(passing TENANT_ECDSA_KEY_SUFFIX); ensure the helper prevents double-appending
by treating the suffix as always appended only by the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e7c404eb-f480-4854-97c2-d7675133acfa
📒 Files selected for processing (1)
core/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/util/KeyStoreUtil.java
hwupathum
approved these changes
Mar 18, 2026
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
2 participants
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
This change introduces support for resolving tenant specific
ECandEdDSAkey aliases inKeyStoreUtil.The new method
getTenantECKeyAlias(String tenantDomain)resolves the ec alias based on the tenant domain and appends_ecat the end. For the super tenant, the alias is derived from the server primary keystore alias.This improves maintainability and ensures consistent handling of tenant ECDSA key aliases.
Related issues:
wso2/product-is#26486