Support keystore with multiple alias entries #18
Conversation
…to true when the keystore location string is actually a base64 encoded keystore string used in the PCF environments.
…to true when the keystore location string is actually a base64 encoded keystore string used in the PCF environments.
…h multiple alias entries
…h multiple alias entries
…h multiple alias entries
|
Thanks for the PR. This seems like a nice improvement however since it's introducing a new configuration, you need to create a KIP to propose this change. See https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals |
|
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
|
@rahulnirgude Are you still interested in contributing this feature? I see you drafted a KIP (https://cwiki.apache.org/confluence/display/KAFKA/KIP-1117%3A+Support+keystore+with+multiple+alias+entries) but it's not complete and you've not started a discussion on the mailing list. |
|
@mimaison , Please find the KIP https://cwiki.apache.org/confluence/display/KAFKA/KIP-1117%3A+Support+keystore+with+multiple+alias+entries |
|
I commented in your DISCUSS thread on the mailing list: https://lists.apache.org/thread/mwpm54kwjsj9vobzkovwp4r5ts1t4jo0 |
|
@mimaison , I have updated the KIP (https://cwiki.apache.org/confluence/display/KAFKA/KIP-1117%3A+Support+keystore+with+multiple+alias+entries ) and code as per the review comments |
WalkthroughA new SSL configuration property, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SslConfigs
participant DefaultSslEngineFactory
participant KeyManager
Client->>SslConfigs: Provide ssl.keystore.alias in config
Client->>DefaultSslEngineFactory: Initialize SSL context with config
DefaultSslEngineFactory->>DefaultSslEngineFactory: createSSLContext(keystore, truststore, configs)
DefaultSslEngineFactory->>DefaultSslEngineFactory: applyAliasToKM(KeyManager[], alias)
DefaultSslEngineFactory->>KeyManager: Wrap with alias selection logic
DefaultSslEngineFactory-->>Client: SSL context uses specified alias for handshake
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Usually when people address reviews and make updates to their KIP, they also reply to the DISCUSS thread, you know to keep the discussion going. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces the ssl.keystore.alias configuration option to allow clients to specify a key alias when the keystore contains multiple keys. This ensures the correct key is used for SSL/TLS communication with Kafka brokers, preventing potential handshake or authorization issues. The changes involve adding the new configuration option, updating the DefaultSslEngineFactory to handle the alias, and adding a unit test for the new functionality.
Highlights
- New Configuration Option: Introduces
ssl.keystore.aliasto specify the key alias in the keystore. - Key Manager Implementation: Modifies the key manager implementation to return the named key based on the provided alias.
- SSL Engine Creation: Updates the SSL engine creation process to use the specified key alias when available.
- Unit Test: Adds a unit test to verify the functionality of applying the alias to the key manager.
Changelog
Click here to see the changelog
- clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java
- Added
SSL_KEYSTORE_ALIAS_CONFIGandSSL_KEYSTORE_ALIAS_DOCconstants to define the new configuration option. - Added
ssl.keystore.aliasto the configuration definition with type STRING and HIGH importance.
- Added
- clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java
- Added imports for classes related to SSL/TLS and security.
- Modified
configuremethod to pass the configs tocreateSSLContext. - Modified
createSSLContextmethod to retrieve the key managers and trust managers. - Modified
createSSLContextmethod to callapplyAliasToKMto apply the alias to the key managers. - Added
applyAliasToKMmethod to wrap the key managers with anX509ExtendedKeyManagerthat chooses the specified alias. - Added logging to
applyAliasToKMto indicate when a custom KeyManager is being applied.
- clients/src/test/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactoryTest.java
- Added imports for KeyManager and Method.
- Added a test case
testApplyAliasToKMto test theapplyAliasToKMmethod using reflection.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In SSL's embrace,
Aliases dance, keys find their place,
Security's art.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration option ssl.keystore.alias to allow clients to specify a key alias when the keystore contains multiple keys. This enhancement improves the flexibility of SSL/TLS configuration for Kafka clients. The implementation appears well-structured, but I have a few suggestions to enhance clarity and robustness.
Summary of Findings
- Missing Null Check: The
applyAliasToKMmethod inDefaultSslEngineFactory.javacould benefit from a null check on thekmsparameter to prevent potentialNullPointerException. - Incomplete Test Coverage: The test case
testApplyAliasToKMinDefaultSslEngineFactoryTest.javaonly validates that the method returns a non-null array of the correct length. It does not verify that the alias is correctly applied to theKeyManager.
Merge Readiness
The pull request introduces a valuable feature with a reasonable implementation. However, addressing the identified issues, particularly the missing null check and incomplete test coverage, would significantly improve the robustness and reliability of the code. I recommend addressing these issues before merging. I am unable to directly approve this pull request, and other reviewers should also examine the code before a merge occurs.
| // Validate results (Modify based on actual method behavior) | ||
| assertNotNull(result); | ||
| assertEquals(1, result.length); |
There was a problem hiding this comment.
The test case only verifies that the method returns a non-null array of the correct length. It does not validate that the alias is correctly applied to the KeyManager. Add assertions to verify that the returned KeyManager uses the specified alias.
assertNotNull(result);
assertEquals(1, result.length);
// Additional validation to check if alias is applied
assertTrue(result[0] instanceof X509ExtendedKeyManager, "Resulting KeyManager should be an X509ExtendedKeyManager");![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
🔭 Outside diff range comments (1)
clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java (1)
166-188:⚠️ Potential issueUpdate reconfigurable configs sets
The new
ssl.keystore.aliasconfig is missing from both theRECONFIGURABLE_CONFIGSandNON_RECONFIGURABLE_CONFIGSsets. This needs to be fixed to ensure proper handling during configuration updates.Apply this diff to add the new config to the appropriate set (likely
RECONFIGURABLE_CONFIGS):public static final Set<String> RECONFIGURABLE_CONFIGS = Utils.mkSet( SslConfigs.SSL_KEYSTORE_TYPE_CONFIG, SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG, SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG, SslConfigs.SSL_KEY_PASSWORD_CONFIG, + SslConfigs.SSL_KEYSTORE_ALIAS_CONFIG, SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG, SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG, SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG, SslConfigs.SSL_KEYSTORE_CERTIFICATE_CHAIN_CONFIG, SslConfigs.SSL_KEYSTORE_KEY_CONFIG, SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_CONFIG);
🧹 Nitpick comments (2)
clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java (2)
33-59: Clean up duplicate importsThere are several duplicate imports in this section, which should be cleaned up for better code hygiene.
Apply this diff to clean up the imports:
-import org.apache.kafka.common.KafkaException; -import org.apache.kafka.common.Reconfigurable; -import org.apache.kafka.common.config.ConfigException; -import org.apache.kafka.common.config.SslConfigs; -import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; -import org.apache.kafka.common.config.types.Password; -import org.apache.kafka.common.network.Mode; -import org.apache.kafka.common.utils.Utils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.security.PrivateKey; import javax.net.ssl.KeyManager; import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLEngineResult; import javax.net.ssl.SSLException; import javax.net.ssl.SSLParameters; import javax.net.ssl.TrustManagerFactory; import javax.net.ssl.X509ExtendedKeyManager; import javax.net.ssl.X509KeyManager; -import java.io.ByteArrayInputStream; -import java.io.InputStream; -import javax.net.ssl.X509ExtendedKeyManager; -import javax.net.ssl.X509KeyManager;
76-76: Missing importsNeed to add relevant imports for the new functionality in the
applyAliasToKMmethod.Add this import for proper type checking:
+import java.security.cert.X509Certificate;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java(2 hunks)clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java(6 hunks)clients/src/test/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactoryTest.java(4 hunks)
🔇 Additional comments (4)
clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java (2)
92-94: Proper documentation for the new SSL keystore alias configurationThe new configuration property
ssl.keystore.aliasis well documented, clearly explaining its purpose for selecting a specific named key from keystores with multiple entries.
150-150: Correctly added configuration to client SSL supportThe new configuration is properly registered in the
addClientSslSupportmethod with appropriate type (STRING), no default value, and HIGH importance level.clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java (2)
265-265: Method signature updated to receive full configuration mapThe
createSSLContextmethod has been properly updated to accept the full configuration map, allowing it to access the alias configuration.
289-289: Key manager array correctly wrapped with alias supportThe call to
sslContext.initnow correctly passes the wrapped key managers that enforce the client alias if specified.
| @Test | ||
| void testApplyAliasToKM() throws Exception { | ||
| DefaultSslEngineFactory instance = new DefaultSslEngineFactory(); | ||
| // Mock KeyManager array | ||
| KeyManager mockKeyManager = mock(KeyManager.class); | ||
| KeyManager[] kms = new KeyManager[]{mockKeyManager}; | ||
|
|
||
| // Define the alias | ||
| String alias = "testAlias"; | ||
|
|
||
| // Use reflection to access the private method | ||
| Method method = DefaultSslEngineFactory.class.getDeclaredMethod("applyAliasToKM", KeyManager[].class, String.class); | ||
| method.setAccessible(true); | ||
|
|
||
| // Invoke the method | ||
| KeyManager[] result = (KeyManager[]) method.invoke(instance, (Object) kms, alias); | ||
|
|
||
| // Validate results (Modify based on actual method behavior) | ||
| assertNotNull(result); | ||
| assertEquals(1, result.length); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve unit test for applyAliasToKM method
The test checks that the method returns a non-null array with the expected length, but it doesn't verify the actual behavior - that the key manager properly enforces the alias when requested.
Consider enhancing the test to verify the actual alias substitution behavior:
@Test
void testApplyAliasToKM() throws Exception {
DefaultSslEngineFactory instance = new DefaultSslEngineFactory();
// Mock KeyManager array
X509KeyManager mockKeyManager = mock(X509KeyManager.class);
KeyManager[] kms = new KeyManager[]{mockKeyManager};
// Define the alias
String alias = "testAlias";
// Use reflection to access the private method
Method method = DefaultSslEngineFactory.class.getDeclaredMethod("applyAliasToKM", KeyManager[].class, String.class);
method.setAccessible(true);
// Invoke the method
KeyManager[] result = (KeyManager[]) method.invoke(instance, (Object) kms, alias);
// Validate results
assertNotNull(result);
assertEquals(1, result.length);
+
+ // Verify that the wrapped key manager returns our alias
+ X509ExtendedKeyManager wrappedKM = (X509ExtendedKeyManager) result[0];
+
+ // Mock parameters for the chooseEngineClientAlias method
+ String[] keyTypes = new String[]{"RSA"};
+ Principal[] issuers = new Principal[0];
+ SSLEngine engine = mock(SSLEngine.class);
+
+ // Verify the alias is returned as expected
+ assertEquals(alias, wrappedKM.chooseEngineClientAlias(keyTypes, issuers, engine));
+
+ // Also check client alias method
+ Socket socket = mock(Socket.class);
+ assertEquals(alias, wrappedKM.chooseClientAlias(keyTypes, issuers, socket));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void testApplyAliasToKM() throws Exception { | |
| DefaultSslEngineFactory instance = new DefaultSslEngineFactory(); | |
| // Mock KeyManager array | |
| KeyManager mockKeyManager = mock(KeyManager.class); | |
| KeyManager[] kms = new KeyManager[]{mockKeyManager}; | |
| // Define the alias | |
| String alias = "testAlias"; | |
| // Use reflection to access the private method | |
| Method method = DefaultSslEngineFactory.class.getDeclaredMethod("applyAliasToKM", KeyManager[].class, String.class); | |
| method.setAccessible(true); | |
| // Invoke the method | |
| KeyManager[] result = (KeyManager[]) method.invoke(instance, (Object) kms, alias); | |
| // Validate results (Modify based on actual method behavior) | |
| assertNotNull(result); | |
| assertEquals(1, result.length); | |
| } | |
| @Test | |
| void testApplyAliasToKM() throws Exception { | |
| DefaultSslEngineFactory instance = new DefaultSslEngineFactory(); | |
| // Mock KeyManager array | |
| X509KeyManager mockKeyManager = mock(X509KeyManager.class); | |
| KeyManager[] kms = new KeyManager[]{mockKeyManager}; | |
| // Define the alias | |
| String alias = "testAlias"; | |
| // Use reflection to access the private method | |
| Method method = DefaultSslEngineFactory.class.getDeclaredMethod("applyAliasToKM", KeyManager[].class, String.class); | |
| method.setAccessible(true); | |
| // Invoke the method | |
| KeyManager[] result = (KeyManager[]) method.invoke(instance, (Object) kms, alias); | |
| // Validate results | |
| assertNotNull(result); | |
| assertEquals(1, result.length); | |
| // Verify that the wrapped key manager returns our alias | |
| X509ExtendedKeyManager wrappedKM = (X509ExtendedKeyManager) result[0]; | |
| // Mock parameters for the chooseEngineClientAlias method | |
| String[] keyTypes = new String[] {"RSA"}; | |
| Principal[] issuers = new Principal[0]; | |
| SSLEngine engine = mock(SSLEngine.class); | |
| // Verify the alias is returned as expected | |
| assertEquals(alias, wrappedKM.chooseEngineClientAlias(keyTypes, issuers, engine)); | |
| // Also check client alias method | |
| Socket socket = mock(Socket.class); | |
| assertEquals(alias, wrappedKM.chooseClientAlias(keyTypes, issuers, socket)); | |
| } |
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | ||
| if(alias == null || alias.isEmpty()){ | ||
| return kms; | ||
| } | ||
|
|
||
| log.info("Applying the custom KeyManagers for alias: {}", alias); | ||
|
|
||
| KeyManager[] updatedKMs = new KeyManager[kms.length]; | ||
|
|
||
| int i=0; | ||
| for(KeyManager km : kms){ | ||
| final X509KeyManager origKM = (X509KeyManager)km; | ||
| X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() { | ||
| /* (non-Javadoc) | ||
| * @see javax.net.ssl.X509ExtendedKeyManager#chooseEngineClientAlias(java.lang.String[], java.security.Principal[], javax.net.ssl.SSLEngine) | ||
| */ | ||
| @Override | ||
| public String chooseEngineClientAlias(String[] arg0, | ||
| Principal[] arg1, SSLEngine arg2) { | ||
| return alias; | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getServerAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getServerAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public PrivateKey getPrivateKey(String arg0) { | ||
| return origKM.getPrivateKey(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getClientAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getClientAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getCertificateChain(String arg0) { | ||
| return origKM.getCertificateChain(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseServerAlias(String arg0, Principal[] arg1, Socket arg2) { | ||
| return origKM.chooseServerAlias(arg0, arg1, arg2); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseClientAlias(String[] arg0, Principal[] arg1, Socket arg2) { | ||
| return alias; | ||
| } | ||
| }; | ||
| updatedKMs[i++] = exKM; | ||
| } | ||
| return updatedKMs; | ||
| } |
There was a problem hiding this comment.
Improve robustness of the applyAliasToKM method
The applyAliasToKM method has several issues that should be addressed:
- It logs at INFO level for every context creation, which could be excessive
- It doesn't check if the key manager is of the right type before casting
- It doesn't check if the specified alias actually exists in the keystore
Apply this diff to improve the method:
private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) {
if(alias == null || alias.isEmpty() || kms == null || kms.length == 0){
return kms;
}
- log.info("Applying the custom KeyManagers for alias: {}", alias);
+ log.debug("Applying the custom KeyManagers for alias: {}", alias);
KeyManager[] updatedKMs = new KeyManager[kms.length];
int i=0;
for(KeyManager km : kms){
+ if (!(km instanceof X509KeyManager)) {
+ updatedKMs[i++] = km;
+ continue;
+ }
final X509KeyManager origKM = (X509KeyManager)km;
+
+ // Check if the alias exists in the keystore
+ PrivateKey privateKey = origKM.getPrivateKey(alias);
+ X509Certificate[] certificateChain = origKM.getCertificateChain(alias);
+ if (privateKey == null || certificateChain == null || certificateChain.length == 0) {
+ log.warn("The specified alias '{}' does not exist in the keystore or does not have a private key/certificate chain", alias);
+ }
+
X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() {
/* (non-Javadoc)
* @see javax.net.ssl.X509ExtendedKeyManager#chooseEngineClientAlias(java.lang.String[], java.security.Principal[], javax.net.ssl.SSLEngine)
*/
@Override
public String chooseEngineClientAlias(String[] arg0,
Principal[] arg1, SSLEngine arg2) {
return alias;
}
@Override
public String[] getServerAliases(String arg0, Principal[] arg1) {
return origKM.getServerAliases(arg0, arg1);
}
@Override
public PrivateKey getPrivateKey(String arg0) {
return origKM.getPrivateKey(arg0);
}
@Override
public String[] getClientAliases(String arg0, Principal[] arg1) {
return origKM.getClientAliases(arg0, arg1);
}
@Override
public X509Certificate[] getCertificateChain(String arg0) {
return origKM.getCertificateChain(arg0);
}
@Override
public String chooseServerAlias(String arg0, Principal[] arg1, Socket arg2) {
return origKM.chooseServerAlias(arg0, arg1, arg2);
}
@Override
public String chooseClientAlias(String[] arg0, Principal[] arg1, Socket arg2) {
return alias;
}
};
updatedKMs[i++] = exKM;
}
return updatedKMs;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | |
| if(alias == null || alias.isEmpty()){ | |
| return kms; | |
| } | |
| log.info("Applying the custom KeyManagers for alias: {}", alias); | |
| KeyManager[] updatedKMs = new KeyManager[kms.length]; | |
| int i=0; | |
| for(KeyManager km : kms){ | |
| final X509KeyManager origKM = (X509KeyManager)km; | |
| X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() { | |
| /* (non-Javadoc) | |
| * @see javax.net.ssl.X509ExtendedKeyManager#chooseEngineClientAlias(java.lang.String[], java.security.Principal[], javax.net.ssl.SSLEngine) | |
| */ | |
| @Override | |
| public String chooseEngineClientAlias(String[] arg0, | |
| Principal[] arg1, SSLEngine arg2) { | |
| return alias; | |
| } | |
| @Override | |
| public String[] getServerAliases(String arg0, Principal[] arg1) { | |
| return origKM.getServerAliases(arg0, arg1); | |
| } | |
| @Override | |
| public PrivateKey getPrivateKey(String arg0) { | |
| return origKM.getPrivateKey(arg0); | |
| } | |
| @Override | |
| public String[] getClientAliases(String arg0, Principal[] arg1) { | |
| return origKM.getClientAliases(arg0, arg1); | |
| } | |
| @Override | |
| public X509Certificate[] getCertificateChain(String arg0) { | |
| return origKM.getCertificateChain(arg0); | |
| } | |
| @Override | |
| public String chooseServerAlias(String arg0, Principal[] arg1, Socket arg2) { | |
| return origKM.chooseServerAlias(arg0, arg1, arg2); | |
| } | |
| @Override | |
| public String chooseClientAlias(String[] arg0, Principal[] arg1, Socket arg2) { | |
| return alias; | |
| } | |
| }; | |
| updatedKMs[i++] = exKM; | |
| } | |
| return updatedKMs; | |
| } | |
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | |
| if (alias == null || alias.isEmpty() || kms == null || kms.length == 0) { | |
| return kms; | |
| } | |
| log.debug("Applying the custom KeyManagers for alias: {}", alias); | |
| KeyManager[] updatedKMs = new KeyManager[kms.length]; | |
| int i = 0; | |
| for (KeyManager km : kms) { | |
| // 1) Only wrap X509KeyManager instances | |
| if (!(km instanceof X509KeyManager)) { | |
| updatedKMs[i++] = km; | |
| continue; | |
| } | |
| final X509KeyManager origKM = (X509KeyManager) km; | |
| // 2) Check if the alias actually exists / has a key+cert chain | |
| PrivateKey privateKey = origKM.getPrivateKey(alias); | |
| X509Certificate[] certificateChain = origKM.getCertificateChain(alias); | |
| if (privateKey == null || certificateChain == null || certificateChain.length == 0) { | |
| log.warn( | |
| "The specified alias '{}' does not exist in the keystore or does not have a private key/certificate chain", | |
| alias | |
| ); | |
| } | |
| X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() { | |
| @Override | |
| public String chooseEngineClientAlias(String[] keyTypes, Principal[] issuers, SSLEngine engine) { | |
| return alias; | |
| } | |
| @Override | |
| public String[] getServerAliases(String keyType, Principal[] issuers) { | |
| return origKM.getServerAliases(keyType, issuers); | |
| } | |
| @Override | |
| public PrivateKey getPrivateKey(String alias) { | |
| return origKM.getPrivateKey(alias); | |
| } | |
| @Override | |
| public String[] getClientAliases(String keyType, Principal[] issuers) { | |
| return origKM.getClientAliases(keyType, issuers); | |
| } | |
| @Override | |
| public X509Certificate[] getCertificateChain(String alias) { | |
| return origKM.getCertificateChain(alias); | |
| } | |
| @Override | |
| public String chooseServerAlias(String keyType, Principal[] issuers, Socket socket) { | |
| return origKM.chooseServerAlias(keyType, issuers, socket); | |
| } | |
| @Override | |
| public String chooseClientAlias(String[] keyTypes, Principal[] issuers, Socket socket) { | |
| return alias; | |
| } | |
| }; | |
| updatedKMs[i++] = exKM; | |
| } | |
| return updatedKMs; | |
| } |
|
/refacto-test |
|
The user @visz11 does not have an active seat allocated for code reviews. Please contact your organization admin to assign a seat or upgrade your subscription. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
Code Review: SSL Keystore Alias Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | ||
| if(alias == null || alias.isEmpty()){ | ||
| return kms; | ||
| } |
There was a problem hiding this comment.
Null Check Missing
Method doesn't check if kms parameter is null before using it. If null KeyManager array is passed, NullPointerException will occur when iterating through array elements.
Standards
- Algorithm-Correctness-Null-Safety
- Business-Rule-Validation
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | ||
| if(alias == null || alias.isEmpty()){ | ||
| return kms; | ||
| } |
There was a problem hiding this comment.
Alias Validation Missing
The alias parameter is used without validation beyond null/empty checks. Malicious alias values could potentially cause unexpected behavior in SSL certificate selection.
Standards
- CWE-20
- OWASP-A04
| TrustManager[] trustManagers = getTrustManagers(truststore, tmfAlgorithm); | ||
|
|
||
| sslContext.init(keyManagers, trustManagers, this.secureRandomImplementation); | ||
| sslContext.init(applyAliasToKM(keyManagers, (String)configs.get(SslConfigs.SSL_KEYSTORE_ALIAS_CONFIG)), trustManagers, this.secureRandomImplementation); |
There was a problem hiding this comment.
Null Check Missing
Missing null check for configs.get() result before casting to String. If SSL_KEYSTORE_ALIAS_CONFIG is undefined, potential ClassCastException could occur affecting SSL initialization.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.kafka.common.KafkaException; | ||
| import org.apache.kafka.common.Reconfigurable; | ||
| import org.apache.kafka.common.config.ConfigException; | ||
| import org.apache.kafka.common.config.SslConfigs; | ||
| import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; | ||
| import org.apache.kafka.common.config.types.Password; | ||
| import org.apache.kafka.common.network.Mode; | ||
|
|
||
| import org.apache.kafka.common.utils.Utils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; |
There was a problem hiding this comment.
Duplicate Import Statements
Multiple duplicate import statements for Logger, LoggerFactory, and Utils classes. Duplicate imports increase maintenance burden and create confusion about which import is actually used.
| import org.slf4j.Logger; | |
| import org.slf4j.LoggerFactory; | |
| import org.apache.kafka.common.KafkaException; | |
| import org.apache.kafka.common.Reconfigurable; | |
| import org.apache.kafka.common.config.ConfigException; | |
| import org.apache.kafka.common.config.SslConfigs; | |
| import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; | |
| import org.apache.kafka.common.config.types.Password; | |
| import org.apache.kafka.common.network.Mode; | |
| import org.apache.kafka.common.utils.Utils; | |
| import org.slf4j.Logger; | |
| import org.slf4j.LoggerFactory; | |
| import org.slf4j.Logger; | |
| import org.slf4j.LoggerFactory; | |
| import org.apache.kafka.common.KafkaException; | |
| import org.apache.kafka.common.Reconfigurable; | |
| import org.apache.kafka.common.config.ConfigException; | |
| import org.apache.kafka.common.config.SslConfigs; | |
| import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; | |
| import org.apache.kafka.common.config.types.Password; | |
| import org.apache.kafka.common.network.Mode; | |
| import org.apache.kafka.common.utils.Utils; |
Standards
- Clean-Code-Organization
- Clean-Code-Readability
| import javax.net.ssl.X509ExtendedKeyManager; | ||
| import javax.net.ssl.X509KeyManager; | ||
| import java.io.ByteArrayInputStream; | ||
| import java.io.InputStream; | ||
| import javax.net.ssl.X509ExtendedKeyManager; | ||
| import javax.net.ssl.X509KeyManager; |
There was a problem hiding this comment.
Duplicate KeyManager Imports
Duplicate imports for X509ExtendedKeyManager and X509KeyManager classes. Redundant imports reduce code clarity and create maintenance overhead when managing dependencies.
| import javax.net.ssl.X509ExtendedKeyManager; | |
| import javax.net.ssl.X509KeyManager; | |
| import java.io.ByteArrayInputStream; | |
| import java.io.InputStream; | |
| import javax.net.ssl.X509ExtendedKeyManager; | |
| import javax.net.ssl.X509KeyManager; | |
| import javax.net.ssl.X509ExtendedKeyManager; | |
| import javax.net.ssl.X509KeyManager; | |
| import java.io.ByteArrayInputStream; | |
| import java.io.InputStream; |
Standards
- Clean-Code-Organization
- Clean-Code-Readability
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | ||
| if(alias == null || alias.isEmpty()){ | ||
| return kms; | ||
| } | ||
|
|
||
| log.info("Applying the custom KeyManagers for alias: {}", alias); | ||
|
|
||
| KeyManager[] updatedKMs = new KeyManager[kms.length]; | ||
|
|
||
| int i=0; | ||
| for(KeyManager km : kms){ | ||
| final X509KeyManager origKM = (X509KeyManager)km; | ||
| X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() { | ||
| /* (non-Javadoc) | ||
| * @see javax.net.ssl.X509ExtendedKeyManager#chooseEngineClientAlias(java.lang.String[], java.security.Principal[], javax.net.ssl.SSLEngine) | ||
| */ | ||
| @Override | ||
| public String chooseEngineClientAlias(String[] arg0, | ||
| Principal[] arg1, SSLEngine arg2) { | ||
| return alias; | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getServerAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getServerAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public PrivateKey getPrivateKey(String arg0) { | ||
| return origKM.getPrivateKey(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getClientAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getClientAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getCertificateChain(String arg0) { | ||
| return origKM.getCertificateChain(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseServerAlias(String arg0, Principal[] arg1, Socket arg2) { | ||
| return origKM.chooseServerAlias(arg0, arg1, arg2); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseClientAlias(String[] arg0, Principal[] arg1, Socket arg2) { | ||
| return alias; | ||
| } | ||
| }; | ||
| updatedKMs[i++] = exKM; | ||
| } | ||
| return updatedKMs; | ||
| } |
There was a problem hiding this comment.
Long Method Implementation
Method uses anonymous inner class with multiple overrides creating high complexity. This implementation pattern makes testing difficult and increases maintenance burden when SSL behavior needs modification.
Standards
- Clean-Code-Functions
- Design-Pattern-Decorator
| return kms; | ||
| } | ||
|
|
||
| log.info("Applying the custom KeyManagers for alias: {}", alias); |
There was a problem hiding this comment.
Logging Consistency
Using info level for configuration details is inconsistent with debug level used elsewhere (line 290). Inconsistent logging levels can affect system observability and troubleshooting.
Standards
- ISO-IEC-25010-Reliability-Maturity
- SRE-Observability
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: SSL KeyManager Configuration👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| for(KeyManager km : kms){ | ||
| final X509KeyManager origKM = (X509KeyManager)km; | ||
| X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() { |
There was a problem hiding this comment.
ClassCastException Risk
The code assumes all KeyManager instances are X509KeyManager without verification. This creates a ClassCastException risk if any KeyManager implementation is not an X509KeyManager. Type checking should be added before casting to ensure runtime safety.
| for(KeyManager km : kms){ | |
| final X509KeyManager origKM = (X509KeyManager)km; | |
| X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() { | |
| int i=0; | |
| for(KeyManager km : kms){ | |
| if (!(km instanceof X509KeyManager)) { | |
| log.warn("KeyManager is not an instance of X509KeyManager, skipping alias application for {}", km.getClass().getName()); | |
| updatedKMs[i++] = km; | |
| continue; | |
| } | |
| final X509KeyManager origKM = (X509KeyManager)km; |
Standards
- Algorithm-Correctness-Type-Safety
- Logic-Verification-Runtime-Safety
- Business-Rule-Exception-Prevention
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.kafka.common.KafkaException; | ||
| import org.apache.kafka.common.Reconfigurable; | ||
| import org.apache.kafka.common.config.ConfigException; | ||
| import org.apache.kafka.common.config.SslConfigs; | ||
| import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; | ||
| import org.apache.kafka.common.config.types.Password; | ||
| import org.apache.kafka.common.network.Mode; | ||
|
|
||
| import org.apache.kafka.common.utils.Utils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; |
There was a problem hiding this comment.
Duplicate Import Statements
Duplicate import statements for Logger and LoggerFactory classes reduce code maintainability. Redundant imports create confusion during maintenance and can lead to compilation issues when refactoring.
Standards
- Clean-Code-Organization
- Maintainability-Quality-Readability
| import org.apache.kafka.common.utils.Utils; | ||
|
|
||
| import java.net.Socket; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.kafka.common.KafkaException; | ||
| import org.apache.kafka.common.Reconfigurable; | ||
| import org.apache.kafka.common.config.ConfigException; | ||
| import org.apache.kafka.common.config.SslConfigs; | ||
| import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; | ||
| import org.apache.kafka.common.config.types.Password; | ||
| import org.apache.kafka.common.network.Mode; | ||
|
|
||
| import org.apache.kafka.common.utils.Utils; |
There was a problem hiding this comment.
Duplicate Utils Import
Utils class is imported twice, creating redundancy in the import section. Duplicate imports reduce code maintainability and can cause confusion during refactoring or when resolving import conflicts.
Standards
- Clean-Code-Organization
- Maintainability-Quality-Readability
| import javax.net.ssl.X509ExtendedKeyManager; | ||
| import javax.net.ssl.X509KeyManager; | ||
| import java.io.ByteArrayInputStream; | ||
| import java.io.InputStream; | ||
| import javax.net.ssl.X509ExtendedKeyManager; | ||
| import javax.net.ssl.X509KeyManager; |
There was a problem hiding this comment.
Duplicate KeyManager Imports
X509ExtendedKeyManager and X509KeyManager are imported twice. Duplicate imports reduce code maintainability and can lead to confusion during refactoring or when resolving import conflicts.
Standards
- Clean-Code-Organization
- Maintainability-Quality-Readability
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | ||
| if(alias == null || alias.isEmpty()){ | ||
| return kms; | ||
| } |
There was a problem hiding this comment.
Null Check Missing
The method checks if alias is null or empty but doesn't validate if kms array is null. If kms is null, a NullPointerException will occur when iterating through the array. A null check for kms should be added to ensure robust error handling.
Standards
- Algorithm-Correctness-Input-Validation
- Business-Rule-Null-Safety
- Logic-Verification-Defensive-Programming
| X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() { | ||
| /* (non-Javadoc) | ||
| * @see javax.net.ssl.X509ExtendedKeyManager#chooseEngineClientAlias(java.lang.String[], java.security.Principal[], javax.net.ssl.SSLEngine) | ||
| */ | ||
| @Override | ||
| public String chooseEngineClientAlias(String[] arg0, | ||
| Principal[] arg1, SSLEngine arg2) { | ||
| return alias; | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getServerAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getServerAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public PrivateKey getPrivateKey(String arg0) { | ||
| return origKM.getPrivateKey(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getClientAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getClientAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getCertificateChain(String arg0) { | ||
| return origKM.getCertificateChain(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseServerAlias(String arg0, Principal[] arg1, Socket arg2) { | ||
| return origKM.chooseServerAlias(arg0, arg1, arg2); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseClientAlias(String[] arg0, Principal[] arg1, Socket arg2) { | ||
| return alias; | ||
| } | ||
| }; | ||
| updatedKMs[i++] = exKM; | ||
| } |
There was a problem hiding this comment.
Anonymous Class Complexity
Anonymous inner class implementation creates maintenance challenges. Consider extracting this into a named inner class or separate class to improve readability and testability. The current implementation makes the method overly complex.
Standards
- Clean-Code-Class-Organization
- Refactoring-Extract-Class
- Maintainability-Quality-Complexity
| public String chooseEngineClientAlias(String[] arg0, | ||
| Principal[] arg1, SSLEngine arg2) { | ||
| return alias; | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getServerAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getServerAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public PrivateKey getPrivateKey(String arg0) { | ||
| return origKM.getPrivateKey(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getClientAliases(String arg0, Principal[] arg1) { | ||
| return origKM.getClientAliases(arg0, arg1); | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getCertificateChain(String arg0) { | ||
| return origKM.getCertificateChain(arg0); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseServerAlias(String arg0, Principal[] arg1, Socket arg2) { | ||
| return origKM.chooseServerAlias(arg0, arg1, arg2); | ||
| } | ||
|
|
||
| @Override | ||
| public String chooseClientAlias(String[] arg0, Principal[] arg1, Socket arg2) { | ||
| return alias; |
There was a problem hiding this comment.
Improved Parameter Naming
Non-descriptive parameter names (arg0, arg1, arg2) reduce code readability and maintainability. Meaningful parameter names would improve code clarity and make future modifications easier by clearly communicating parameter purpose.
Standards
- Clean-Code-Naming
- Maintainability-Quality-Readability
| // Validate results (Modify based on actual method behavior) | ||
| assertNotNull(result); | ||
| assertEquals(1, result.length); |
There was a problem hiding this comment.
Insufficient Test Coverage
The test only verifies non-null result and array length but doesn't validate the actual behavior of applying aliases. The comment "Modify based on actual method behavior" indicates incomplete test implementation, reducing maintainability through inadequate verification.
| // Validate results (Modify based on actual method behavior) | |
| assertNotNull(result); | |
| assertEquals(1, result.length); | |
| // Validate results | |
| assertNotNull(result); | |
| assertEquals(1, result.length); | |
| assertTrue(result[0] instanceof javax.net.ssl.X509ExtendedKeyManager, "Result should be an X509ExtendedKeyManager"); | |
| // Test that the alias is correctly applied | |
| X509ExtendedKeyManager extKeyManager = (X509ExtendedKeyManager) result[0]; | |
| assertEquals(alias, extKeyManager.chooseClientAlias(new String[]{"RSA"}, null, null), "Should return configured alias"); | |
| assertEquals(alias, extKeyManager.chooseEngineClientAlias(new String[]{"RSA"}, null, null), "Should return configured alias"); |
Standards
- Maintainability-Quality-Testing
- Clean-Code-Testing
| } | ||
| } | ||
|
|
||
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { |
There was a problem hiding this comment.
Method Documentation Improvement
The applyAliasToKM method lacks documentation explaining its purpose and behavior. Adding Javadoc would improve maintainability by helping future developers understand the method's role in keystore alias selection and its impact on SSL configuration.
Standards
- Clean-Code-Comments
- Maintainability-Quality-Documentation
| /* (non-Javadoc) | ||
| * @see javax.net.ssl.X509ExtendedKeyManager#chooseEngineClientAlias(java.lang.String[], java.security.Principal[], javax.net.ssl.SSLEngine) | ||
| */ |
There was a problem hiding this comment.
Incomplete Javadoc
Incomplete Javadoc comment that only references the overridden method without describing implementation details or behavior changes. Proper documentation is essential for security-critical code to ensure correct usage and maintenance.
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: SSL KeyManager Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.kafka.common.KafkaException; | ||
| import org.apache.kafka.common.Reconfigurable; | ||
| import org.apache.kafka.common.config.ConfigException; | ||
| import org.apache.kafka.common.config.SslConfigs; | ||
| import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; | ||
| import org.apache.kafka.common.config.types.Password; | ||
| import org.apache.kafka.common.network.Mode; | ||
|
|
||
| import org.apache.kafka.common.utils.Utils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; |
There was a problem hiding this comment.
Duplicate Import Statements
The same imports for Logger and LoggerFactory are duplicated in the file. This creates redundancy and potential confusion during code maintenance. Duplicate imports should be removed to maintain clean code structure.
Standards
- Logic-Verification-Code-Structure
- Algorithm-Correctness-Import-Management
| import org.apache.kafka.common.utils.Utils; | ||
|
|
||
| import java.net.Socket; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.kafka.common.KafkaException; | ||
| import org.apache.kafka.common.Reconfigurable; | ||
| import org.apache.kafka.common.config.ConfigException; | ||
| import org.apache.kafka.common.config.SslConfigs; | ||
| import org.apache.kafka.common.config.internals.BrokerSecurityConfigs; | ||
| import org.apache.kafka.common.config.types.Password; | ||
| import org.apache.kafka.common.network.Mode; | ||
|
|
||
| import org.apache.kafka.common.utils.Utils; |
There was a problem hiding this comment.
Duplicate Utils Import
The Utils class is imported twice in the file. Duplicate imports create unnecessary code and can lead to confusion during maintenance. One of these import statements should be removed.
Standards
- Logic-Verification-Code-Structure
- Algorithm-Correctness-Import-Management
| import javax.net.ssl.X509ExtendedKeyManager; | ||
| import javax.net.ssl.X509KeyManager; | ||
| import java.io.ByteArrayInputStream; | ||
| import java.io.InputStream; | ||
| import javax.net.ssl.X509ExtendedKeyManager; | ||
| import javax.net.ssl.X509KeyManager; |
There was a problem hiding this comment.
Duplicate X509KeyManager Imports
The X509ExtendedKeyManager and X509KeyManager classes are imported twice. Duplicate imports create unnecessary code and can lead to confusion. These redundant import statements should be removed.
Standards
- Logic-Verification-Code-Structure
- Algorithm-Correctness-Import-Management
| private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) { | ||
| if(alias == null || alias.isEmpty()){ | ||
| return kms; | ||
| } |
There was a problem hiding this comment.
Null Alias Handling
The method checks for null or empty alias but doesn't validate if the alias actually exists in the keystore. This could lead to a situation where a non-existent alias is specified but not verified, potentially causing runtime issues with certificate selection.
Standards
- Business-Rule-Input-Validation
- Logic-Verification-Error-Handling
| if(alias == null || alias.isEmpty()){ | ||
| return kms; | ||
| } |
There was a problem hiding this comment.
Inconsistent Spacing Style
The code uses inconsistent spacing in conditional statements. There's no space after 'if' and before the opening parenthesis, but spaces are used correctly in other parts of the code. This inconsistency can affect code readability and maintainability.
Standards
- Logic-Verification-Code-Structure
- Algorithm-Correctness-Style-Consistency
| return kms; | ||
| } | ||
|
|
||
| log.info("Applying the custom KeyManagers for alias: {}", alias); |
There was a problem hiding this comment.
Logging Improvement Opportunity
The log message is at INFO level which may be too verbose for a configuration detail. Consider using DEBUG level for this type of operational detail, reserving INFO for more significant events that administrators need to be aware of.
Standards
- Logic-Verification-Logging
- Business-Rule-Operational-Visibility
3 participants
Client applications use SSL/TLS to connect with Kafka brokers in order to implement secured communication. The clients initiate SSL communication with Kafka brokers using the SSL Engine constructed from the ssl.* properties pointing to key store and trust store. This PR addresses couple of important enhancements related to how the key store is loaded for secured communication with Kafka brokers.
Problem :
Most of the times, the key store on the client side contains single key. But when the key store contains multiple keys, in order to avoid SSL handshake issues or authorization issues communicating with Kafka brokers, it is required to choose the right key from the key store.
Solution :
The key can be identified via key alias while constructing the SSL engine. This requires client to provide a new property ssl.keystore.alias that points to the key alias within the key store. The key manager implementation is modified to return the named key to be used for building the SSL Engine.
Example configuration:
ssl.keystore.alias=<alias.name>
Ashutosh Gijare and Moreshwar Dayte from Mastercard have contributed to this implementation
Summary by CodeRabbit
New Features
Tests