added gssapi support (Kerberos) for SASL #1152
Conversation
kafka/conn.py
Outdated
| @@ -21,6 +21,7 @@ | |||
| from kafka.protocol.metadata import MetadataRequest | |||
| from kafka.protocol.types import Int32 | |||
| from kafka.version import __version__ | |||
| from mechanize import _response | |||
There was a problem hiding this comment.
No, i have not willingly added this line
|
the tests fails for the unknown GSSError type, which comes with the gssapi module and is added with the line edit: |
kafka/conn.py
Outdated
| except ImportError: | ||
| #no gssapi available, will disable gssapi mechanism | ||
| disable_gssapi = True | ||
| class GSSError(Exception): |
There was a problem hiding this comment.
let's drop this class back one indent level and not leave it undefined if the gssapi import succeeds
There was a problem hiding this comment.
looking at this again, I might do something like
try:
from gssapi.raw.misc import GSSError
except ImportError:
GSSError = None
| try: | ||
| self._sock.setblocking(True) | ||
| # Send output token | ||
| msg = output_token |
There was a problem hiding this comment.
will this be bytes on python3 ? if not, should encode('utf-8')
There was a problem hiding this comment.
it is a binary token that is exchanged. I'll check the type for python3, but i think encoding will harm the authentication token.
There was a problem hiding this comment.
return type is bytes
| #calculate the output token | ||
| try: | ||
| output_token = ctx_Context.step(received_token) | ||
| except GSSError as e: |
There was a problem hiding this comment.
won't this raise a NameError unless GSSError has been imported directly?
There was a problem hiding this comment.
Alternately I'd be ok with except Exception as e here
There was a problem hiding this comment.
To avoid the NameError i added the lines below in case of ImportError
class GSSError(Exception):
pass
kafka/conn.py
Outdated
| @@ -167,9 +182,13 @@ class BrokerConnection(object): | |||
| 'metric_group_prefix': '', | |||
| 'sasl_mechanism': 'PLAIN', | |||
| 'sasl_plain_username': None, | |||
| 'sasl_plain_password': None | |||
| 'sasl_plain_password': None, | |||
| 'sasl_servicename_kafka':'kafka' | |||
There was a problem hiding this comment.
is this the same as sasl.kerberos.service.name in the java client? If so, let's reuse that config name (but with underscores)
There was a problem hiding this comment.
Yes, this is exactly the same as sasl.kerberos.service.name in the java client, i'll change the name accordingly
kafka/conn.py
Outdated
| disable_gssapi = False | ||
| try: | ||
| import gssapi | ||
| from gssapi import raw |
There was a problem hiding this comment.
is raw used? I dont see it in the changes below
There was a problem hiding this comment.
not raw, but it solved the name resolution error of GSSError for me.
kafka/conn.py
Outdated
| @@ -54,6 +54,21 @@ class SSLWantReadError(Exception): | |||
| class SSLWantWriteError(Exception): | |||
| pass | |||
|
|
|||
| # needed for SASL_GSSAPI authentication: | |||
| disable_gssapi = False | |||
There was a problem hiding this comment.
mild preference for just setting gssapi = None on ImportError. See kafka/codec.py
There was a problem hiding this comment.
OK for me, i'll change it
kafka/conn.py
Outdated
| } | ||
| SASL_MECHANISMS = ('PLAIN',) | ||
| if gssapi is None: | ||
| SASL_MECHANISMS = ('PLAIN') |
There was a problem hiding this comment.
this needs a trailing comma for tuple-ization
kafka/conn.py
Outdated
| #class GSSError(Exception): | ||
| # pass | ||
|
|
||
| try: |
There was a problem hiding this comment.
cool. you could also combine w/ the above import try. I think we can safely assume that either both imports work, or both will fail.
|
I am a little lost on the issues with the travis ci test? |
|
Test failure looks unrelated -- we have sporadic fixture issues on travis that I haven't been able to figure out. For now I typically just restart the failed test if it appears related to a kafka broker not starting up properly. I'm sure these could be improved, but it's out of scope for this PR. |
|
This is great! Thanks for the PR. I'm sure a lot of folks will appreciate the new feature. |
|
Thank you |
this is the first version, please have a look on it