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

Support SASL OAuthBearer Authentication #1750

Merged
merged 5 commits into from
Mar 23, 2019
Merged

Support SASL OAuthBearer Authentication #1750

merged 5 commits into from
Mar 23, 2019

Conversation

pt2pham
Copy link
Contributor

@pt2pham pt2pham commented Mar 20, 2019

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:

class TokenProvider(object):
    def token(self):
        return "some_token_id"

producer = KafkaProducer(
    bootstrap_servers='127.0.0.1:9094',
    ...
    security_protocol='SASL_PLAINTEXT',
    sasl_oauth_token_provider=TokenProvider(),
    sasl_mechanism='OAUTHBEARER'
)

Tested with a local Kafka cluster and ID Token, and successfully authenticated with the example client initialization above.


This change is Reviewable

Copy link
Owner

@dpkp dpkp left a 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())
Copy link
Owner

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'])

Copy link
Contributor Author

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.

kafka/admin/client.py Outdated Show resolved Hide resolved
import abc

# This statement is compatible with both Python 2.7 & 3+
ABC = abc.ABCMeta('ABC', (object,), {'__slots__': ()})
Copy link
Owner

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

kafka/oauth/abstract.py Outdated Show resolved Hide resolved
kafka/oauth/abstract.py Outdated Show resolved Hide resolved
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
Copy link
Owner

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?

Copy link
Contributor Author

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.

kafka/conn.py Show resolved Hide resolved
kafka/oauth/abstract.py Outdated Show resolved Hide resolved
kafka/conn.py Outdated Show resolved Hide resolved
@dpkp dpkp merged commit 8e2ed3e into dpkp:master Mar 23, 2019
@dpkp
Copy link
Owner

dpkp commented Mar 23, 2019

Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants