[broker][authentication]Support pass http auth status #14044

tuteng · 2022-01-29T09:38:36Z

(If this PR fixes a github issue, please add Fixes #<xyz>.)

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

Currently, pulsar auth is divided into two parts, one is the authn and authz of the pulsar protocol (e.g. produce and consume) and the other is the authn and authz of the HTTP protocol (e.g. management of pulsar clusters), auth is divided into two phases authn and authz, currently in the authn phase will return a string role, authz phase will check this role's permissions, The string role contains very little information and that blocks some work in the authz phase, so in pulsar, there is an interface AuthenticationDataSource which is used to pass more information from the authn to the authz phase

In auth, there are two classes AuthenticationDataHttps and AuthenticationDataCommand that implement this interface AuthenticationDataSource. AuthenticationDataCommand is used to pass the state information after the pulsar protocol authentication. AuthenticationDataHttps is used to pass the status information after the HTTP protocol authentication. AuthenticationDataCommand and AuthenticationDataHttps are both default implementations, but now for the pulsar protocol there is support for using user-defined implementations https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L817, that gives the user the ability to extend the auth state and pass more information, but for the HTTP protocol data does not yet support the use of user-defined data, this pr implementation it.

Modifications

Add a new interface newHttpAuthState for passing HTTP auth state
Set auth method name for pulsar client
Fixed wrong method signatures

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API: (yes)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly (if you have committer privilege).

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)

github-actions · 2022-01-29T09:39:01Z

@tuteng:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

michaeljmarshall · 2022-02-03T06:02:49Z

@tuteng - since this is a new feature, I don't think we can cherry pick it to 2.8 or 2.9. I removed those labels.

michaeljmarshall

@tuteng - thanks for your contribution! I left some initial feedback. A few of the points are important to address.

It's possible that this PR is supposed to need a PIP. I don't personally think we need one, considering the scope of the PR and the fact that it is a reasonable addition to the existing interfaces. However, I'm not sure that I can officially waive the PIP process here.

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataBasic.java

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java

...client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationDataOAuth2.java

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java

eolivelli

Great work
I left a few small suggestions that I think it is worth to do.

eolivelli · 2022-02-23T07:44:00Z

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

        }

        @Override
        public String getAuthRole() throws AuthenticationException {
            return provider.getPrincipal(jwt);
        }

+        /**
+         * Here is an explanation of why the null value is returned.
+         * pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java#L49


Lines change in the future, please refer to a method name, using javadoc syntax

eolivelli · 2022-02-23T07:48:53Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataBasic.java


    public AuthenticationDataBasic(String userId, String password) {
        httpAuthToken = "Basic " + Base64.getEncoder().encodeToString((userId + ":" + password).getBytes());
        commandAuthToken = userId + ":" + password;
+        headers.put(HTTP_HEADER_NAME, httpAuthToken);


What about using a UnmofiableMap here?

eolivelli · 2022-02-23T07:50:21Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataTls.java

@@ -42,6 +45,8 @@
    @SuppressFBWarnings(value = "SE_TRANSIENT_FIELD_NOT_RESTORED",
            justification = "Using custom serializer which Findbugs can't detect")
    private transient Supplier<ByteArrayInputStream> certStreamProvider, keyStreamProvider, trustStoreStreamProvider;
+    private final Map<String, String> headers = Collections.singletonMap(


This can be static

eolivelli · 2022-02-23T07:50:48Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataToken.java


    public AuthenticationDataToken(Supplier<String> tokenSupplier) {
        this.tokenSupplier = tokenSupplier;
+        headers.put(PULSAR_AUTH_METHOD_NAME, AuthenticationToken.AUTH_METHOD_NAME);


Unmodifiable?

eolivelli · 2022-02-23T07:51:35Z

...client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationDataOAuth2.java


    public AuthenticationDataOAuth2(String accessToken) {
        this.accessToken = accessToken;
-        this.headers = Collections.singletonMap(HTTP_HEADER_NAME, "Bearer " + accessToken).entrySet();
+        headers.put(HTTP_HEADER_NAME, "Bearer " + accessToken);
+        headers.put(PULSAR_AUTH_METHOD_NAME, AuthenticationOAuth2.AUTH_METHOD_NAME);


Unmodifiable

tuteng · 2022-02-23T08:47:16Z

@eolivelli Thanks for your comment, PTAL

tuteng · 2022-02-24T03:15:59Z

@eolivelli Please review it again when you have time, thanks!

tuteng · 2022-02-25T02:29:07Z

@eolivelli Please review it again when you have time, thanks!

@eolivelli Please review it again when you have time, thanks!

tuteng · 2022-02-25T07:40:19Z

I will deal with the conflict later, you have time please take a look at it @eolivelli , thanks

Handle conflict

tuteng · 2022-02-27T14:20:08Z

ping @eolivelli PTAL

eolivelli · 2022-02-27T21:18:43Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataBasic.java

-        Map<String, String> headers = new HashMap<>();
-        headers.put(HTTP_HEADER_NAME, httpAuthToken);
-        return headers.entrySet();
+        return Collections.unmodifiableMap(this.headers).entrySet();


Why are you wrapping it all the times instead of creating this wrapper only once in the constructor? Here and in other places

Thanks, fixed

Fixes apache#14404 *(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>` to link to the master issue.)* Master Issue: #<xyz> Currently, pulsar auth is divided into two parts, one is the authn and authz of the pulsar protocol (e.g. produce and consume) and the other is the authn and authz of the HTTP protocol (e.g. management of pulsar clusters), auth is divided into two phases authn and authz, currently in the authn phase will return a string role, authz phase will check this role's permissions, The string role contains very little information and that blocks some work in the authz phase, so in pulsar, there is an interface `AuthenticationDataSource` which is used to pass more information from the authn to the authz phase In auth, there are two classes `AuthenticationDataHttps` and `AuthenticationDataCommand` that implement this interface `AuthenticationDataSource`. AuthenticationDataCommand is used to pass the state information after the pulsar protocol authentication. `AuthenticationDataHttps` is used to pass the status information after the HTTP protocol authentication. `AuthenticationDataCommand` and `AuthenticationDataHttps` are both default implementations, but now for the pulsar protocol there is support for using user-defined implementations https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L817, that gives the user the ability to extend the auth state and pass more information, but for the HTTP protocol data does not yet support the use of user-defined data, this pr implementation it. * Add a new interface `newHttpAuthState` for passing HTTP auth state * Set auth method name for pulsar client * Fixed wrong method signatures (cherry picked from commit 330fcb9) cherry-pick 14404

Fixes apache#14404 *(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>` to link to the master issue.)* Master Issue: #<xyz> ### Motivation Currently, pulsar auth is divided into two parts, one is the authn and authz of the pulsar protocol (e.g. produce and consume) and the other is the authn and authz of the HTTP protocol (e.g. management of pulsar clusters), auth is divided into two phases authn and authz, currently in the authn phase will return a string role, authz phase will check this role's permissions, The string role contains very little information and that blocks some work in the authz phase, so in pulsar, there is an interface `AuthenticationDataSource` which is used to pass more information from the authn to the authz phase In auth, there are two classes `AuthenticationDataHttps` and `AuthenticationDataCommand` that implement this interface `AuthenticationDataSource`. AuthenticationDataCommand is used to pass the state information after the pulsar protocol authentication. `AuthenticationDataHttps` is used to pass the status information after the HTTP protocol authentication. `AuthenticationDataCommand` and `AuthenticationDataHttps` are both default implementations, but now for the pulsar protocol there is support for using user-defined implementations https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L817, that gives the user the ability to extend the auth state and pass more information, but for the HTTP protocol data does not yet support the use of user-defined data, this pr implementation it. ### Modifications * Add a new interface `newHttpAuthState` for passing HTTP auth state * Set auth method name for pulsar client * Fixed wrong method signatures

…19197) PIP: #12105 ### Motivation This is the first of several PRs to implement [PIP 97](#12105). This PR seeks to start to solve the fact that the `AuthenticationState` class currently authenticates `authData` twice instead of just once. This change is important to make before we are able to utilize the async methods introduced in #12104. Historical context: #14044 introduced the `AuthenticationProvider#newHttpAuthState` method. The only use case for this method in the pulsar code base is to let custom providers specify the `AuthenticationDataSource` on http request attributes. The primary problem with that implementation is that the `AuthenticationState` class currently involves authenticating the `authData` passed in to the `newHttpAuthState`. As such, this code is sub-optimal, and creates a confusing flow. I propose we deprecate the `newHttpAuthState` method and instead start using the `authenticateHttpRequestAsync` and `authenticateHttpRequest` methods to allow custom implementations to configure the desired `AuthenticationDataSource` on the request attributes. In order to simplify the diff for reviewers, this PR uses the deprecated `AuthenticationProvider#authenticateHttpRequest` method. I plan to follow up and switch to use the `AuthenticationProvider#authenticateHttpRequestAsync` method. Note that these changes are completely backwards compatible. The only risk is to users that have custom code loaded into the broker that calls the `AuthenticationProvider#authenticateHttpRequest` method. ### Modifications * Deprecate `AuthenticationService#authenticateHttpRequest(HttpServletRequest request, AuthenticationDataSource authData)`. It is no longer used. * Deprecate `AuthenticationProvider#newHttpAuthState(HttpServletRequest request)`. It is no longer used outside of the `AuthenticationProvider` interface itself. * Remove `@InterfaceStability.Unstable` annotation from the `authenticateHttpRequestAsync` method. When I introduced that annotation, I was under the impression that we didn't need it. However, in order to meet the requirements introduced in #14044, we need to let custom `AuthenticationProviders` add their own attributes. * Update the default implementation of `authenticateHttpRequest`. Because the previous implementation was unreachable by all auth providers except for the SASL implementation, this is a backwards compatible change. * Implement changes for the `AuthenticationProviderToken` so that it no longer relies on `newHttpAuthState`. ### Verifying this change I added new tests. ### Does this pull request potentially affect one of the following parts: - [x] The public API This changes the public API within the broker by marking some methods as `@Deprecated`. ### Documentation - [x] `doc-not-needed` We document the `AuthenticationProvider` interface in the code. I added these docs. There is currently no where else to update docs. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#12 ### Additional motivation from PR discussion My primary motivation for this PR is to implement PIP 97. If we want authentication to be asynchronous, we cannot assume that when the `AuthenticationState` object is initialized, the `authenticationDataSource` and the `authRole` are present because the authentication might not yet be completed. My goal with this PR is to break an unnecessary relationship between `AuthenticationState` and http request authentication that was created by #14044.

Fixes apache#14404 *(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>` to link to the master issue.)* Master Issue: #<xyz> Currently, pulsar auth is divided into two parts, one is the authn and authz of the pulsar protocol (e.g. produce and consume) and the other is the authn and authz of the HTTP protocol (e.g. management of pulsar clusters), auth is divided into two phases authn and authz, currently in the authn phase will return a string role, authz phase will check this role's permissions, The string role contains very little information and that blocks some work in the authz phase, so in pulsar, there is an interface `AuthenticationDataSource` which is used to pass more information from the authn to the authz phase In auth, there are two classes `AuthenticationDataHttps` and `AuthenticationDataCommand` that implement this interface `AuthenticationDataSource`. AuthenticationDataCommand is used to pass the state information after the pulsar protocol authentication. `AuthenticationDataHttps` is used to pass the status information after the HTTP protocol authentication. `AuthenticationDataCommand` and `AuthenticationDataHttps` are both default implementations, but now for the pulsar protocol there is support for using user-defined implementations https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L817, that gives the user the ability to extend the auth state and pass more information, but for the HTTP protocol data does not yet support the use of user-defined data, this pr implementation it. * Add a new interface `newHttpAuthState` for passing HTTP auth state * Set auth method name for pulsar client * Fixed wrong method signatures (cherry picked from commit 330fcb9)

…pache#19197) PIP: apache#12105 This is the first of several PRs to implement [PIP 97](apache#12105). This PR seeks to start to solve the fact that the `AuthenticationState` class currently authenticates `authData` twice instead of just once. This change is important to make before we are able to utilize the async methods introduced in apache#12104. Historical context: apache#14044 introduced the `AuthenticationProvider#newHttpAuthState` method. The only use case for this method in the pulsar code base is to let custom providers specify the `AuthenticationDataSource` on http request attributes. The primary problem with that implementation is that the `AuthenticationState` class currently involves authenticating the `authData` passed in to the `newHttpAuthState`. As such, this code is sub-optimal, and creates a confusing flow. I propose we deprecate the `newHttpAuthState` method and instead start using the `authenticateHttpRequestAsync` and `authenticateHttpRequest` methods to allow custom implementations to configure the desired `AuthenticationDataSource` on the request attributes. In order to simplify the diff for reviewers, this PR uses the deprecated `AuthenticationProvider#authenticateHttpRequest` method. I plan to follow up and switch to use the `AuthenticationProvider#authenticateHttpRequestAsync` method. Note that these changes are completely backwards compatible. The only risk is to users that have custom code loaded into the broker that calls the `AuthenticationProvider#authenticateHttpRequest` method. * Deprecate `AuthenticationService#authenticateHttpRequest(HttpServletRequest request, AuthenticationDataSource authData)`. It is no longer used. * Deprecate `AuthenticationProvider#newHttpAuthState(HttpServletRequest request)`. It is no longer used outside of the `AuthenticationProvider` interface itself. * Remove `@InterfaceStability.Unstable` annotation from the `authenticateHttpRequestAsync` method. When I introduced that annotation, I was under the impression that we didn't need it. However, in order to meet the requirements introduced in apache#14044, we need to let custom `AuthenticationProviders` add their own attributes. * Update the default implementation of `authenticateHttpRequest`. Because the previous implementation was unreachable by all auth providers except for the SASL implementation, this is a backwards compatible change. * Implement changes for the `AuthenticationProviderToken` so that it no longer relies on `newHttpAuthState`. I added new tests. - [x] The public API This changes the public API within the broker by marking some methods as `@Deprecated`. - [x] `doc-not-needed` We document the `AuthenticationProvider` interface in the code. I added these docs. There is currently no where else to update docs. PR in forked repository: #12 My primary motivation for this PR is to implement PIP 97. If we want authentication to be asynchronous, we cannot assume that when the `AuthenticationState` object is initialized, the `authenticationDataSource` and the `authRole` are present because the authentication might not yet be completed. My goal with this PR is to break an unnecessary relationship between `AuthenticationState` and http request authentication that was created by apache#14044. (cherry picked from commit 3c38ed5)

Support pass http auth status

77b049f

github-actions bot assigned tuteng Jan 29, 2022

github-actions bot added the doc-label-missing label Jan 29, 2022

tuteng added 3 commits January 29, 2022 17:43

Fixed comment and style

ce3c5d3

Add method name for oauth2 auth

1badfaa

Change var type to private from public

cbc79e2

tuteng requested review from hangc0276, jiazhai, sijie, codelipenghui and merlimat and removed request for hangc0276 and jiazhai January 29, 2022 09:55

tuteng added release/2.8.3 release/2.9.3 labels Jan 29, 2022

Fixed style check

8924aa6

tuteng added area/authn component/authorization area/broker area/client area/security labels Jan 29, 2022

tuteng requested a review from addisonj February 3, 2022 01:43

michaeljmarshall self-requested a review February 3, 2022 06:02

michaeljmarshall removed release/2.8.3 release/2.9.3 labels Feb 3, 2022

michaeljmarshall requested changes Feb 3, 2022

View reviewed changes

eolivelli requested changes Feb 23, 2022

View reviewed changes

Update map headers to unmodifiableMap type

3764121

tuteng dismissed michaeljmarshall’s stale review via 3764121 February 23, 2022 08:46

michaeljmarshall approved these changes Feb 25, 2022

View reviewed changes

tuteng added 3 commits February 27, 2022 20:57

Merge branch 'master' into fixed/support-pass-http-auth-status

673818c

Handle conflict

Remove redundant final

49c1080

Fixed style

b1def58

eolivelli approved these changes Feb 27, 2022

View reviewed changes

Fixed comment

485e961

tuteng merged commit 330fcb9 into apache:master Feb 28, 2022

momo-jun mentioned this pull request Mar 11, 2022

[Doc] [Do-not-merge] add doc for Oauth2 authentication support in node.js client and update doc for custom AuthN/AuthZ #14659

Merged

1 task

Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-label-missing labels Apr 25, 2022

nodece mentioned this pull request Jun 12, 2022

[fix][broker] Fix token expiration #16016

Merged

1 task

BewareMyPower mentioned this pull request Jul 29, 2022

[improve][authentication] Support for get token from HTTP params #16650

Merged

1 task

tisonkun removed the component/authorization label Dec 6, 2022

michaeljmarshall mentioned this pull request Jan 11, 2023

[feat][broker] Update AuthenticationProvider to simplify HTTP Authn #19197

Merged

2 tasks

tisonkun mentioned this pull request May 19, 2023

org.apache.pulsar.broker.web.AuthenticationFilter Filter Out WebSocket with authentication token in query. #12912

Closed

nodece mentioned this pull request Dec 31, 2024

[fix][broker] Continue using the next provider for authentication if one fails #23797

Merged

4 tasks

nodece mentioned this pull request Jan 13, 2025

[fix][broker] Continue using the next provider for http authentication if one fails #23842

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[broker][authentication]Support pass http auth status #14044

[broker][authentication]Support pass http auth status #14044

tuteng commented Jan 29, 2022 •

edited

Loading

github-actions bot commented Jan 29, 2022

michaeljmarshall commented Feb 3, 2022

michaeljmarshall left a comment

eolivelli left a comment

eolivelli Feb 23, 2022

eolivelli Feb 23, 2022

eolivelli Feb 23, 2022

eolivelli Feb 23, 2022

eolivelli Feb 23, 2022

tuteng commented Feb 23, 2022

tuteng commented Feb 24, 2022

tuteng commented Feb 25, 2022

tuteng commented Feb 25, 2022

tuteng commented Feb 27, 2022

eolivelli Feb 27, 2022

tuteng Feb 28, 2022

[broker][authentication]Support pass http auth status #14044

[broker][authentication]Support pass http auth status #14044

Conversation

tuteng commented Jan 29, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

github-actions bot commented Jan 29, 2022

michaeljmarshall commented Feb 3, 2022

michaeljmarshall left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Feb 23, 2022

Choose a reason for hiding this comment

eolivelli Feb 23, 2022

Choose a reason for hiding this comment

eolivelli Feb 23, 2022

Choose a reason for hiding this comment

eolivelli Feb 23, 2022

Choose a reason for hiding this comment

eolivelli Feb 23, 2022

Choose a reason for hiding this comment

tuteng commented Feb 23, 2022

tuteng commented Feb 24, 2022

tuteng commented Feb 25, 2022

tuteng commented Feb 25, 2022

tuteng commented Feb 27, 2022

eolivelli Feb 27, 2022

Choose a reason for hiding this comment

tuteng Feb 28, 2022

Choose a reason for hiding this comment

tuteng commented Jan 29, 2022 •

edited

Loading