Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

[FEATURE] Add authorization to handle topic metadata request #662

Conversation

Demogorgon314
Copy link
Member

@Demogorgon314 Demogorgon314 commented Aug 18, 2021

#236 Add authorization to handleTopicMetadataRequest.

Fix #415 and #571

Motivation

When client fetch metadata need check topic permission, so we need add authorization in handleTopicMetadataRequest, and do not perform role verification in authentication.

Modifications

Add a common method in KafkaRequestHandler#authorize , this method use authorizer to authorization.
Modify the authentication behavior, and do not verify the role during authentication, verify the role in fetch metadata(#571 )

@Demogorgon314 Demogorgon314 force-pushed the add_authorization_to_handleTopicMetadataRequest branch from 3cdac8e to 9798a9f Compare August 19, 2021 06:26
@Demogorgon314 Demogorgon314 changed the title [WIP][FEATURE] Add authorization to handle topic metadata request [FEATURE] Add authorization to handle topic metadata request Aug 19, 2021
@BewareMyPower BewareMyPower added type/feature Indicates new functionality doc-required This pr needs a document labels Aug 19, 2021
@BewareMyPower
Copy link
Collaborator

My review is completed, PTAL. The main problem is that we should not authorize the CreateTopic operation.

I also add the doc-required label because we may update the docs/security.md later.

@Demogorgon314
Copy link
Member Author

My review is completed, PTAL. The main problem is that we should not authorize the CreateTopic operation.

I also add the doc-required label because we may update the docs/security.md later.

Thanks for review, I will update docs later.

@Demogorgon314 Demogorgon314 force-pushed the add_authorization_to_handleTopicMetadataRequest branch from a933591 to 0a7d5e0 Compare August 21, 2021 02:33
@Demogorgon314 Demogorgon314 merged commit 26dc138 into streamnative:master Aug 21, 2021
wangjialing218 pushed a commit to wangjialing218/kop that referenced this pull request Aug 24, 2021
…ative#662)

Add authorization to handleTopicMetadataRequest(streamnative#236 ).

Fix streamnative#415 and streamnative#571 

## Motivation
When client fetch metadata need check topic permission, so we need add authorization in handleTopicMetadataRequest, and do not perform role verification in authentication.

## Modifications
Add a common method in `KafkaRequestHandler#authorize` , this method use `authorizer` to authorization.
Modify the authentication behavior, and do not verify the role during authentication, verify the role in fetch metadata(streamnative#571  )
BewareMyPower pushed a commit that referenced this pull request Aug 25, 2021
Add authorization to handleTopicMetadataRequest(#236 ).

Fix #415 and #571 

## Motivation
When client fetch metadata need check topic permission, so we need add authorization in handleTopicMetadataRequest, and do not perform role verification in authentication.

## Modifications
Add a common method in `KafkaRequestHandler#authorize` , this method use `authorizer` to authorization.
Modify the authentication behavior, and do not verify the role during authentication, verify the role in fetch metadata(#571  )
@BewareMyPower BewareMyPower mentioned this pull request Aug 27, 2021
9 tasks
@eolivelli
Copy link
Contributor

This implementation is probably wrong.

I am working on the KOP Proxy and this is the test that fails using the Proxy

https://github.com/datastax/kop/blob/29be05f851c8c6b506703b524fb28d3adecea693/test[…]treamnative/pulsar/handlers/kop/KafkaAuthorizationTestBase.java

In the test we grant AuthAction.consume, AuthAction.produce and the the topic is "visible" using listTopics to ANOTHER_USER.

When you use the KOP proxy we use PulsarAdmin to implement listTopics and the user is not able to "see" the topic, because with Pulsar REST API you need tenant admin role to get the partitions for a topic.

I have filed this issue on Pulsar
apache/pulsar#11945

we have to have a consistent behaviour in KOP

@Demogorgon314
Copy link
Member Author

@eolivelli You're right, the behaviour is not consistent. Current pulsar don't support Topic Level permissions to listTopics, IMO, if we have the topics permissions, we should be able to list the topic?

@BewareMyPower BewareMyPower removed the doc-required This pr needs a document label Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/feature Indicates new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Change of behavior between KoP 0.3.0 (2.6.2) and 2.7+ using SASL
3 participants