I discussed this feature with Matteo and Addison during yesterday's Pulsar Community Meeting. Matteo suggested that it might be easier to create a new interface as part of this work instead of adding a new method to the existing interface while maintaining backwards compatibility.
The new interface would be called AsyncAuthenticationProvider, and it would extend the existing AuthenticationProvider interface. It would have one additional method "authenticateAsync" and it would be up to the implementation to make sure that there is no blocking on the netty thread pool. We need the async interface to extend the synchronous one because of the way the AuthenticationService loads classes. It loads AuthenticationProvider implementations and then stores them in a single map from authentication method name to authentication provider. This map is useful. The easiest way to integrate with this is to have the new interface extend the AuthenticationProvider interface. Then, when we get the provider from the map, we can check its type and call "authenticateAsync" if it is an async authentication provider. Otherwise, we'll call "authenticate" and wrap the result with the appropriate completable future result. Note that there won't be a need to deprecate the synchronous AuthenticationProvider interface because there are, and will continue to be, legitimate use cases where an authentication provider does not rely on any kind of blocking or asynchronous calls. During the meeting, both Matteo and Addison seemed interested in this feature. As such, I plan to write a PIP and send it out next week. Please let me know if you have any thoughts on this design. Thanks, Michael On Mon, Aug 30, 2021 at 11:19 PM Michael Marshall <mikemars...@gmail.com> wrote: > Hello Pulsar Community, > > Pulsar's current AuthenticationProvider interface only exposes synchronous > methods for authenticating a connection. To date, this has been sufficient > because we have not had any providers that rely on network calls. However, > in looking at the OAuth2.0 spec, there are some cases where network calls > are necessary to verify a token. As a prerequisite to adding an OAuth2.0 > AuthenticationProvider, I propose the addition of asynchronous methods to > the AuthenticationProvider interface. The implementation would very closely > follow that of the AuthorizationProvider interface, which has both > synchronous and asynchronous method declarations. It would also require > refactoring any code that invokes the interface's "authenticate" method. > > Note that the "authenticate" method from the AuthenticationProvider is > called from a Netty thread and that a synchronous network call would block > that Netty thread, which we want to avoid at all costs. (Thank you to Lari > Hotari for pointing out this potential issue and its severity to me in a > separate conversation.) The introduction of async methods would allow for a > correct OAuth2.0 implementation without blocking any Netty threads. > > For reference, the OAuth2.0 spec defines a process for retrieving > Authorization Server Metadata from authorization servers here in RFC-8414: > https://datatracker.ietf.org/doc/html/rfc8414#section-3. In the OpenID > Connect spec, which is an implementation and extension of the OAuth2.0 > spec, there is an additional network call required to retrieve the Public > Key from an authorization server at its `jwks_uri`. Here is a reference to > the spec where it briefly mentions the network call: > https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys > > If there is support and interest in this idea, I am happy to put together > a PIP and implement the change, once the PIP is accepted. > > I'd also like to note that there is already a draft PR ( > https://github.com/apache/pulsar/pull/11794) proposing a new > authentication provider that would retrieve JSON Web Key sets from a > configured URI. This proposal would impact that PR, as it proposes making a > network call in the AuthenticationProvider's authenticate method. > > Thanks, > Michael Marshall > >
