KAFKA-18627:add allowed modules to JaasUtils #18683

gongxuanzhang · 2025-01-24T01:17:27Z

Fix https://issues.apache.org/jira/browse/KAFKA-18627 and update same test case

gongxuanzhang · 2025-01-25T02:31:22Z

I cannot reproduce this issue locally. I suspect that the test case has added environment variables

gongxuanzhang · 2025-01-25T12:47:35Z

@chia7712 PTAL

chia7712

@gongxuanzhang thanks for this patch

chia7712 · 2025-01-29T18:10:51Z

clients/src/main/java/org/apache/kafka/common/security/JaasUtils.java

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


why to use full package name?

chia7712 · 2025-01-29T18:12:59Z

...est/java/org/apache/kafka/common/security/authenticator/ClientAuthenticationFailureTest.java

@@ -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();


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

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

chia7712 · 2025-01-29T18:13:04Z

...t/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorFailureDelayTest.java

@@ -74,6 +75,7 @@ public void setup() throws Exception {
        credentialCache = new CredentialCache();
        SaslAuthenticatorTest.TestLogin.loginCount.set(0);
        startTimeMs = time.milliseconds();
+        JaasUtils.allowDefaultJaasAndCustomJass();


chia7712 · 2025-01-29T18:13:13Z

core/src/test/scala/integration/kafka/api/SaslSslAdminIntegrationTest.scala

@@ -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)
-


please revert unrelated change

chia7712 · 2025-01-29T18:13:16Z

core/src/test/scala/integration/kafka/api/SaslEndToEndAuthorizationTest.scala

@@ -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)
-


gongxuanzhang · 2025-02-01T03:51:21Z

@chia7712 PTAL

chia7712 · 2025-02-03T04:00:05Z

clients/src/test/java/org/apache/kafka/common/security/JaasContextTest.java

@@ -68,11 +70,13 @@ public void tearDown() throws Exception {

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


Adding this change means we break some compatibility ...

print warning message if users define the org.apache.kafka.disallowed.login.modules

adopt the org.apache.kafka.allowed.login.modules if it is existent

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?

gongxuanzhang · 2025-02-06T14:33:18Z

@chia7712 PTAL

TaiJuWu

LGTM. By the way, could you open a jira for 5.0 to remove DISALLOWED_LOGIN_MODULES_CONFIG

TaiJuWu · 2025-02-06T15:04:50Z

clients/src/main/java/org/apache/kafka/common/security/JaasUtils.java

-    private JaasUtils() {}
+    private JaasUtils() {
+    }


Please revert unrelated this change.

TaiJuWu · 2025-02-06T15:05:03Z

clients/src/main/java/org/apache/kafka/common/security/JaasUtils.java

@@ -16,13 +16,17 @@
 */
 package org.apache.kafka.common.security;

+


Please revet this too.

TaiJuWu · 2025-02-06T15:36:08Z

clients/src/main/java/org/apache/kafka/common/security/JaasContext.java

+        }
+        String loginModuleName = appConfigurationEntry.getLoginModuleName().trim();
+        String allowedProperty = System.getProperty(ALLOWED_LOGIN_MODULES_CONFIG);
+        if (allowedProperty != null) {


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

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

make sense, thanks!

chia7712

@gongxuanzhang thanks for this patch. overall LGTM, and could you please add this change to upgrade.html?

Also ping @showuon @mumrah @mimaison - after merging this PR, we can file a ticket for 5.0 to remove disallowed list

gongxuanzhang · 2025-02-07T04:25:51Z

@chia7712 PTAL

modify disallow to allow

e2967a8

github-actions bot added triage PRs from the community clients small Small PRs labels Jan 24, 2025

FrankYang0529 added the ci-approved label Jan 24, 2025

fix test case and test ci

2b0c20d

github-actions bot added core Kafka Broker and removed small Small PRs labels Jan 24, 2025

fix jaas test case

d151ba5

chia7712 reviewed Jan 29, 2025

View reviewed changes

github-actions bot removed the triage PRs from the community label Jan 30, 2025

fix

9393507

chia7712 reviewed Feb 3, 2025

View reviewed changes

revert

7490422

github-actions bot added the small Small PRs label Feb 6, 2025

gongxuanzhang added 3 commits February 6, 2025 15:30

remove set properties

63628e0

format

a57b15e

fix

9608e0a

TaiJuWu reviewed Feb 6, 2025

View reviewed changes

TaiJuWu approved these changes Feb 6, 2025

View reviewed changes

TaiJuWu reviewed Feb 6, 2025

View reviewed changes

chia7712 reviewed Feb 6, 2025

View reviewed changes

gongxuanzhang added 3 commits February 7, 2025 09:48

add upgrade

f0bdcd2

Merge remote-tracking branch 'origin/update_jaas' into update_jaas

f68f3e9

add upgrade

272c638

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-18627:add allowed modules to JaasUtils #18683

KAFKA-18627:add allowed modules to JaasUtils #18683

gongxuanzhang commented Jan 24, 2025

gongxuanzhang commented Jan 25, 2025

gongxuanzhang commented Jan 25, 2025

chia7712 left a comment

chia7712 Jan 29, 2025

chia7712 Jan 29, 2025

gongxuanzhang Jan 30, 2025

chia7712 Jan 29, 2025

chia7712 Jan 29, 2025

chia7712 Jan 29, 2025

gongxuanzhang commented Feb 1, 2025

chia7712 Feb 3, 2025

gongxuanzhang commented Feb 6, 2025

TaiJuWu left a comment

TaiJuWu Feb 6, 2025

TaiJuWu Feb 6, 2025

TaiJuWu Feb 6, 2025 •

edited

Loading

gongxuanzhang Feb 6, 2025

TaiJuWu Feb 6, 2025

chia7712 left a comment

gongxuanzhang commented Feb 7, 2025

		@@ -16,13 +16,17 @@
		*/
		package org.apache.kafka.common.security;

KAFKA-18627:add allowed modules to JaasUtils #18683

Are you sure you want to change the base?

KAFKA-18627:add allowed modules to JaasUtils #18683

Conversation

gongxuanzhang commented Jan 24, 2025

gongxuanzhang commented Jan 25, 2025

gongxuanzhang commented Jan 25, 2025

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongxuanzhang commented Feb 1, 2025

Choose a reason for hiding this comment

gongxuanzhang commented Feb 6, 2025

TaiJuWu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TaiJuWu Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

gongxuanzhang commented Feb 7, 2025

TaiJuWu Feb 6, 2025 •

edited

Loading