KAFKA-18813: [1/N] ConsumerGroupHeartbeat API and ConsumerGroupDescribe API must check topic describe #18989

dongnuo123 · 2025-02-21T02:12:17Z

This patch filters out the topic describe unauthorized topics from the ConsumerGroupHeartbeat and ConsumerGroupDescribe response.

In ConsumerGroupHeartbeat,

if the request has subscribedTopicNames set, we directly check the authz in KafkaApis and return a topic auth failure in the response if any of the topics is denied.
Otherwise, we check the authz only if a regex refresh is triggered and we do it based on the acl of the consumer that triggered the refresh. If any of the topic is denied, we filter it out from the resolved subscription.

In ConsumerGroupDescribe, we check the authz of the coordinator response. If any of the topic in the group is denied, we remove the described info and add a topic auth failure to the described group. (similar to the group auth failure)

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

core/src/main/scala/kafka/server/KafkaApis.scala

dajac

@dongnuo123 Thanks for the patch. I left some high level comments.

core/src/main/scala/kafka/coordinator/group/GroupCoordinatorAdapter.scala

core/src/main/scala/kafka/server/KafkaApis.scala

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java

…onsumer-group-heartbeat-and-describe-feb20

...tor-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntime.java

core/src/main/scala/kafka/server/KafkaApis.scala

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java

dajac

@dongnuo123 Thanks for the update. I left some more comments for consideration. It looks pretty good overall. Could you also rebase to include the client side changes made by @lianetm? They should resolve the test failures.

clients/src/main/resources/common/message/ConsumerGroupHeartbeatResponse.json

...common/src/test/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntimeTest.java

core/src/main/scala/kafka/server/KafkaApis.scala

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java

...p-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java

core/src/main/scala/kafka/server/KafkaApis.scala

This reverts commit e80adf4.

…onsumer-group-heartbeat-and-describe-feb20

dajac · 2025-02-22T19:27:18Z

@dongnuo123 Thanks for the update. The patch looks pretty good to me. There are some test failures which are related. Could you please check them with @lianetm?

chia7712

@dongnuo123 thanks for this patch.

chia7712 · 2025-02-23T03:47:38Z

core/src/main/scala/kafka/server/KafkaApis.scala

@@ -2529,9 +2529,24 @@ class KafkaApis(val requestChannel: RequestChannel,
      requestHelper.sendMaybeThrottle(request, consumerGroupHeartbeatRequest.getErrorResponse(Errors.GROUP_AUTHORIZATION_FAILED.exception))
      CompletableFuture.completedFuture[Unit](())
    } else {
+      if (consumerGroupHeartbeatRequest.data.subscribedTopicNames != null &&


Does handleShareGroupHeartbeat have similar issue?

Yes but it will be treated separately. This is a critical blocker for 4.0 for the new consumer protocol whereas it is not for share group.

Got it. open https://issues.apache.org/jira/browse/KAFKA-18851 to log it

I think that there is already one open but I cannot find it from my phone.

There it is: https://issues.apache.org/jira/browse/KAFKA-18817

thanks! will close KAFKA-18851 as duplicate

chia7712 · 2025-02-23T04:05:01Z

core/src/main/scala/kafka/server/KafkaApis.scala

+              }
+            }
+          })
+          val authorizedTopics = authHelper.filterByAuthorized(request.context, DESCRIBE, TOPIC,


It seems handleDescribeGroupsRequest does not require topic authorization. Does it mean users need to update the ACLs when migrating to use AsyncConsumer?

That’s right. The thing is that we cannot apply it on there because the broker is not aware of the topic partitions. It just sees bytes by design.

We could consider parsing the bytes to check the partitions too but as it has been like this since the beginning of the classic protocol. We would need a KIP to discuss it for sure. Personally, I would not change it.

Regarding your second question, the Consumer is not impacted by this as both implementations require topic describe but in different ways. Only the admin client will require extra permissions if not given yet.

@dongnuo123 Could you add a sentence about it in the doc? We could mention this in the ops.html, consumer group section.

chia7712 · 2025-02-23T07:37:42Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java

@@ -3771,6 +3852,7 @@ public CoordinatorResult<ConsumerGroupHeartbeatResponseData, CoordinatorRecord>
        } else {
            // Otherwise, it is a regular heartbeat.
            return consumerGroupHeartbeat(
+                context,


it seems we can remove some arguments from consumerGroupHeartbeat after passing whole context.

chia7712 · 2025-02-23T07:58:20Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java

+
+        List<Action> actions = topicNameCount.entrySet().stream().map(entry -> {
+            ResourcePattern resource = new ResourcePattern(TOPIC, entry.getKey(), LITERAL);
+            return new Action(DESCRIBE, resource, entry.getValue(), true, true);


logIfDenied should be false due to regex pattern, right?

dongnuo123 added 4 commits February 20, 2025 18:33

CGHB

6fe8560

CGHB fix existing UTs

d1bceb4

CGHB with UTs

1fbd09b

describe

01e0675

github-actions bot added triage PRs from the community core Kafka Broker clients labels Feb 21, 2025

dongnuo123 commented Feb 21, 2025

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved

dongnuo123 mentioned this pull request Feb 21, 2025

KAFKA-18813: [1/N] ConsumerGroupHeartbeat API and ConsumerGroupDescribe API must check topic describe #18974

Closed

3 tasks

dongnuo123 added 2 commits February 20, 2025 21:17

spotless check

9d2d286

admin javadoc

6f1dd00

github-actions bot removed the triage PRs from the community label Feb 21, 2025

dajac added KIP-848 The Next Generation of the Consumer Rebalance Protocol Blocker This pull request is identified as solving a blocker for a release. ci-approved labels Feb 21, 2025

dajac reviewed Feb 21, 2025

View reviewed changes

dongnuo123 added 2 commits February 21, 2025 11:24

CGHB

388d11a

CGHB

24f0520

github-actions bot added the KIP-932 Queues for Kafka label Feb 21, 2025

dongnuo123 added 9 commits February 21, 2025 13:58

adapter

4ff3ec6

Merge remote-tracking branch 'ak/trunk' into topic-describe-auth-in-c…

23ddb9e

…onsumer-group-heartbeat-and-describe-feb20

revert adapter change

7c0a042

spotless

808d69d

adapter test

784a13e

spotless check

594e3ab

import control

cd9d2fd

import control

3e11c72

import control

8c6c25c

dajac reviewed Feb 21, 2025

View reviewed changes

...tor-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntime.java Outdated Show resolved Hide resolved

dajac reviewed Feb 21, 2025

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Show resolved Hide resolved

pass authorizer to shard supplier

e31e39b

dajac reviewed Feb 21, 2025

View reviewed changes

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java Outdated Show resolved Hide resolved

dajac reviewed Feb 21, 2025

View reviewed changes

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java Outdated Show resolved Hide resolved

dongnuo123 added 2 commits February 21, 2025 15:41

move filtering topics to executor thread

357a560

use mutable set

d194416

github-actions bot added the performance label Feb 21, 2025

call authorize for each group

e80adf4

dajac reviewed Feb 22, 2025

View reviewed changes

dongnuo123 added 5 commits February 22, 2025 11:00

comments

db8ca9b

Revert "call authorize for each group"

3ad4bc6

This reverts commit e80adf4.

regex updated log

2b0c043

remove withAuthorizer from CoordinatorShardBuilder

b72ede6

Merge remote-tracking branch 'ak/trunk' into topic-describe-auth-in-c…

768e23e

…onsumer-group-heartbeat-and-describe-feb20

chia7712 reviewed Feb 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-18813: [1/N] ConsumerGroupHeartbeat API and ConsumerGroupDescribe API must check topic describe #18989

KAFKA-18813: [1/N] ConsumerGroupHeartbeat API and ConsumerGroupDescribe API must check topic describe #18989

dongnuo123 commented Feb 21, 2025 •

edited

Loading

dajac left a comment

dajac left a comment

dajac commented Feb 22, 2025

chia7712 left a comment

chia7712 Feb 23, 2025

dajac Feb 23, 2025

chia7712 Feb 23, 2025

dajac Feb 23, 2025

dajac Feb 23, 2025

chia7712 Feb 23, 2025

chia7712 Feb 23, 2025

dajac Feb 23, 2025

chia7712 Feb 23, 2025

chia7712 Feb 23, 2025

KAFKA-18813: [1/N] ConsumerGroupHeartbeat API and ConsumerGroupDescribe API must check topic describe #18989

Are you sure you want to change the base?

KAFKA-18813: [1/N] ConsumerGroupHeartbeat API and ConsumerGroupDescribe API must check topic describe #18989

Conversation

dongnuo123 commented Feb 21, 2025 • edited Loading

Committer Checklist (excluded from commit message)

dajac left a comment

Choose a reason for hiding this comment

dajac left a comment

Choose a reason for hiding this comment

dajac commented Feb 22, 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

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

dongnuo123 commented Feb 21, 2025 •

edited

Loading