KAFKA-14062: OAuth client token refresh fails with SASL extensions

[kirktrue]

Authored by @emissionnebula

What

Kafka client is adding and removing the SASL extensions alternatively at the time of token refresh. During the window when the extensions are not present in the subject. If a connection to a broker is reattempted, it fails with the error that the extensions are missing.

See KAFKA-14062 for more information.

Why

In clients, a Subject object is maintained which contains two sets each for Private and Public Credentials. Public Credentials includes the extensions. These values are stored in a SaslExtensions object which internally maintains these in a HashMap.

At the time of token refresh, a SaslExtensions object with these extensions is added to the public credentials set. As a next step, the refresh thread tries to logout the client for the older credentials. So it tries to remove the older token (private credential) and older SaslExtensions object (public credential) from the sets maintained in the Subject object.

SaslExtensions Class overrides the equals and hashcode functions and directly calls the equals and hashcode functions of HashMap. So at the time refresh when a new SaslExtensions object is added, because the extension values don't change, it results in a no-op because the hashes of the existing SaslExtensions object and the new object will be equals. But in the logout step, the only SaslExtensions object present in the set gets removed.

After removing the extensions in 1st refresh, the extensions will get added again at the time of 2nd refresh. So, this addition and removal keep happening alternatively.

The addition and removal of private credentials (tokens) from Subject work just fine because the tokens are always different.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[kirktrue]

cc @omkreddy @YiDing-Duke @stanislavkozlovski @rite2nikhil @emissionnebula @ijuma

[dajac]

Could we add a unit test?

[kirktrue]

Thanks for the feedback, @dajac! 😄

Could we add a unit test?

Yes, I'd like to see that too.

@emissionnebula: has the testing for this been mostly manual up to this point?

I am willing to try to come up with one or more tests around this, but I can't commit to it.

[kirktrue]

I've modified some existing unit tests in OAuthBearerLoginModuleTest that appear to exercise/mimic the refresh logic.

If the equals and hashCode method implementations in the SaslExtensions class are left in, the tests fail. When I remove the methods (as per @emissionnebula's change), the unit tests pass as expected.

[kirktrue]

The issue really does seem to boil down to how the Subject class implicitly expects the objects in its public credentials set to behave. They seem to really be expected to be unique. I'm not sure if we're violating some assumption or tenet in OAuthBearerLoginModule, SaslExtensions, or not.

I don't see a way to change OAuthBearerLoginModule to account for the case when two instances in the set evaluated to being equal.

[kirktrue]

cc @rondagostino

[kirktrue]

Looking at this a little more, I see multiple classes (OAuthBearerLoginModule, OAuthBearerRefreshingLogin, ExpiringCredentialRefreshingLogin, etc.) that seem to play a part in the login process. Given that there is a dedicated refresh thread that modifies the Subject, I'm still wondering if there's some kind of race condition here 🤔

[omkreddy]

@kirktrue Thanks for the PR. LGTM