Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ public class SslConfigs {
public static final String SSL_KEYSTORE_LOCATION_DOC = "The location of the key store file. "
+ "This is optional for client and can be used for two-way authentication for client.";

public static final String SSL_KEYSTORE_ALIAS_CONFIG = "ssl.keystore.alias";
public static final String SSL_KEYSTORE_ALIAS_DOC = "This config is used to pick named alias from the keystore to build the SSL engine and authenticate the client with broker. " +
"This is an optional config and used only when you have multiple keys in the keystore and you need to control which key needs to be presented to server.";


public static final String SSL_KEYSTORE_PASSWORD_CONFIG = "ssl.keystore.password";
public static final String SSL_KEYSTORE_PASSWORD_DOC = "The store password for the key store file. "
+ "This is optional for client and only needed if 'ssl.keystore.location' is configured. "
Expand Down Expand Up @@ -142,6 +147,7 @@ public static void addClientSslSupport(ConfigDef config) {
.define(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, ConfigDef.Type.LIST, SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS, ConfigDef.Importance.MEDIUM, SslConfigs.SSL_ENABLED_PROTOCOLS_DOC)
.define(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG, ConfigDef.Type.STRING, SslConfigs.DEFAULT_SSL_KEYSTORE_TYPE, ConfigDef.Importance.MEDIUM, SslConfigs.SSL_KEYSTORE_TYPE_DOC)
.define(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG, ConfigDef.Type.STRING, null, ConfigDef.Importance.HIGH, SslConfigs.SSL_KEYSTORE_LOCATION_DOC)
.define(SslConfigs.SSL_KEYSTORE_ALIAS_CONFIG, ConfigDef.Type.STRING, null, ConfigDef.Importance.HIGH, SslConfigs.SSL_KEYSTORE_ALIAS_DOC)
.define(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG, ConfigDef.Type.PASSWORD, null, ConfigDef.Importance.HIGH, SslConfigs.SSL_KEYSTORE_PASSWORD_DOC)
.define(SslConfigs.SSL_KEY_PASSWORD_CONFIG, ConfigDef.Type.PASSWORD, null, ConfigDef.Importance.HIGH, SslConfigs.SSL_KEY_PASSWORD_DOC)
.define(SslConfigs.SSL_KEYSTORE_KEY_CONFIG, ConfigDef.Type.PASSWORD, null, ConfigDef.Importance.HIGH, SslConfigs.SSL_KEYSTORE_KEY_DOC)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,36 @@
import org.apache.kafka.common.security.auth.SslEngineFactory;
import org.apache.kafka.common.utils.SecurityUtils;
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;
Comment on lines 28 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 28 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Comment on lines 30 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Comment on lines 30 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 30 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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;
Comment on lines +53 to +58
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate KeyManager Imports

Duplicate imports for X509ExtendedKeyManager and X509KeyManager classes. Redundant imports reduce code clarity and create maintenance overhead when managing dependencies.

Suggested change
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

Comment on lines +53 to +58
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +53 to +58
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
Expand All @@ -47,6 +73,8 @@
import java.security.cert.CertificateFactory;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.PKCS8EncodedKeySpec;
import java.security.Principal;
import java.security.cert.X509Certificate;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Base64;
Expand Down Expand Up @@ -171,7 +199,7 @@ public void configure(Map<String, ?> configs) {
(Password) configs.get(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG),
(Password) configs.get(SslConfigs.SSL_TRUSTSTORE_CERTIFICATES_CONFIG));

this.sslContext = createSSLContext(keystore, truststore);
this.sslContext = createSSLContext(keystore, truststore, configs);
}

@Override
Expand Down Expand Up @@ -234,7 +262,7 @@ private static SecureRandom createSecureRandom(String key) {
}
}

private SSLContext createSSLContext(SecurityStore keystore, SecurityStore truststore) {
private SSLContext createSSLContext(SecurityStore keystore, SecurityStore truststore, Map<String, ?> configs) {
try {
SSLContext sslContext;
if (provider != null)
Expand All @@ -258,7 +286,7 @@ private SSLContext createSSLContext(SecurityStore keystore, SecurityStore trusts
String tmfAlgorithm = this.tmfAlgorithm != null ? this.tmfAlgorithm : TrustManagerFactory.getDefaultAlgorithm();
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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

log.debug("Created SSL context with keystore {}, truststore {}, provider {}.",
keystore, truststore, sslContext.getProvider().getName());
return sslContext;
Expand All @@ -267,6 +295,63 @@ private SSLContext createSSLContext(SecurityStore keystore, SecurityStore trusts
}
}

private KeyManager[] applyAliasToKM(KeyManager[] kms, final String alias) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if(alias == null || alias.isEmpty()){
return kms;
}
Comment on lines +298 to +301
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +298 to +301
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +298 to +301
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +298 to +301
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +299 to +301
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


log.info("Applying the custom KeyManagers for alias: {}", alias);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


KeyManager[] updatedKMs = new KeyManager[kms.length];

int i=0;
for(KeyManager km : kms){
final X509KeyManager origKM = (X509KeyManager)km;
X509ExtendedKeyManager exKM = new X509ExtendedKeyManager() {
Comment on lines +308 to +310
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

/* (non-Javadoc)
* @see javax.net.ssl.X509ExtendedKeyManager#chooseEngineClientAlias(java.lang.String[], java.security.Principal[], javax.net.ssl.SSLEngine)
*/
Comment on lines +311 to +313
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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;
Comment on lines +315 to +347
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
};
updatedKMs[i++] = exKM;
}
Comment on lines +310 to +351
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return updatedKMs;
}
Comment on lines +298 to +353
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve robustness of the applyAliasToKM method

The applyAliasToKM method has several issues that should be addressed:

  1. It logs at INFO level for every context creation, which could be excessive
  2. It doesn't check if the key manager is of the right type before casting
  3. 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.

Suggested change
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;
}

Comment on lines +298 to +353
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


protected TrustManager[] getTrustManagers(SecurityStore truststore, String tmfAlgorithm) throws NoSuchAlgorithmException, KeyStoreException {
TrustManagerFactory tmf = TrustManagerFactory.getInstance(tmfAlgorithm);
KeyStore ts = truststore == null ? null : truststore.get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import javax.net.ssl.KeyManager;
import java.lang.reflect.Method;
import java.security.KeyStore;
import java.util.Arrays;
import java.util.Collections;
Expand All @@ -33,6 +35,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.mockito.Mockito.mock;

@SuppressWarnings("this-escape")
public class DefaultSslEngineFactoryTest {
Expand Down Expand Up @@ -324,6 +327,8 @@ private String pemFilePath(String pem) throws Exception {
return TestUtils.tempFile(pem).getAbsolutePath();
}



private Password pemAsConfigValue(String... pemValues) {
StringBuilder builder = new StringBuilder();
for (String pem : pemValues) {
Expand All @@ -332,4 +337,27 @@ private Password pemAsConfigValue(String... pemValues) {
}
return new Password(builder.toString().trim());
}

@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);
Comment on lines +358 to +360

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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");

Comment on lines +358 to +360
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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

}
Comment on lines +341 to +361
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
@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));
}


}
Loading