-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support SASL OAuthBearer Authentication #1750
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few comments in-line for your consideration.
|
||
def _build_oauth_client_request(self): | ||
token_provider = self.config['sasl_oauth_token_provider'] | ||
return "n,,\x01auth=Bearer {}{}\x01\x01".format(token_provider.token(), self._token_extensions()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is string format()
going to guarantee correct encoding here? I might suggest operating on and returning bytes
instead. E.g., b''.join([b'n,,\x01auth=Bearer ', token, extensions, b'\x01\x01'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I convert to bytes and encode the output of this function in _try_authenticate_oauth
so I believe the encoding will be correct.
import abc | ||
|
||
# This statement is compatible with both Python 2.7 & 3+ | ||
ABC = abc.ABCMeta('ABC', (object,), {'__slots__': ()}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting -- haven't seen this before
The implementation shsould ensure token reuse so that multiple | ||
calls at connect time do not create multiple tokens. The implementation | ||
should also periodically refresh the token in order to guarantee | ||
that each call returns an unexpired token. A timeout error should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this timeout error work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to the implementer but ideally if the Token Provider pings some OAuth server for a token, if it takes too long it shouldn't go on forever. This was something that I've read/done in other OAuth implementations for different Kafka client libraries.
Thanks for this! |
This PR adds support for OAuthBearer as a method of SASL Authentication as described in KIP-255 and #1710.
When initializing the client, the user must pass in an instance of a class that implements a token method, which returns an ID or Access Token. The interface is described in the documentation for every class where
sasl_oauth_token_provider
should be configured. I am open to better ideas of where to store the required interface details!Example:
Tested with a local Kafka cluster and ID Token, and successfully authenticated with the example client initialization above.
This change is