KAFKA-12394: Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission #10223

hachikuji · 2021-02-26T22:52:16Z

We now accept topicIds in the DeleteTopic request. If the client principal does not have Describe permission, then we return TOPIC_AUTHORIZATION_FAILED. This is justified because the topicId is not considered sensitive. However, in this case, we should not return the name of the topic in the response since it is sensitive.

Committer Checklist (excluded from commit message)

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

…ibe permission

jolshan

Thanks for catching this. LGTM

dajac

@hachikuji Thanks for the PR. That makes sense. I have left a meta question. Besides this, the PR looks good.

dajac · 2021-02-27T08:56:48Z

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

+        } else if (topicIdsFromRequest.contains(topic.topicId) && !authorizedDescribeTopics(topic.name)) {
+          // Because the client does not have Describe permission, the name should
+          // not be returned in the response.
+          topic.setName(null)


I just noticed that the Name in the schema [1] used mapKey: true. I wonder if we should remove it now that Name is nullable. Usually, when we use mapKey, we expect to be able to query with the name and this is not always the case now. It seems that we don't rely on it in the admin client but we do in tests. What do you think?

[1] https://github.com/apache/kafka/blob/trunk/clients/src/main/resources/common/message/DeleteTopicsResponse.json#L41

That's a good question. I agree using mapKey makes less sense now. Let me try it out. If the diff is not too bad, we can do it here. Otherwise, we can do it separately.

We have one dependence here which makes this a little more work than I wanted to do here: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaApis.scala#L1909. I filed a separate JIRA so that we don't forget about it: https://issues.apache.org/jira/browse/KAFKA-12395.

dengziming

LGTM, Just a minor comment.

dengziming · 2021-02-27T10:13:36Z

core/src/test/scala/unit/kafka/server/KafkaApisTest.scala

+    assertEquals(Some(Errors.UNKNOWN_TOPIC_OR_PARTITION), lookupErrorCode("baz"))
+  }
+
+  def expectTopicAuthorization(


Could we simplify this method using AuthHelperTest.matchSameElements().

I looked at doing this, but the need to preserve order in the result makes it just as cumbersome. The capture also provides some type safety and avoids the need for casting from the argument list.

chia7712

@hachikuji Thanks for your patch. a couple of comments are left. BTW, it would be better that error code can be consistent to #10184.

chia7712 · 2021-02-27T13:39:56Z

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

+          // Because the client does not have Describe permission, the name should
+          // not be returned in the response.
+          topic.setName(null)
+          topic.setErrorCode(Errors.UNKNOWN_TOPIC_ID.code)


I prepared to discuss this error code on #10184 (comment) :)

return UNKNOWN_TOPIC_ID

Could you add clear error message to this response (call setErrorMessage)? I can imagine users get confusing for the error code UNKNOWN_TOPIC_ID because it presents two scenarios now (the id does not exist or you have no permission to describe topic).

return TOPIC_AUTHORIZATION_FAILED

It indicates accurate error and it can help user handle un-authorized requests. Personally, this error is more suitable :)

I think the issue with TOPIC_AUTHORIZATION_FAILED is that we are returning a different error message than the case where the topic ID does not exists and we are implying the existence of a topic when we should not be.

I made the comment before fully realizing how topic IDs were complicating the matter. I filed this JIRA to discuss further: https://issues.apache.org/jira/browse/KAFKA-12394. I'd suggest that we keep the current behavior in this PR and fix the small issue. It would be good to have the tests in any case.

@chia7712 @jolshan I ended up doing this here after all since it sounds like there is consensus on not treating the topicId as sensitive based on the JIRA discussion. Let me know if you have any concerns.

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

chia7712 · 2021-02-27T14:31:20Z

Not sure whether it is possible to have a helper method to process this complicated code for both KafkaApis and ControllApis.

jolshan · 2021-02-27T17:25:57Z

It also seems like this part of KafkaApis is completely rewritten in #10184. So maybe any changes to KafkaApis should be addressed in that PR?

hachikuji · 2021-03-01T20:13:43Z

@jolshan @chia7712 I don't think KafkaApis has been rewritten yet. My suggestion is probably get this checked in as is so that we can definitely get it into 2.8. Then we should consider how to consolidate the handling logic in ControllerApis and KafkaApis after #10184 is merged.

jolshan · 2021-03-01T20:30:06Z

@hachikuji Ah sorry. I was misreading the file name. Makes sense.

chia7712

@hachikuji thanks for consolidating the exception handle. one small question is left.

chia7712 · 2021-03-02T07:33:28Z

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

+          // this case with `UNKNOWN_TOPIC_ID`.
+          topic.setName(null)
+          topic.setErrorCode(Errors.TOPIC_AUTHORIZATION_FAILED.code)
+        } else if (!authorizedDeleteTopics.contains(topic.name)) {


according to #10184 (comment), should it handle the case name provided, topic missing, describable => UNKNOWN_TOPIC_OR_PARTITION?

I guess there are really two sub-cases. Here's how they are currently handled:

name provided, topic missing, describable, deletable => UNKNOWN_TOPIC_OR_PARTITION name provided, topic missing, describable, undeletable => TOPIC_AUTHORIZATION_FAILED

This seems defensible to me. The UNKNOWN_TOPIC_OR_PARTITION error will cause the client to retry because of the possibility of stale metadata, but it can't delete the topic anyway because of the authorization failure. It seems better to fail fast?

This seems defensible to me. The UNKNOWN_TOPIC_OR_PARTITION error will cause the client to retry because of the possibility of stale metadata, but it can't delete the topic anyway because of the authorization failure. It seems better to fail fast?

That makes sense to me. We have to make sure this rule is applied to #10184 :)

chia7712

+1 to this nice patch. one code style comment is left.

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

chia7712

+1 to latest commit :)

…onse if no describe permission (#10223) We now accept topicIds in the `DeleteTopic` request. If the client principal does not have `Describe` permission, then we return `TOPIC_AUTHORIZATION_FAILED`. This is justified because the topicId is not considered sensitive. However, in this case, we should not return the name of the topic in the response since we do consider it sensitive. Reviewers: David Jacot <[email protected]>, dengziming <[email protected]>, Justine Olshan <[email protected]>, Chia-Ping Tsai <[email protected]>

* apache-github/trunk: (37 commits) KAFKA-10357: Extract setup of changelog from Streams partition assignor (apache#10163) KAFKA-10251: increase timeout for consuming records (apache#10228) KAFKA-12394; Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission (apache#10223) MINOR: Disable transactional/idempotent system tests for Raft quorums (apache#10224) KAFKA-10766: Unit test cases for RocksDBRangeIterator (apache#9717) KAFKA-12289: Adding test cases for prefix scan in InMemoryKeyValueStore (apache#10052) KAFKA-12268: Implement task idling semantics via currentLag API (apache#10137) MINOR: Time and log producer state recovery phases (apache#10241) MINOR: correct the error message of validating uint32 (apache#10193) MINOR: Format the revoking active log output in `StreamsPartitionAssignor` (apache#10242) KAFKA-12323 Follow-up: Refactor the unit test a bit (apache#10205) MINOR: Remove stack trace of the lock exception in a debug log4j (apache#10231) MINOR: Word count should account for extra whitespaces between words (apache#10229) MINOR; Small refactor in `GroupMetadata` (apache#10236) KAFKA-10340: Proactively close producer when cancelling source tasks (apache#10016) KAFKA-12329; kafka-reassign-partitions command should give a better error message when a topic does not exist (apache#10141) KAFKA-12254: Ensure MM2 creates topics with source topic configs (apache#10217) MINOR: fix kafka-metadata-shell.sh (apache#10226) KAFKA-12374: Add missing config sasl.mechanism.controller.protocol (apache#10199) KAFKA-10101: Fix edge cases in Log.recoverLog and LogManager.loadLogs (apache#8812) ...

MINOR: Do not expose topic name in DeleteTopic response if no descr…

ef340d3

…ibe permission

jolshan approved these changes Feb 26, 2021

View reviewed changes

dajac reviewed Feb 27, 2021

View reviewed changes

dengziming approved these changes Feb 27, 2021

View reviewed changes

chia7712 reviewed Feb 27, 2021

View reviewed changes

Use TOPIC_AUTHORIZATION_FAILED when no describe permission

76589c1

hachikuji changed the title ~~MINOR: Do not expose topic name in DeleteTopic response if no describe permission~~ KAFKA-12394: Return TOPIC_AUTHORIZATION_FAILED in delete topic response if no describe permission Mar 2, 2021

chia7712 reviewed Mar 2, 2021

View reviewed changes

chia7712 approved these changes Mar 2, 2021

View reviewed changes

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

Prefer explicit contains call

7dffa6d

chia7712 approved these changes Mar 2, 2021

View reviewed changes

hachikuji merged commit a4ba732 into apache:trunk Mar 2, 2021

chia7712 mentioned this pull request Mar 3, 2021

MINOR: enable topic deletion in the KIP-500 controller #10184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-12394: Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission #10223

KAFKA-12394: Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission #10223

hachikuji commented Feb 26, 2021 •

edited

Loading

jolshan left a comment

dajac left a comment

dajac Feb 27, 2021

hachikuji Feb 27, 2021

hachikuji Mar 1, 2021

dengziming left a comment

dengziming Feb 27, 2021

hachikuji Mar 1, 2021 •

edited

Loading

chia7712 left a comment

chia7712 Feb 27, 2021

jolshan Feb 27, 2021

hachikuji Mar 1, 2021

hachikuji Mar 2, 2021

chia7712 commented Feb 27, 2021

jolshan commented Feb 27, 2021 •

edited

Loading

hachikuji commented Mar 1, 2021 •

edited

Loading

jolshan commented Mar 1, 2021

chia7712 left a comment

chia7712 Mar 2, 2021

hachikuji Mar 2, 2021

chia7712 Mar 2, 2021

chia7712 left a comment

chia7712 left a comment

KAFKA-12394: Return TOPIC_AUTHORIZATION_FAILED in delete topic response if no describe permission #10223

KAFKA-12394: Return TOPIC_AUTHORIZATION_FAILED in delete topic response if no describe permission #10223

Conversation

hachikuji commented Feb 26, 2021 • edited Loading

Committer Checklist (excluded from commit message)

jolshan left a comment

Choose a reason for hiding this comment

dajac 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

dengziming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hachikuji Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

return UNKNOWN_TOPIC_ID

return TOPIC_AUTHORIZATION_FAILED

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 commented Feb 27, 2021

jolshan commented Feb 27, 2021 • edited Loading

hachikuji commented Mar 1, 2021 • edited Loading

jolshan commented Mar 1, 2021

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

chia7712 left a comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

KAFKA-12394: Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission #10223

KAFKA-12394: Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission #10223

hachikuji commented Feb 26, 2021 •

edited

Loading

hachikuji Mar 1, 2021 •

edited

Loading

return `UNKNOWN_TOPIC_ID`

return `TOPIC_AUTHORIZATION_FAILED`

jolshan commented Feb 27, 2021 •

edited

Loading

hachikuji commented Mar 1, 2021 •

edited

Loading