Re: Add Asynchronous Methods to AuthenticationProvider Interface

[Michael Marshall <mi...@gmail.com>]

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 <mi...@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
>
>

Re: Add Asynchronous Methods to AuthenticationProvider Interface

[Michael Marshall <mi...@gmail.com>]

> I was not exactly suggesting to have AsyncAuthenticationProvider
> extend the current interface :)

Thank you for clarifying. :) You mentioned a bridging class in the
community meeting yesterday, but I misunderstood its implementation.

Your AsyncAuthBridge implementation makes sense to me. The main
modification I'll make is that we'll use an authenticate method instead of
a canPublish method.

Thanks,
Michael

On Fri, Sep 3, 2021 at 2:04 PM Matteo Merli <ma...@gmail.com> wrote:

> On Fri, Sep 3, 2021 at 11:51 AM Michael Marshall <mi...@gmail.com>
> wrote:
> > 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.
>
> I was not exactly suggesting to have AsyncAuthenticationProvider
> extend the current interface :)
>
> The callers should be always using the async interface, so we don't
> need to have the sync methods in the new interface.
>
> Instead we'd have 2 interface and bridging class that would look like:
>
> class AsyncAuthBridge implements AsyncAuthenticationProvider {
>       AuthenticationProvider delegate;
>       Executor e;
>
>      CompletableFuture<Boolean> canPublishAsync(String topic,
> AuthenticationDataSource s) {
>          CompletableFuture<Boolean> f = new CompletableFuture<>();
>          e.execute(() -> {
>                f.complete(delegate.canPublish(topic, s));
>          }
>          return f;
>     }
> }
>

Re: Add Asynchronous Methods to AuthenticationProvider Interface

[Matteo Merli <ma...@gmail.com>]

On Fri, Sep 3, 2021 at 11:51 AM Michael Marshall <mi...@gmail.com> wrote:
> 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.

I was not exactly suggesting to have AsyncAuthenticationProvider
extend the current interface :)

The callers should be always using the async interface, so we don't
need to have the sync methods in the new interface.

Instead we'd have 2 interface and bridging class that would look like:

class AsyncAuthBridge implements AsyncAuthenticationProvider {
      AuthenticationProvider delegate;
      Executor e;

     CompletableFuture<Boolean> canPublishAsync(String topic,
AuthenticationDataSource s) {
         CompletableFuture<Boolean> f = new CompletableFuture<>();
         e.execute(() -> {
               f.complete(delegate.canPublish(topic, s));
         }
         return f;
    }
}