Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-18627:add allowed modules to JaasUtils #18683

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from
+56 −6
Open

KAFKA-18627:add allowed modules to JaasUtils #18683

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e2967a8
modify disallow to allow
gongxuanzhang Jan 22, 2025
2b0c20d
fix test case and test ci
gongxuanzhang Jan 24, 2025
d151ba5
fix jaas test case
gongxuanzhang Jan 24, 2025
9393507
fix
gongxuanzhang Jan 31, 2025
7490422
revert
gongxuanzhang Feb 6, 2025
63628e0
remove set properties
gongxuanzhang Feb 6, 2025
a57b15e
format
gongxuanzhang Feb 6, 2025
9608e0a
fix
gongxuanzhang Feb 6, 2025
f0bdcd2
add upgrade
gongxuanzhang Feb 7, 2025
f68f3e9
Merge remote-tracking branch 'origin/update_jaas' into update_jaas
gongxuanzhang Feb 7, 2025
272c638
add upgrade
gongxuanzhang Feb 7, 2025
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
39 changes: 33 additions & 6 deletions clients/src/main/java/org/apache/kafka/common/security/JaasContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@
import javax.security.auth.login.AppConfigurationEntry;
import javax.security.auth.login.Configuration;

import static org.apache.kafka.common.security.JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG;
import static org.apache.kafka.common.security.JaasUtils.ALLOWED_LOGIN_MODULES_DEFAULT;
import static org.apache.kafka.common.security.JaasUtils.DISALLOWED_LOGIN_MODULES_CONFIG;
import static org.apache.kafka.common.security.JaasUtils.DISALLOWED_LOGIN_MODULES_DEFAULT;

public class JaasContext {

Expand Down Expand Up @@ -103,15 +104,41 @@ else if (contextModules.length != 1)
return defaultContext(contextType, listenerContextName, globalContextName);
}

@SuppressWarnings("deprecation")
private static void throwIfLoginModuleIsNotAllowed(AppConfigurationEntry appConfigurationEntry) {
Set<String> disallowedLoginModuleList = Arrays.stream(
System.getProperty(DISALLOWED_LOGIN_MODULES_CONFIG, DISALLOWED_LOGIN_MODULES_DEFAULT).split(","))
String disallowedProperty = System.getProperty(DISALLOWED_LOGIN_MODULES_CONFIG);
if (disallowedProperty != null) {
LOG.warn("System property '{}' is deprecated and will be removed in a future release. Use '{}' instead.",
DISALLOWED_LOGIN_MODULES_CONFIG, ALLOWED_LOGIN_MODULES_CONFIG);
}
String loginModuleName = appConfigurationEntry.getLoginModuleName().trim();
String allowedProperty = System.getProperty(ALLOWED_LOGIN_MODULES_CONFIG);
if (allowedProperty != null) {
Copy link
Contributor

@TaiJuWu TaiJuWu Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should we need to consider a case if a module in disallow and allow at same time?
Maybe it will happen in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new logic is to prioritize "allow", right?

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense, thanks!

Set<String> allowedLoginModuleList = Arrays.stream(allowedProperty.split(","))
.map(String::trim)
.collect(Collectors.toSet());
if (!allowedLoginModuleList.contains(loginModuleName)) {
throw new IllegalArgumentException(loginModuleName + " is not allowed. Update System property '"
+ ALLOWED_LOGIN_MODULES_CONFIG + "' to allow " + loginModuleName);
}
return;
}
if (disallowedProperty != null) {
Set<String> disallowedLoginModuleList = Arrays.stream(disallowedProperty.split(","))
.map(String::trim)
.collect(Collectors.toSet());
if (disallowedLoginModuleList.contains(loginModuleName)) {
throw new IllegalArgumentException(loginModuleName + " is not allowed. Update System property '"
+ DISALLOWED_LOGIN_MODULES_CONFIG + "' to allow " + loginModuleName);
}
return;
}
Set<String> defaultAllowedLoginModuleList = Arrays.stream(ALLOWED_LOGIN_MODULES_DEFAULT.split(","))
.map(String::trim)
.collect(Collectors.toSet());
String loginModuleName = appConfigurationEntry.getLoginModuleName().trim();
if (disallowedLoginModuleList.contains(loginModuleName)) {
if (!defaultAllowedLoginModuleList.contains(loginModuleName)) {
throw new IllegalArgumentException(loginModuleName + " is not allowed. Update System property '"
+ DISALLOWED_LOGIN_MODULES_CONFIG + "' to allow " + loginModuleName);
+ ALLOWED_LOGIN_MODULES_CONFIG + "' to allow " + loginModuleName);
}
}

Expand Down
25 changes: 22 additions & 3 deletions clients/src/main/java/org/apache/kafka/common/security/JaasUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,32 @@
*/
package org.apache.kafka.common.security;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revet this too.

public final class JaasUtils {
public static final String JAVA_LOGIN_CONFIG_PARAM = "java.security.auth.login.config";
@Deprecated
public static final String DISALLOWED_LOGIN_MODULES_CONFIG = "org.apache.kafka.disallowed.login.modules";
public static final String DISALLOWED_LOGIN_MODULES_DEFAULT =
"com.sun.security.auth.module.JndiLoginModule,com.sun.security.auth.module.LdapLoginModule";
public static final String ALLOWED_LOGIN_MODULES_CONFIG = "org.apache.kafka.allowed.login.modules";
public static final String ALLOWED_LOGIN_MODULES_DEFAULT = String.join(",", List.of(
"org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule",
"org.apache.kafka.common.security.plain.PlainLoginModule",
"org.apache.kafka.connect.rest.basic.auth.extension.PropertyFileLoginModule",
"org.apache.kafka.common.security.scram.ScramLoginModule",
"com.sun.security.auth.module.Krb5LoginModule"
));
public static final String SERVICE_NAME = "serviceName";

private JaasUtils() {}
private JaasUtils() {
}
Comment on lines -26 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert unrelated this change.


public static void allowDefaultJaasAndCustomJass(String... customJaas) {
List<String> jaasModules = new ArrayList<>();
jaasModules.add(ALLOWED_LOGIN_MODULES_DEFAULT);
jaasModules.addAll(Arrays.asList(customJaas));
System.setProperty(org.apache.kafka.common.security.JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG, String.join(",", jaasModules));
Copy link
Member

Choose a reason for hiding this comment

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

why to use full package name?

}

}
39 changes: 37 additions & 2 deletions clients/src/test/java/org/apache/kafka/common/security/JaasContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@
import javax.security.auth.login.AppConfigurationEntry.LoginModuleControlFlag;
import javax.security.auth.login.Configuration;

import static org.apache.kafka.common.security.JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG;
import static org.apache.kafka.common.security.JaasUtils.DISALLOWED_LOGIN_MODULES_CONFIG;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
Expand Down Expand Up @@ -68,11 +70,13 @@ public void tearDown() throws Exception {

@Test
public void testConfigNoOptions() throws Exception {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.testConfigNoOptions");
Copy link
Member

Choose a reason for hiding this comment

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

Adding this change means we break some compatibility ...

  1. print warning message if users define the org.apache.kafka.disallowed.login.modules
  2. adopt the org.apache.kafka.allowed.login.modules if it is existent
  3. evaluate the modules according to DISALLOWED_LOGIN_MODULES_DEFAULT

Additionally, we should open a jira for 5.0 to add ALLOWED_LOGIN_MODULES_DEFAULT

WDYT?

checkConfiguration("test.testConfigNoOptions", LoginModuleControlFlag.REQUIRED, new HashMap<>());
}

@Test
public void testControlFlag() throws Exception {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.testControlFlag");
LoginModuleControlFlag[] controlFlags = new LoginModuleControlFlag[] {
LoginModuleControlFlag.REQUIRED,
LoginModuleControlFlag.REQUISITE,
Expand All @@ -88,13 +92,15 @@ public void testControlFlag() throws Exception {

@Test
public void testSingleOption() throws Exception {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.testSingleOption");
Map<String, Object> options = new HashMap<>();
options.put("propName", "propValue");
checkConfiguration("test.testSingleOption", LoginModuleControlFlag.REQUISITE, options);
}

@Test
public void testMultipleOptions() throws Exception {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.testMultipleOptions");
Map<String, Object> options = new HashMap<>();
for (int i = 0; i < 10; i++)
options.put("propName" + i, "propValue" + i);
Expand All @@ -103,6 +109,7 @@ public void testMultipleOptions() throws Exception {

@Test
public void testQuotedOptionValue() throws Exception {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.testQuotedOptionValue");
Map<String, Object> options = new HashMap<>();
options.put("propName", "prop value");
options.put("propName2", "value1 = 1, value2 = 2");
Expand All @@ -112,6 +119,7 @@ public void testQuotedOptionValue() throws Exception {

@Test
public void testQuotedOptionName() throws Exception {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.testQuotedOptionName");
Map<String, Object> options = new HashMap<>();
options.put("prop name", "propValue");
String config = "test.testQuotedOptionName required \"prop name\"=propValue;";
Expand Down Expand Up @@ -224,8 +232,8 @@ public void testDisallowedLoginModulesSystemProperty() throws Exception {
"SOME-MECHANISM", Collections.emptyMap()));


//Remove default value for org.apache.kafka.disallowed.login.modules
System.setProperty(DISALLOWED_LOGIN_MODULES_CONFIG, "");
// add allowed login modules
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "com.sun.security.auth.module.JndiLoginModule, com.sun.security.auth.module.LdapLoginModule");

checkConfiguration("com.sun.security.auth.module.JndiLoginModule", LoginModuleControlFlag.REQUIRED, new HashMap<>());
checkConfiguration("com.sun.security.auth.module.LdapLoginModule", LoginModuleControlFlag.REQUIRED, new HashMap<>());
Expand All @@ -252,9 +260,34 @@ public void testDisallowedLoginModulesSystemProperty() throws Exception {
checkEntry(context.configurationEntries().get(0), "com.sun.security.auth.module.LdapLoginModule",
LoginModuleControlFlag.REQUISITE, Collections.emptyMap());
}

@Test
void testAllowedLoginModulesSystemProperty() {

// default
String jaasConfigProp1 = "com.ibm.security.auth.module.LdapLoginModule required;";
assertThrows(IllegalArgumentException.class, () -> configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1));

String jaasConfigProp2 = "com.sun.security.auth.module.JndiLoginModule required;";
// set allow dont' set not allow
System.setProperty(JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG, "com.ibm.security.auth.module.LdapLoginModule");
assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1));
assertThrows(IllegalArgumentException.class, () -> configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp2));

// set allow and set not allow
System.setProperty(JaasUtils.DISALLOWED_LOGIN_MODULES_CONFIG, "com.ibm.security.auth.module.LdapLoginModule");
assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1));
assertThrows(IllegalArgumentException.class, () -> configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp2));

// don't set allow and set not allow
System.clearProperty(JaasUtils.ALLOWED_LOGIN_MODULES_CONFIG);
assertThrows(IllegalArgumentException.class, () -> configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp1));
assertDoesNotThrow(() -> configurationEntry(JaasContext.Type.CLIENT, jaasConfigProp2));
}

@Test
public void testNumericOptionWithQuotes() throws Exception {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.testNumericOptionWithQuotes");
Map<String, Object> options = new HashMap<>();
options.put("option1", "3");
String config = "test.testNumericOptionWithQuotes required option1=\"3\";";
Expand All @@ -263,6 +296,7 @@ public void testNumericOptionWithQuotes() throws Exception {

@Test
public void testLoadForServerWithListenerNameOverride() throws IOException {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.LoginModuleDefault, test.LoginModuleOverride");
writeConfiguration(Arrays.asList(
"KafkaServer { test.LoginModuleDefault required; };",
"plaintext.KafkaServer { test.LoginModuleOverride requisite; };"
Expand All @@ -278,6 +312,7 @@ public void testLoadForServerWithListenerNameOverride() throws IOException {

@Test
public void testLoadForServerWithListenerNameAndFallback() throws IOException {
System.setProperty(ALLOWED_LOGIN_MODULES_CONFIG, "test.LoginModule");
writeConfiguration(Arrays.asList(
"KafkaServer { test.LoginModule required; };",
"other.KafkaServer { test.LoginModuleOther requisite; };"
Expand Down
2 changes: 2 additions & 0 deletions .../java/org/apache/kafka/common/security/authenticator/ClientAuthenticationFailureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.kafka.common.network.ListenerName;
import org.apache.kafka.common.network.NetworkTestUtils;
import org.apache.kafka.common.network.NioEchoServer;
import org.apache.kafka.common.security.JaasUtils;
import org.apache.kafka.common.security.TestSecurityConfig;
import org.apache.kafka.common.security.auth.SecurityProtocol;
import org.apache.kafka.common.serialization.StringDeserializer;
Expand Down Expand Up @@ -75,6 +76,7 @@ public void setup() throws Exception {
TestJaasConfig testJaasConfig = TestJaasConfig.createConfiguration("PLAIN", Collections.singletonList("PLAIN"));
testJaasConfig.setClientOptions("PLAIN", TestJaasConfig.USERNAME, "anotherpassword");
server = createEchoServer(securityProtocol);
JaasUtils.allowDefaultJaasAndCustomJass();
Copy link
Member

Choose a reason for hiding this comment

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

it does not add any other modules, so do we really need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case reports an error. It may be that a change to the default configuration caused the jaas tested to be rejected.
So I add this line

}

@AfterEach
Expand Down
2 changes: 2 additions & 0 deletions clients/src/test/java/org/apache/kafka/common/security/authenticator/LoginManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.apache.kafka.common.config.types.Password;
import org.apache.kafka.common.network.ListenerName;
import org.apache.kafka.common.security.JaasContext;
import org.apache.kafka.common.security.JaasUtils;
import org.apache.kafka.common.security.auth.AuthenticateCallbackHandler;
import org.apache.kafka.common.security.auth.Login;
import org.apache.kafka.common.security.plain.PlainLoginModule;
Expand Down Expand Up @@ -59,6 +60,7 @@ public void setUp() {
" required user=\"digestuser\" password=\"digest-secret\";");
TestJaasConfig.createConfiguration("SCRAM-SHA-256",
Collections.singletonList("SCRAM-SHA-256"));
JaasUtils.allowDefaultJaasAndCustomJass("org.apache.kafka.common.security.authenticator.TestDigestLoginModule");
}

@AfterEach
Expand Down
2 changes: 2 additions & 0 deletions ...ava/org/apache/kafka/common/security/authenticator/SaslAuthenticatorFailureDelayTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.kafka.common.network.NioEchoServer;
import org.apache.kafka.common.network.Selector;
import org.apache.kafka.common.security.JaasContext;
import org.apache.kafka.common.security.JaasUtils;
import org.apache.kafka.common.security.TestSecurityConfig;
import org.apache.kafka.common.security.auth.SecurityProtocol;
import org.apache.kafka.common.utils.LogContext;
Expand Down Expand Up @@ -74,6 +75,7 @@ public void setup() throws Exception {
credentialCache = new CredentialCache();
SaslAuthenticatorTest.TestLogin.loginCount.set(0);
startTimeMs = time.milliseconds();
JaasUtils.allowDefaultJaasAndCustomJass();
Copy link
Member

Choose a reason for hiding this comment

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

ditto

}

@AfterEach
Expand Down
10 changes: 4 additions & 6 deletions ...s/src/test/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import org.apache.kafka.common.requests.SaslHandshakeRequest;
import org.apache.kafka.common.requests.SaslHandshakeResponse;
import org.apache.kafka.common.security.JaasContext;
import org.apache.kafka.common.security.JaasUtils;
import org.apache.kafka.common.security.TestSecurityConfig;
import org.apache.kafka.common.security.auth.AuthenticateCallbackHandler;
import org.apache.kafka.common.security.auth.AuthenticationContext;
Expand Down Expand Up @@ -174,6 +175,8 @@ public void setup() throws Exception {
saslClientConfigs = clientCertStores.getTrustingConfig(serverCertStores);
credentialCache = new CredentialCache();
TestLogin.loginCount.set(0);
JaasUtils.allowDefaultJaasAndCustomJass("org.apache.kafka.common.security.authenticator.TestDigestLoginModule",
"org.apache.kafka.common.security.authenticator.SaslAuthenticatorTest$TestPlainLoginModule");
}

@AfterEach
Expand Down Expand Up @@ -1053,12 +1056,7 @@ public void testInvalidLoginModule() throws Exception {

SecurityProtocol securityProtocol = SecurityProtocol.SASL_SSL;
server = createEchoServer(securityProtocol);
try {
createSelector(securityProtocol, saslClientConfigs);
fail("SASL/PLAIN channel created without valid login module");
} catch (KafkaException e) {
// Expected exception
}
assertThrows(IllegalArgumentException.class, () -> createSelector(securityProtocol, saslClientConfigs));
}

/**
Expand Down
1 change: 0 additions & 1 deletion core/src/test/scala/integration/kafka/api/SaslEndToEndAuthorizationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ abstract class SaslEndToEndAuthorizationTest extends EndToEndAuthorizationTest {
producerConfig.put(SaslConfigs.SASL_JAAS_CONFIG, clientLoginContext)
consumerConfig.put(SaslConfigs.SASL_JAAS_CONFIG, clientLoginContext)
adminClientConfig.put(SaslConfigs.SASL_JAAS_CONFIG, clientLoginContext)

Copy link
Member

Choose a reason for hiding this comment

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

ditto

val superuserLoginContext = jaasAdminLoginModule(kafkaClientSaslMechanism)
superuserClientConfig.put(SaslConfigs.SASL_JAAS_CONFIG, superuserLoginContext)
super.setUp(testInfo)
Expand Down
1 change: 0 additions & 1 deletion core/src/test/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ class SaslSslAdminIntegrationTest extends BaseAdminIntegrationTest with SaslSetu
this.serverConfig.setProperty(DelegationTokenManagerConfigs.DELEGATION_TOKEN_SECRET_KEY_CONFIG, secretKey)
this.serverConfig.setProperty(DelegationTokenManagerConfigs.DELEGATION_TOKEN_EXPIRY_TIME_MS_CONFIG, Long.MaxValue.toString)
this.serverConfig.setProperty(DelegationTokenManagerConfigs.DELEGATION_TOKEN_MAX_LIFETIME_CONFIG, Long.MaxValue.toString)

Copy link
Member

Choose a reason for hiding this comment

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

please revert unrelated change

setUpSasl()
super.setUp(testInfo)
setInitialAcls()
Expand Down
Loading