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

michaeljmarshall · 2023-01-11T22:51:56Z

Motivation

This is the first of several PRs to implement PIP 97.

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 [broker][authentication]Support pass http auth status #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:

The public API

This changes the public API within the broker by marking some methods as @Deprecated.

Documentation

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

nodece · 2023-01-13T05:42:17Z

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

@@ -160,6 +163,20 @@ public String authenticate(AuthenticationDataSource authData) throws Authenticat
        }
    }

+    @Override
+    public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletResponse response) throws Exception {


Don't need to override authenticateHttpRequest method.

See newHttpAuthState method, we have wrapped the request.

This PR deprecates newHttpAuthState because that method creates an object that is unnecessary and that triggers an authentication check for no reason in the OneStageAuthenticationState class.

Note that I am leaving newHttpAuthState as is in case someone already relies on the method itself.

Your PR is awesome! but I have second thoughts about whether to deprecate the newHttpAuthState, I don't suggest you deprecate this method,

Using newHttpAuthState returns AuthenticationState, which includes role and authentication data, we can simply get these from AuthenticationState, and also quickly check the user authentication data.

Keep the same logic with the newAuthState, it looks cleaner.

This PR deprecates newHttpAuthState because that method creates an object that is unnecessary and that triggers an authentication check for no reason in the OneStageAuthenticationState class.

I can accept authentication checks in the constructor of OneStageAuthenticationState, it is a quick check.

Maybe we can improve here, but the Pulsar must explicitly call the authenticate of AuthenticationState.

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.

Using newHttpAuthState returns AuthenticationState, which includes role and authentication data, we can simply get these from AuthenticationState, and also quickly check the user authentication data.

This is not the intention of the newHttpAuthState method. The method was added by #14044 so that the AuthenticationDataSource for an AuthenticationProvider can be added on the http request as the AuthenticatedDataAttributeName attribute on the request. This PR proposes an alternate way to meet those criteria so that we can implement PIP 97.

Keep the same logic with the newAuthState, it looks cleaner.

I disagree that it looks cleaner. The primary reason that we need a state object in the pulsar protocol handlers is that we track isExpired and whether or not the authentication is complete (first with isComplete and in 2.12 with the result of the authenticationAsync). We do not track state for individual HTTP request connections, so we shouldn't have a method to create a state object for HTTP requests.

Further, we're only building the AuthenticationState object to get the AuthenticationDataSource and we're not calling the AuthenticationState#authenticate method. There is no need for this indirection. We can give AuthenticationProvider implementations freedom to add the right request attributes by using the authenticateHttpRequest (and eventually authenticateHttpRequestAsync).

Also, another problem is with the AuthenticationDataSource in the AuthenticationState. I think we should recreate the AuthenticationDataSource each time we call authenticate. However, if we have newHttpAuthState, the AuthenticationState object is not able to create a new AuthenticationDataSource. I think the newHttpAuthState creates an confusion about how to use the AuthenticationState. This is a separate issue that I will follow up on in another PR.

I can accept authentication checks in the constructor of OneStageAuthenticationState, it is a quick check.

It does not matter that it is "quick". It is a waste of resources to verify authentication information twice.

Maybe we can improve here, but the Pulsar must explicitly call the authenticate of AuthenticationState.

This is already the way that Pulsar operates, but it has not been a stated requirement. I think this is essential for making authentication asynchronous. I will follow up with a separate PR.

Thank you for your explanation. This looks good to me.

Thank you for sharing your concerns. It is great to have this context written down in the PR for future reference.

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

lhotari

LGTM

…sts-pre-work

codecov-commenter · 2023-01-17T22:26:34Z

Codecov Report

Merging #19197 (8786d14) into master (334c3a5) will increase coverage by 0.18%.
The diff coverage is 21.99%.

@@             Coverage Diff              @@
##             master   #19197      +/-   ##
============================================
+ Coverage     47.04%   47.22%   +0.18%     
- Complexity     9190    18840    +9650     
============================================
  Files           607     1625    +1018     
  Lines         57677   132324   +74647     
  Branches       6007    14567    +8560     
============================================
+ Hits          27132    62491   +35359     
- Misses        27598    63528   +35930     
- Partials       2947     6305    +3358

Flag	Coverage Δ
unittests	`47.22% <21.99%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../broker/authentication/AuthenticationProvider.java	`7.14% <0.00%> (ø)`
...r/broker/authentication/AuthenticationService.java	`0.00% <0.00%> (ø)`
...apache/pulsar/broker/web/AuthenticationFilter.java	`15.00% <0.00%> (ø)`
...rg/apache/pulsar/broker/delayed/bucket/Bucket.java	`0.00% <0.00%> (ø)`
...r/delayed/bucket/BucketDelayedDeliveryTracker.java	`0.00% <0.00%> (ø)`
...layed/bucket/CombinedSegmentDelayedIndexQueue.java	`0.00% <0.00%> (ø)`
...ulsar/broker/delayed/bucket/DelayedIndexQueue.java	`0.00% <0.00%> (ø)`
.../pulsar/broker/delayed/bucket/ImmutableBucket.java	`0.00% <0.00%> (ø)`
...he/pulsar/broker/delayed/bucket/MutableBucket.java	`0.00% <0.00%> (ø)`
...ed/bucket/TripleLongPriorityDelayedIndexQueue.java	`0.00% <0.00%> (ø)`
... and 1065 more

…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)

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

536b800

michaeljmarshall added release/note-required doc-not-needed Your PR changes do not impact docs area/authn labels Jan 11, 2023

michaeljmarshall added this to the 2.12.0 milestone Jan 11, 2023

michaeljmarshall requested review from lhotari, tuteng, eolivelli, nodece and liudezhi2098 January 11, 2023 22:51

michaeljmarshall self-assigned this Jan 11, 2023

michaeljmarshall added 2 commits January 11, 2023 17:44

Implement changes for AuthenticationProviderToken

e840148

Include AuthenticatedDataAttributeName for anonymous role

1df8a18

michaeljmarshall added the ready-to-test label Jan 12, 2023

nodece reviewed Jan 13, 2023

View reviewed changes

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

lhotari approved these changes Jan 17, 2023

View reviewed changes

michaeljmarshall added 2 commits January 17, 2023 15:18

Handle SASL edge case

767427e

Merge remote-tracking branch 'upstream/master' into pip-97-http-reque…

8786d14

…sts-pre-work

nodece approved these changes Jan 19, 2023

View reviewed changes

michaeljmarshall merged commit 3c38ed5 into apache:master Jan 19, 2023

michaeljmarshall deleted the pip-97-http-requests-pre-work branch January 19, 2023 05:38

This was referenced Jan 23, 2023

[improve][broker] Replace authenticate with authenticateAsync #19313

Merged

[fix][broker] TokenAuthenticationState: authenticate token only once #19314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michaeljmarshall commented Jan 11, 2023 •

edited

Loading

nodece Jan 13, 2023

michaeljmarshall Jan 17, 2023

michaeljmarshall Jan 17, 2023

nodece Jan 18, 2023

michaeljmarshall Jan 18, 2023

nodece Jan 19, 2023

michaeljmarshall Jan 19, 2023

lhotari left a comment

codecov-commenter commented Jan 17, 2023

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

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

Conversation

michaeljmarshall commented Jan 11, 2023 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

nodece Jan 13, 2023

Choose a reason for hiding this comment

michaeljmarshall Jan 17, 2023

Choose a reason for hiding this comment

michaeljmarshall Jan 17, 2023

Choose a reason for hiding this comment

nodece Jan 18, 2023

Choose a reason for hiding this comment

michaeljmarshall Jan 18, 2023

Choose a reason for hiding this comment

nodece Jan 19, 2023

Choose a reason for hiding this comment

michaeljmarshall Jan 19, 2023

Choose a reason for hiding this comment

lhotari left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 17, 2023

Codecov Report

michaeljmarshall commented Jan 11, 2023 •

edited

Loading