[improve] Pass group ID to authorizer when using OAuth #1926

Demogorgon314 · 2023-06-28T04:37:54Z

Motivation

Currently, the KoP only supports checking the topic permission when doing the consume authorization, but Kafka support checking the topic and group ID permission.

Before introducing this change, let's understand why KoP can't check group ID permission.

When Kafka does the consume authorization check, it will first check the group ID permission in handleJoinGroupRequest method, then it will check the topic permission in the handleFetchRequest method.

However, Pulsar is using another way to check consume permission. See the org.apache.pulsar.broker.authorization.AuthorizationService#canConsumeAsync method, it requires passing the topic name and subscription to check both permissions in the same place, and the topic name is required param.

public CompletableFuture<Boolean> canConsumeAsync(TopicName topicName, String role, AuthenticationDataSource authenticationData,String subscription)

If we follow the Kafka way to check the consumer permission, we can't get the topic name when joining a group since the join group request does not contain the topic name. So we have to authorize permission in the fetch request.

However, to authorization consume permission in the fetch request, we can only get the topic name in this request. In this case, we can't authorize the group ID.

Modifications

Since we can't get group ID when handling fetch requests, we need to find a way to pass through group ID when doing the authentication.

In OAuth, we have a credentials_file.json file that needs to be config, for example:

{
	"client_id": "my-id",
	"client_secret": "my-secret",
	"tenant": "my-tenant"
}

Here we can add a new parameter into the config:

{
	"client_id": "my-id",
	"client_secret": "my-secret",
	"tenant": "my-tenant",
        "group_id": "my-group-id"
}

Then we can add these parameters to SaslExtensions, and send it to the broker.

Documentation

Check the box below.

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

codecov · 2023-06-28T05:30:05Z

Codecov Report

Merging #1926 (a0566e8) into master (24180bb) will decrease coverage by 0.10%.
The diff coverage is 9.37%.

@@             Coverage Diff              @@
##             master    #1926      +/-   ##
============================================
- Coverage     17.94%   17.85%   -0.10%     
- Complexity      745      752       +7     
============================================
  Files           194      195       +1     
  Lines         13957    14061     +104     
  Branches       1291     1308      +17     
============================================
+ Hits           2505     2511       +6     
- Misses        11271    11368      +97     
- Partials        181      182       +1

Impacted Files	Coverage Δ
...ative/pulsar/handlers/kop/KafkaRequestHandler.java	`1.06% <0.00%> (-0.02%)`	⬇️
...ive/pulsar/handlers/kop/SchemaRegistryManager.java	`0.00% <0.00%> (ø)`
...e/pulsar/handlers/kop/security/KafkaPrincipal.java	`0.00% <0.00%> (ø)`
.../pulsar/handlers/kop/security/PlainSaslServer.java	`45.00% <0.00%> (-1.56%)`	⬇️
...ulsar/handlers/kop/security/SaslAuthenticator.java	`0.00% <0.00%> (ø)`
...ulsar/handlers/kop/security/auth/ResourceType.java	`0.00% <0.00%> (ø)`
...andlers/kop/security/auth/SimpleAclAuthorizer.java	`0.00% <0.00%> (ø)`
...s/kop/security/oauth/KopOAuthBearerSaslServer.java	`0.00% <0.00%> (ø)`
.../security/oauth/OauthValidatorCallbackHandler.java	`0.00% <0.00%> (ø)`
.../kop/security/oauth/OauthLoginCallbackHandler.java	`0.00% <0.00%> (ø)`
... and 4 more

kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KafkaServiceConfiguration.java

BewareMyPower

We need to prevent consumers from subscribing group-B if they have configured the permission for group-A.

BewareMyPower · 2023-06-30T08:48:46Z

...c/main/java/io/streamnative/pulsar/handlers/kop/security/oauth/KopOAuthBearerSaslServer.java

+/**
+ * {@code SaslServer} implementation for SASL/OAUTHBEARER in Kafka. An instance
+ * of {@link OAuthBearerToken} is available upon successful authentication via
+ * the negotiated property "{@code OAUTHBEARER.token}"; the token could be used
+ * in a custom authorizer (to authorize based on JWT claims rather than ACLs,
+ * for example).
+ */


Did you just copy the Java docs from Kafka? It might be misleading here. It's better to remove them. For example, {@code SaslServer} and {@code OAUTHBEARER.token} never link correctly.

tests/src/test/java/io/streamnative/pulsar/handlers/kop/SaslOAuthKopHandlersWithGroupId.java

oauth-client/src/main/java/io/streamnative/pulsar/handlers/kop/security/oauth/ClientConfig.java

.../main/java/io/streamnative/pulsar/handlers/kop/security/oauth/OauthLoginCallbackHandler.java

BewareMyPower · 2023-07-04T13:02:38Z

@Demogorgon314 Could you resolve the conflicts?

Demogorgon314 · 2023-07-04T13:54:41Z

@BewareMyPower Resolved.

.../test/java/io/streamnative/pulsar/handlers/kop/security/oauth/ClientCredentialsFlowTest.java

...mpl/src/main/java/io/streamnative/pulsar/handlers/kop/security/auth/SimpleAclAuthorizer.java

…1926) ### Motivation Currently, the KoP only supports checking the topic permission when doing the consume authorization, but Kafka support checking the topic and group ID permission. Before introducing this change, let's understand why KoP can't check group ID permission. When Kafka does the consume authorization check, it will first check the group ID permission in `handleJoinGroupRequest` method, then it will check the topic permission in the `handleFetchRequest` method. However, Pulsar is using another way to check consume permission. See the `org.apache.pulsar.broker.authorization.AuthorizationService#canConsumeAsync` method, it requires passing the topic name and subscription to check both permissions in the same place, and the topic name is required param. ``` public CompletableFuture<Boolean> canConsumeAsync(TopicName topicName, String role, AuthenticationDataSource authenticationData,String subscription) ``` If we follow the Kafka way to check the consumer permission, we can't get the topic name when joining a group since the join group request does not contain the topic name. So we have to authorize permission in the fetch request. However, to authorization consume permission in the fetch request, we can only get the topic name in this request. In this case, we can't authorize the group ID. ### Modifications Since we can't get group ID when handling fetch requests, we need to find a way to pass through group ID when doing the authentication. In OAuth, we have a `credentials_file.json` file that needs to be config, for example: ``` { "client_id": "my-id", "client_secret": "my-secret", "tenant": "my-tenant" } ``` Here we can add a new parameter into the config: ``` { "client_id": "my-id", "client_secret": "my-secret", "tenant": "my-tenant", "group_id": "my-group-id" } ``` Then we can add these parameters to `SaslExtensions`, and send it to the broker. (cherry picked from commit e108e44)

…#1926) (#1945) ### Motivation Currently, the KoP only supports checking the topic permission when doing the consume authorization, but Kafka support checking the topic and group ID permission. Before introducing this change, let's understand why KoP can't check group ID permission. When Kafka does the consume authorization check, it will first check the group ID permission in `handleJoinGroupRequest` method, then it will check the topic permission in the `handleFetchRequest` method. However, Pulsar is using another way to check consume permission. See the `org.apache.pulsar.broker.authorization.AuthorizationService#canConsumeAsync` method, it requires passing the topic name and subscription to check both permissions in the same place, and the topic name is required param. ``` public CompletableFuture<Boolean> canConsumeAsync(TopicName topicName, String role, AuthenticationDataSource authenticationData,String subscription) ``` If we follow the Kafka way to check the consumer permission, we can't get the topic name when joining a group since the join group request does not contain the topic name. So we have to authorize permission in the fetch request. However, to authorization consume permission in the fetch request, we can only get the topic name in this request. In this case, we can't authorize the group ID. ### Modifications Since we can't get group ID when handling fetch requests, we need to find a way to pass through group ID when doing the authentication. In OAuth, we have a `credentials_file.json` file that needs to be config, for example: ``` { "client_id": "my-id", "client_secret": "my-secret", "tenant": "my-tenant" } ``` Here we can add a new parameter into the config: ``` { "client_id": "my-id", "client_secret": "my-secret", "tenant": "my-tenant", "group_id": "my-group-id" } ``` Then we can add these parameters to `SaslExtensions`, and send it to the broker. (cherry picked from commit e108e44)

### Motivation #1728 Introduced tenant for OAuth, and #1926 introduced group ID for OAuth, we should add docs.

…1926) ### Motivation Currently, the KoP only supports checking the topic permission when doing the consume authorization, but Kafka support checking the topic and group ID permission. Before introducing this change, let's understand why KoP can't check group ID permission. When Kafka does the consume authorization check, it will first check the group ID permission in `handleJoinGroupRequest` method, then it will check the topic permission in the `handleFetchRequest` method. However, Pulsar is using another way to check consume permission. See the `org.apache.pulsar.broker.authorization.AuthorizationService#canConsumeAsync` method, it requires passing the topic name and subscription to check both permissions in the same place, and the topic name is required param. ``` public CompletableFuture<Boolean> canConsumeAsync(TopicName topicName, String role, AuthenticationDataSource authenticationData,String subscription) ``` If we follow the Kafka way to check the consumer permission, we can't get the topic name when joining a group since the join group request does not contain the topic name. So we have to authorize permission in the fetch request. However, to authorization consume permission in the fetch request, we can only get the topic name in this request. In this case, we can't authorize the group ID. ### Modifications Since we can't get group ID when handling fetch requests, we need to find a way to pass through group ID when doing the authentication. In OAuth, we have a `credentials_file.json` file that needs to be config, for example: ``` { "client_id": "my-id", "client_secret": "my-secret", "tenant": "my-tenant" } ``` Here we can add a new parameter into the config: ``` { "client_id": "my-id", "client_secret": "my-secret", "tenant": "my-tenant", "group_id": "my-group-id" } ``` Then we can add these parameters to `SaslExtensions`, and send it to the broker. (cherry picked from commit e108e44)

Demogorgon314 self-assigned this Jun 28, 2023

github-actions bot added the doc-required This pr needs a document label Jun 28, 2023

Demogorgon314 marked this pull request as ready for review June 29, 2023 07:05

Demogorgon314 requested a review from BewareMyPower as a code owner June 29, 2023 07:05

Demogorgon314 commented Jun 29, 2023

View reviewed changes

kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KafkaServiceConfiguration.java Show resolved Hide resolved

BewareMyPower added release/2.11 release/2.10.4 release/3.0 type/enhancement Indicates an improvement to an existing feature labels Jun 30, 2023

BewareMyPower suggested changes Jun 30, 2023

View reviewed changes

Demogorgon314 force-pushed the Demogorgon314/pass-groupid-when-use-oauth branch from 3aa978d to 591c45b Compare June 30, 2023 05:35

BewareMyPower reviewed Jun 30, 2023

View reviewed changes

tests/src/test/java/io/streamnative/pulsar/handlers/kop/SaslOAuthKopHandlersWithGroupId.java Outdated Show resolved Hide resolved

BewareMyPower reviewed Jun 30, 2023

View reviewed changes

xiaogang-sn requested a review from BewareMyPower July 4, 2023 10:21

Demogorgon314 added 8 commits July 4, 2023 21:49

Pass groupId to authorizer when using OAuth

3067e03

Use SaslExtensions pass groupId

bd9eae9

Remove unused change

83c51ce

Fix test

819b2fe

Add check for different groupID

eb20cb3

rename the test class name

17beaed

Move ClientInfo out of class

879f342

Fix code style

dc9f1d7

Demogorgon314 force-pushed the Demogorgon314/pass-groupid-when-use-oauth branch from e159653 to dc9f1d7 Compare July 4, 2023 13:52

Fix comment

458b97d

Fix build

a8af9e1

BewareMyPower reviewed Jul 4, 2023

View reviewed changes

.../test/java/io/streamnative/pulsar/handlers/kop/security/oauth/ClientCredentialsFlowTest.java Show resolved Hide resolved

...mpl/src/main/java/io/streamnative/pulsar/handlers/kop/security/auth/SimpleAclAuthorizer.java Outdated Show resolved Hide resolved

Address comments

a0566e8

BewareMyPower approved these changes Jul 5, 2023

View reviewed changes

BewareMyPower merged commit e108e44 into streamnative:master Jul 5, 2023
17 of 19 checks passed

Demogorgon314 deleted the Demogorgon314/pass-groupid-when-use-oauth branch July 5, 2023 09:39

Demogorgon314 mentioned this pull request Jul 17, 2023

[docs] Add docs for OAuth credentials #1964

Merged

3 tasks

BewareMyPower pushed a commit that referenced this pull request Jul 31, 2023

[docs] Add docs for OAuth credentials (#1964)

308d75a

### Motivation #1728 Introduced tenant for OAuth, and #1926 introduced group ID for OAuth, we should add docs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve] Pass group ID to authorizer when using OAuth #1926

[improve] Pass group ID to authorizer when using OAuth #1926

Demogorgon314 commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 28, 2023 •

edited

Loading

BewareMyPower left a comment

BewareMyPower Jun 30, 2023

Demogorgon314 Jul 4, 2023

BewareMyPower commented Jul 4, 2023

Demogorgon314 commented Jul 4, 2023

[improve] Pass group ID to authorizer when using OAuth #1926

[improve] Pass group ID to authorizer when using OAuth #1926

Conversation

Demogorgon314 commented Jun 28, 2023 • edited Loading

Motivation

Modifications

Documentation

codecov bot commented Jun 28, 2023 • edited Loading

Codecov Report

BewareMyPower left a comment

Choose a reason for hiding this comment

BewareMyPower Jun 30, 2023

Choose a reason for hiding this comment

Demogorgon314 Jul 4, 2023

Choose a reason for hiding this comment

BewareMyPower commented Jul 4, 2023

Demogorgon314 commented Jul 4, 2023

Demogorgon314 commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 28, 2023 •

edited

Loading