Implement AES256 with encryption key input#4512
Merged
AqeelMuhammad merged 16 commits intowso2:4.11.xfrom Mar 6, 2026
Merged
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java
Outdated
Show resolved
Hide resolved
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a new EncryptionKeyCallbackHandler that sources an encryption key from files or console, introduces a new config flag and template entries for key ID handling, bumps several crypto-related Maven properties, and makes minor whitespace formatting edits in DefaultSecretCallbackHandler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SecureVault as SecureVault
participant CallbackHandler as EncryptionKeyCallbackHandler
participant FS as FileSystem
participant Console as Console
participant Logger as Logger
SecureVault->>CallbackHandler: request secret (ENCRYPTION_KEY_PASSWORD)
CallbackHandler->>FS: check platform-specific key files (encryption-key, *-tmp, encryption-key-persist)
alt file found
FS-->>CallbackHandler: return first-line key
CallbackHandler->>FS: if tmp -> rename/overwrite or delete depending on persist flag
else no file
CallbackHandler->>Console: prompt for key input
Console-->>CallbackHandler: user enters key
end
CallbackHandler->>Logger: log events/errors
CallbackHandler-->>SecureVault: provide resolved key
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.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: 1 | N | Not need |
| #### Log Improvement Suggestion No: 2 | N | Not need |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java (2)
167-172:IDENTITY_KEY_PASSWORDhandling relies onprivateKeyPassWordbeing set by a prior keystore callback.When the callback ID is
IDENTITY_KEY_PASSWORD, the method doesn't prompt for any special input — it falls through the normal keystore/private-key prompt path and then returnsprivateKeyPassWord. This works correctly only if this callback arrives after (or as part of the same initialization as) the keystore password prompt. Document this assumption or add a defensive null-check forprivateKeyPassWordbefore setting the secret.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java` around lines 167 - 172, The IDENTITY_KEY_PASSWORD branch in DefaultSecretCallbackHandler assumes privateKeyPassWord is already set; add a defensive null-check before calling singleSecretCallback.setSecret(privateKeyPassWord) (check privateKeyPassWord != null) and if null either fall back to keyStorePassWord, throw a clear exception, or trigger the keystore/private-key prompt to obtain the value, logging a descriptive error; ensure the change touches the block that examines singleSecretCallback.getId() (IDENTITY_KEY_PASSWORD / ENCRYPTION_KEY_PASSWORD) and uses privateKeyPassWord / keyStorePassWord so future callbacks won't return a null secret.
56-57: ChangeENCRYPTION_KEY_PASSWORDandIDENTITY_KEY_PASSWORDfrompublictoprivate.These constants are not referenced anywhere outside this class. Since they are internal implementation details of the callback handler, they should be private (consistent with the existing
KEY_PASSWORDconstant) to avoid exposing internal identifiers as part of the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java` around lines 56 - 57, Change the visibility of the two constants ENCRYPTION_KEY_PASSWORD and IDENTITY_KEY_PASSWORD in class DefaultSecretCallbackHandler from public to private; these are internal identifiers (like the existing KEY_PASSWORD constant) and should not be exposed in the public API, so update their declarations to private static final String and leave names/values 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
`@core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java`:
- Around line 138-144: Introduce a separate field (e.g., encryptionKeyPassWord)
in DefaultSecretCallbackHandler and use it for ENCRYPTION_KEY_PASSWORD instead
of reusing privateKeyPassWord; change the block that handles
singleSecretCallback.getId().equals(ENCRYPTION_KEY_PASSWORD) to set
encryptionKeyPassWord (via console or other source) and call
singleSecretCallback.setSecret(encryptionKeyPassWord) without mutating
privateKeyPassWord or relying on the early-return to short-circuit other
initialization; also remove ENCRYPTION_KEY_PASSWORD from any shared-null guards
or conditional checks that assume privateKeyPassWord controls flow so
keyStorePassWord and privateKeyPassWord initialization logic (the existing
fields keyStorePassWord and privateKeyPassWord and their initialization guards)
remain correct and independent.
In `@parent/pom.xml`:
- Line 601: The pom now references unreleased artifact versions
(securevault.version 1.1.17 and updated cipher-tool, carbon.crypto and axis2
versions) which will break the build; do not merge until those four artifacts
are published—verify the presence of securevault 1.1.17, ciphertool 1.1.29,
carbon.crypto 1.1.16 and axis2 1.6.1-wso2v121 in the WSO2/Maven repositories and
only then update/commit the securevault.version property (and any corresponding
version properties or dependency entries) or roll back to previously released
versions if they remain unavailable.
---
Nitpick comments:
In
`@core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java`:
- Around line 167-172: The IDENTITY_KEY_PASSWORD branch in
DefaultSecretCallbackHandler assumes privateKeyPassWord is already set; add a
defensive null-check before calling
singleSecretCallback.setSecret(privateKeyPassWord) (check privateKeyPassWord !=
null) and if null either fall back to keyStorePassWord, throw a clear exception,
or trigger the keystore/private-key prompt to obtain the value, logging a
descriptive error; ensure the change touches the block that examines
singleSecretCallback.getId() (IDENTITY_KEY_PASSWORD / ENCRYPTION_KEY_PASSWORD)
and uses privateKeyPassWord / keyStorePassWord so future callbacks won't return
a null secret.
- Around line 56-57: Change the visibility of the two constants
ENCRYPTION_KEY_PASSWORD and IDENTITY_KEY_PASSWORD in class
DefaultSecretCallbackHandler from public to private; these are internal
identifiers (like the existing KEY_PASSWORD constant) and should not be exposed
in the public API, so update their declarations to private static final String
and leave names/values unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.javadistribution/kernel/carbon-home/repository/resources/conf/default.jsondistribution/kernel/carbon-home/repository/resources/conf/templates/repository/conf/carbon.xml.j2parent/pom.xml
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java
Outdated
Show resolved
Hide resolved
AqeelMuhammad
commented
Feb 24, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java
Outdated
Show resolved
Hide resolved
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java`:
- Around line 142-148: The handler currently allows encryptionKey (and other
secrets) to be set to null when System.console() is null or
Console.readPassword(...) returns null; update DefaultSecretCallbackHandler so
that in the branch handling ENCRYPTION_KEY_PASSWORD (and the equivalent branches
for other secrets) you check System.console() and the result of
readPassword(...) and throw a clear IllegalStateException (or similar) with a
descriptive message if either is null instead of calling
singleSecretCallback.setSecret(null); reference singleSecretCallback,
ENCRYPTION_KEY_PASSWORD, encryptionKey, System.console(), readPassword(...), and
singleSecretCallback.setSecret(...) when implementing the guard and exception.
- Around line 99-101: The file-based flow currently couples encryptionKey to the
keystore password by unconditionally assigning encryptionKey = passwords[0] in
DefaultSecretCallbackHandler (references: encryptionKey, keyStorePassWord,
readPassword); change this so the file format can optionally supply a distinct
encryption key: update the password parsing logic (readPassword or its caller)
to recognize a dedicated ENCRYPTION_KEY_PASSWORD entry or a second password
slot, set encryptionKey to that distinct value when present, and fall back to
passwords[0] only when no explicit encryption key is provided; ensure
keyStorePassWord remains passwords[0] and maintain backward compatibility with
existing files.
- Line 142: Extract singleSecretCallback.getId() into a local variable (e.g.,
callbackId) in DefaultSecretCallbackHandler and replace the three direct calls
like singleSecretCallback.getId().equals(ENCRYPTION_KEY_PASSWORD) with null-safe
comparisons using CONSTANT.equals(callbackId) (for ENCRYPTION_KEY_PASSWORD and
the other two constants used in the same method). This avoids NPEs if getId()
returns null and matches the pattern used by SecretManagerSecretCallbackHandler;
apply the same change to each occurrence in the method.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.javaparent/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- parent/pom.xml
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java
Outdated
Show resolved
Hide resolved
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java
Outdated
Show resolved
Hide resolved
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java
Outdated
Show resolved
Hide resolved
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Show resolved
Hide resolved
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.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: 1 | Y | Seems ok |
| #### Log Improvement Suggestion No: 2 | N | Security concerns |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java`:
- Around line 72-85: In handleSingleSecretCallback, avoid resolving
encryptionKey for unrelated or null callbacks: first check singleSecretCallback
is non-null and singleSecretCallback.getId() equals ENCRYPTION_KEY_PASSWORD, and
return early if not; only then enter the synchronized block and call
resolveEncryptionKey() to initialize encryptionKey (use the existing
double-checked locking around encryptionKey); finally set the secret on
singleSecretCallback. This keeps resolveEncryptionKey() from running for
irrelevant callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0083ff2-7a71-44eb-ba55-9650655c70a5
📒 Files selected for processing (2)
core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.javacore/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
✅ Files skipped from review due to trivial changes (1)
- core/org.wso2.carbon.securevault/src/main/java/org/wso2/carbon/securevault/DefaultSecretCallbackHandler.java
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Show resolved
Hide resolved
PasanT9
reviewed
Mar 5, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Outdated
Show resolved
Hide resolved
PasanT9
reviewed
Mar 5, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Outdated
Show resolved
Hide resolved
PasanT9
reviewed
Mar 5, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Outdated
Show resolved
Hide resolved
PasanT9
reviewed
Mar 5, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Show resolved
Hide resolved
chamilaadhi
reviewed
Mar 5, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Outdated
Show resolved
Hide resolved
PasanT9
reviewed
Mar 6, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Outdated
Show resolved
Hide resolved
PasanT9
reviewed
Mar 6, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Outdated
Show resolved
Hide resolved
PasanT9
reviewed
Mar 6, 2026
...rbon.securevault/src/main/java/org/wso2/carbon/securevault/EncryptionKeyCallbackHandler.java
Show resolved
Hide resolved
PasanT9
approved these changes
Mar 6, 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
3 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.
Purpose
This pull request introduces enhancements to the encryption key handling and updates several dependency versions to support the feature. The most significant changes are the addition of new configuration options for encryption keys and key IDs, and updates to core dependencies.
Encryption key handling improvements:
ENCRYPTION_KEY_PASSWORDandIDENTITY_KEY_PASSWORDtoDefaultSecretCallbackHandler, and updated logic to prompt for and handle these keys separately during secret callback processing. [1] [2] [3]encryption.enable_key_idproperty indefault.jsonand template support for<EnableKeyId>and<OldSecret>incarbon.xml.j2, allowing for more flexible encryption key management and migration. [1] [2]Dependency updates:
carbon.crypto.versionto1.1.16,securevault.versionto1.1.17,version.ciphertoolto1.1.29, andversion.axis2to1.6.1-wso2v121inpom.xml. [1] [2] [3]Related Issue
Summary by CodeRabbit
New Features
Chores