[PIP 97] Update Authentication Interfaces to Include Async Authentication Methods #12104

michaeljmarshall · 2021-09-20T20:58:41Z

Master Issue: #12105

Motivation

As the first part of PIP-97, we need to update the interfaces. This PR is the only PR that will update interfaces. It should not introduce any breaking changes.

Modifications

AuthenticationProvider

Add AuthenticationProvider#authenticateAsync. Include a default implementation that calls the authenticate method. Note that current implementations should all be non-blocking, so there is no need to push the execution to a separate thread.
Deprecate AuthenticationProvider#authenticate.
Add AuthenticationProvider#authenticateHttpRequestAsync. This method is complicated. It is only called when using the SASL authentication provider (this is hard coded into the Pulsar code base). As such, I would argue that it is worth removing support for this unreachable method and then refactor the SASL authentication provider. I annotated this method with @InterfaceStability.Unstable and added details to the Javadoc in order to communicate the uncertainty of this method's future. I am happy to discuss this in more detail though.
Deprecate AuthenticationProvider#authenticateHttpRequest.

AuthenticationState

Add AuthenticationState#authenticateAsync. Include a default implementation that calls the authenticate method and then performs a check to determine what result to return. Note that current implementations should all be non-blocking, so there is no need to push the execution to a separate thread.
Deprecate AuthenticationState#authenticate. The preferred method is AuthenticationState#authenticateAsync.
Deprecate AuthenticationState#isComplete. This method can be avoided by inferring authentication completeness from the result of AuthenticationState#authenticateAsync. When the result is null, auth is complete. When it is not null, auth is not complete. Since the result of the authenticateAsync method is the body delivered to the client, this seems like a reasonable abstraction to make. As a consequence, the AuthenticationState is simpler and also avoids certain thread safety issues that might arise when calling isComplete from a different thread.

AuthenticationDataSource

Deprecate AuthenticationDataSource#authenticate. This method is not called by the Pulsar authentication framework. This needs to be deprecated to prevent confusion for end users seeking to extend the authentication framework. There is no need for an async version of this method.

Verifying this change

These changes only affect the interfaces. I will need to add tests to verify the correctness of the default implementations in this PR.

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

Yes, it affects the public API. That is why it has a PIP.

Documentation

I've updated the Javadocs. There is not any current documentation on implementing your own authentication provider, so I think updating Javadocs is sufficient documentation, for now.

Anonymitaet · 2021-09-22T03:20:40Z

...ker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java

@@ -51,6 +54,29 @@
     */
    String getAuthMethodName();

+    /**
+     * Validate the authentication for the given credentials with the specified authentication data.
+     * This method is useful in one stage authn, if you're not doing one stage or if you're providing


Some people know "authn is short for authentication, and authz is short for authorization." while others may not. It is suggested to spell out "authentication" or "authorization" to make everyone knows the info.

Great point @Anonymitaet. I just updated it to authentication.

RobertIndie

It looks like the method implementation here is still synchronous. Or are you planning to implement asynchronous in another PR?

michaeljmarshall · 2021-09-23T18:42:25Z

It looks like the method implementation here is still synchronous. Or are you planning to implement asynchronous in another PR?

I address this concern in the PR description. All current implementations of the AuthenticationProvider interface should be non-blocking, otherwise they are already blocking IO threads. The default implementation of authenticateAsync preserves the current behavior for all implementations. Further, my proposed design expects implementations to manage their own concurrency model. The Javadoc states this design decision.

I will submit subsequent PRs for each of the Pulsar authentication provider implementations to ensure that they remove implementations of the deprecated methods.

michaeljmarshall · 2021-09-29T16:54:23Z

/pulsarbot run-failure-checks

michaeljmarshall · 2021-10-04T14:49:59Z

@eolivelli - this PR doesn't have a label yet. It should target 2.10.

eolivelli

Looks great to me

@merlimat PTAL

michaeljmarshall · 2021-11-11T07:21:08Z

Thank you for merging this @codelipenghui. I will follow up next week with the implementation described in the master issue.

…tion Methods (apache#12104) Master Issue: apache#12105 ### Motivation As the first part of PIP-97, we need to update the interfaces. This PR is the only PR that will update interfaces. It should not introduce any breaking changes. ### Modifications #### AuthenticationProvider * Add `AuthenticationProvider#authenticateAsync`. Include a default implementation that calls the `authenticate` method. Note that current implementations should all be non-blocking, so there is no need to push the execution to a separate thread. * Deprecate `AuthenticationProvider#authenticate`. * Add `AuthenticationProvider#authenticateHttpRequestAsync`. This method is complicated. It is only called when using the SASL authentication provider (this is hard coded into the Pulsar code base). As such, I would argue that it is worth removing support for this unreachable method and then refactor the SASL authentication provider. I annotated this method with `@InterfaceStability.Unstable` and added details to the Javadoc in order to communicate the uncertainty of this method's future. I am happy to discuss this in more detail though. * Deprecate `AuthenticationProvider#authenticateHttpRequest`. #### AuthenticationState * Add `AuthenticationState#authenticateAsync`. Include a default implementation that calls the `authenticate` method and then performs a check to determine what result to return. Note that current implementations should all be non-blocking, so there is no need to push the execution to a separate thread. * Deprecate `AuthenticationState#authenticate`. The preferred method is `AuthenticationState#authenticateAsync`. * Deprecate `AuthenticationState#isComplete`. This method can be avoided by inferring authentication completeness from the result of `AuthenticationState#authenticateAsync`. When the result is `null`, auth is complete. When it is not `null`, auth is not complete. Since the result of the `authenticateAsync` method is the body delivered to the client, this seems like a reasonable abstraction to make. As a consequence, the `AuthenticationState` is simpler and also avoids certain thread safety issues that might arise when calling `isComplete` from a different thread. #### AuthenticationDataSource * Deprecate `AuthenticationDataSource#authenticate`. This method is not called by the Pulsar authentication framework. This needs to be deprecated to prevent confusion for end users seeking to extend the authentication framework. There is no need for an async version of this method. ### Verifying this change These changes only affect the interfaces. I will need to add tests to verify the correctness of the default implementations in this PR. ### Does this pull request potentially affect one of the following parts: Yes, it affects the public API. That is why it has a PIP. ### Documentation I've updated the Javadocs. There is not any current documentation on implementing your own authentication provider, so I think updating Javadocs is sufficient documentation, for now.

…thentication Methods (apache#12104)" This reverts commit 5868025.

…thentication Methods (#12104)" (#14424) This reverts commit 5868025. Master Issue: #12105 ### Motivation Because the PIP is not completely implemented, we should not publish the interface changes in 2.10. ### Modifications * Revert the core commit for PIP 97. ### Verifying this change This change is trivial, because the original change was completely backwards compatible. ### Documentation - [x] `no-need-doc`

BewareMyPower · 2022-12-13T08:28:57Z

...ker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java

     */
+    @Deprecated


Could you explain more about it? IMO, it should not be deprecated. I tried to implement an AuthenticationProvider that only overrides the authenticationAsync method but it never went here. Instead, the authenticate method was still called. There is no test to show it should be deprecated.

It's weird that this PR should have been reverted in #14424, but it still exists in master branch.

@BewareMyPower - I am not sure of the right way to proceed here. I had plans to complete the work for PIP 97, but I haven't been able to do so. I reverted it from branch-2.10 because the necessary implementation for making the ServerCnx and the http filter async was not in place. I will think about this some more and try to find the right approach.

Note that this PIP isn't needed unless you're doing something blocking when authenticate is called.

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

…tion Methods (apache#12104) Master Issue: apache#12105 ### Motivation As the first part of PIP-97, we need to update the interfaces. This PR is the only PR that will update interfaces. It should not introduce any breaking changes. ### Modifications #### AuthenticationProvider * Add `AuthenticationProvider#authenticateAsync`. Include a default implementation that calls the `authenticate` method. Note that current implementations should all be non-blocking, so there is no need to push the execution to a separate thread. * Deprecate `AuthenticationProvider#authenticate`. * Add `AuthenticationProvider#authenticateHttpRequestAsync`. This method is complicated. It is only called when using the SASL authentication provider (this is hard coded into the Pulsar code base). As such, I would argue that it is worth removing support for this unreachable method and then refactor the SASL authentication provider. I annotated this method with `@InterfaceStability.Unstable` and added details to the Javadoc in order to communicate the uncertainty of this method's future. I am happy to discuss this in more detail though. * Deprecate `AuthenticationProvider#authenticateHttpRequest`. #### AuthenticationState * Add `AuthenticationState#authenticateAsync`. Include a default implementation that calls the `authenticate` method and then performs a check to determine what result to return. Note that current implementations should all be non-blocking, so there is no need to push the execution to a separate thread. * Deprecate `AuthenticationState#authenticate`. The preferred method is `AuthenticationState#authenticateAsync`. * Deprecate `AuthenticationState#isComplete`. This method can be avoided by inferring authentication completeness from the result of `AuthenticationState#authenticateAsync`. When the result is `null`, auth is complete. When it is not `null`, auth is not complete. Since the result of the `authenticateAsync` method is the body delivered to the client, this seems like a reasonable abstraction to make. As a consequence, the `AuthenticationState` is simpler and also avoids certain thread safety issues that might arise when calling `isComplete` from a different thread. #### AuthenticationDataSource * Deprecate `AuthenticationDataSource#authenticate`. This method is not called by the Pulsar authentication framework. This needs to be deprecated to prevent confusion for end users seeking to extend the authentication framework. There is no need for an async version of this method. ### Verifying this change These changes only affect the interfaces. I will need to add tests to verify the correctness of the default implementations in this PR. ### Does this pull request potentially affect one of the following parts: Yes, it affects the public API. That is why it has a PIP. ### Documentation I've updated the Javadocs. There is not any current documentation on implementing your own authentication provider, so I think updating Javadocs is sufficient documentation, for now. (cherry picked from commit 5868025)

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

Update Authentication Interfaces to Include Async Authentication Methods

499301a

michaeljmarshall mentioned this pull request Sep 20, 2021

PIP 97: Asynchronous Authentication Provider #12105

Open

sijie mentioned this pull request Sep 20, 2021

ISSUE-12105: PIP 97: Asynchronous Authentication Provider streamnative/pulsar-archived#3075

Open

Switch should with must in Javadoc

aa0014d

Anonymitaet reviewed Sep 22, 2021

View reviewed changes

Anonymitaet added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 22, 2021

RobertIndie reviewed Sep 23, 2021

View reviewed changes

Change authn to authentication in Javadoc

98d4867

eolivelli assigned michaeljmarshall Oct 4, 2021

eolivelli added the area/security label Oct 4, 2021

eolivelli approved these changes Oct 4, 2021

View reviewed changes

Anonymitaet added this to the 2.10.0 milestone Oct 11, 2021

codelipenghui approved these changes Nov 10, 2021

View reviewed changes

codelipenghui merged commit 5868025 into apache:master Nov 10, 2021

michaeljmarshall deleted the async-authentication branch January 20, 2022 05:34

michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Jan 20, 2022

Revert "[PIP 97] Update Authentication Interfaces to Include Async Au…

1fd5834

…thentication Methods (apache#12104)" This reverts commit 5868025.

michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Feb 23, 2022

Revert "[PIP 97] Update Authentication Interfaces to Include Async Au…

97e213f

…thentication Methods (apache#12104)" This reverts commit 5868025.

michaeljmarshall mentioned this pull request Feb 23, 2022

Revert "[PIP 97] Update Authentication Interfaces to Include Async Au… #14424

Merged

1 task

michaeljmarshall modified the milestones: 2.10.0, 2.11.0 Feb 23, 2022

BewareMyPower reviewed Dec 13, 2022

View reviewed changes

michaeljmarshall mentioned this pull request Jan 11, 2023

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

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PIP 97] Update Authentication Interfaces to Include Async Authentication Methods #12104

[PIP 97] Update Authentication Interfaces to Include Async Authentication Methods #12104

michaeljmarshall commented Sep 20, 2021 •

edited

Loading

Anonymitaet Sep 22, 2021

michaeljmarshall Sep 23, 2021

RobertIndie left a comment

michaeljmarshall commented Sep 23, 2021

michaeljmarshall commented Sep 29, 2021

michaeljmarshall commented Oct 4, 2021

eolivelli left a comment

michaeljmarshall commented Nov 11, 2021

BewareMyPower Dec 13, 2022

BewareMyPower Dec 13, 2022

michaeljmarshall Dec 14, 2022

[PIP 97] Update Authentication Interfaces to Include Async Authentication Methods #12104

[PIP 97] Update Authentication Interfaces to Include Async Authentication Methods #12104

Conversation

michaeljmarshall commented Sep 20, 2021 • edited Loading

Motivation

Modifications

AuthenticationProvider

AuthenticationState

AuthenticationDataSource

Verifying this change

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

Documentation

Anonymitaet Sep 22, 2021

Choose a reason for hiding this comment

michaeljmarshall Sep 23, 2021

Choose a reason for hiding this comment

RobertIndie left a comment

Choose a reason for hiding this comment

michaeljmarshall commented Sep 23, 2021

michaeljmarshall commented Sep 29, 2021

michaeljmarshall commented Oct 4, 2021

eolivelli left a comment

Choose a reason for hiding this comment

michaeljmarshall commented Nov 11, 2021

BewareMyPower Dec 13, 2022

Choose a reason for hiding this comment

BewareMyPower Dec 13, 2022

Choose a reason for hiding this comment

michaeljmarshall Dec 14, 2022

Choose a reason for hiding this comment

michaeljmarshall commented Sep 20, 2021 •

edited

Loading