You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/20 21:01:08 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
michaeljmarshall opened a new pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104
Master Issue:
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104#discussion_r715061785
##########
File path: pulsar-broker-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
Review comment:
Great point @Anonymitaet. I just updated it to authentication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104#issuecomment-933562187
@eolivelli - this PR doesn't have a label yet. It should target 2.10.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104#issuecomment-966051350
Thank you for merging this @codelipenghui. I will follow up next week with the implementation described in the master issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104#issuecomment-930356650
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104#issuecomment-926063477
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104#issuecomment-930356650
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #12104: [PIP 97] Update Authentication Interfaces to Include Async Authentication Methods
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #12104:
URL: https://github.com/apache/pulsar/pull/12104#discussion_r713561141
##########
File path: pulsar-broker-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
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org