Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][broker] AuthenticationState authenticate data once #19280

Closed

Conversation

michaeljmarshall
Copy link
Member

PIP 97: #12105

Motivation

In order to implement PIP 97, it is essential that we remove all blocking calls to provider.authenticate(authData). Two places where we currently call the synchronous authenticate method are in the OneStageAuthenticationState and the TokenAuthenticationState class constructors. Since authenticate will be replaced with authenticateAsync, we won't be able to rely on authenticating the authData in the constructor. Further, it is inefficient to verify authentication data twice, which currently happens for TokenAuthenticationState in the ProxyConnection and the ServerCnx.

This PR seeks to make explicit a paradigm that is already followed in the Pulsar code base. Specifically, that the contract for using an AuthenticationState class is as follows:

  1. Initializing the class by calling {@link AuthenticationProvider#newAuthState(SocketAddress, SSLSession)}
  2. Calling {@link #authenticate(AuthData)} in a loop until the result is null.
  3. Calling {@link #getAuthRole()} and {@link #getAuthDataSource()} to use for authentication.
  4. Calling {@link #refreshAuthentication()} when {@link #isExpired()} returns true.
  5. GoTo step 3.

When PIP 97 is complete, step 2 will replace authenticate with authenticateAsync.

Modifications

  • Add AuthenticationProvider method: newAuthState(SocketAddress remoteAddress, SSLSession sslSession). This method does not take AuthData to make it a bit clearer that class constructors are not meant to perform authentication. This method's default definition is OneStageAuthenticationState. Users that want a custom AuthenticationProvider to use a different AuthenticationState implementation must override this method.

  • Add @Deprecated annotation to newAuthState(AuthData authData, SocketAddress remoteAddress, SSLSession sslSession) and to several class constructors that were used by that method. Note: leave the implementations the same for broker extensions that use this method. This is the only backwards compatible portion of this PR.

  • Update ProxyConnection and ServerCnx to use the new newAuthState method. This change could break custom AuthenticationState implementations as described in the first bullet point.

  • Update TokenAuthenticationState and OneStageAuthenticationState to only authenticate tokens in the authenticate method.

Verifying this change

There are new tests and there is already some test coverage.

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

  • The public API

This change affects the AuthenticationProvider interface.

Documentation

  • doc-not-needed

We document the authentication interfaces in the code, and I've added docs there.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#14

@michaeljmarshall
Copy link
Member Author

After thinking about this more, I think we should probably take a different direction. I'll follow up with another PR.

@michaeljmarshall
Copy link
Member Author

This work will end up as multiple PRs and I think I've found a way to avoid confusing breaking changes. The first PR that was initially covered in this PR is here: #19282. The second is here #19283. I expect at least one more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/authn doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant