-
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
Enable SCRAM-SHA-256 and SCRAM-SHA-512 for sasl #1918
Conversation
Documentation is still missing, I'll add something on Friday. |
@dpkp @jeffwidman ready to be reviewed now. |
@dpkp @jeffwidman We are extremely interested in this as well. Could you please take a look? |
Would be great to have this feature. Is there a timeframe for when we can merge it? Thanks! |
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.
Thanks so much for this PR and the excellent work on test fixtures!
@@ -25,7 +25,7 @@ addons: | |||
cache: | |||
directories: | |||
- $HOME/.cache/pip | |||
- servers/ | |||
- servers/dist |
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.
good catch!
return found | ||
|
||
return None | ||
return found |
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.
drive-by!
pass | ||
|
||
logging.getLogger(__name__).addHandler(NullHandler()) | ||
logging.basicConfig(level=logging.INFO) |
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.
What's your thinking on the logging changes here?
@@ -98,6 +107,69 @@ class ConnectionStates(object): | |||
AUTHENTICATING = '<authenticating>' | |||
|
|||
|
|||
class ScramClient: |
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.
Thoughts on moving this to its own file ?
close = False | ||
else: | ||
try: | ||
client_first = scram_client.first_message().encode() |
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.
should we specify an encoding explicitly? is 'utf-8' always the default? does it matter?
self._send_bytes_blocking(size + client_first) | ||
|
||
(data_len,) = struct.unpack('>i', self._recv_bytes_blocking(4)) | ||
server_first = self._recv_bytes_blocking(data_len).decode() |
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.
similarly re: codec. would prefer to specific 'utf-8' explicitly
server_first = self._recv_bytes_blocking(data_len).decode() | ||
scram_client.process_server_first_message(server_first) | ||
|
||
client_final = scram_client.final_message().encode() |
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.
and here - utf-8
self._send_bytes_blocking(size + client_final) | ||
|
||
(data_len,) = struct.unpack('>i', self._recv_bytes_blocking(4)) | ||
server_final = self._recv_bytes_blocking(data_len).decode() |
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.
and finally also here
if sasl_mechanism is not None: | ||
self.sasl_mechanism = sasl_mechanism.upper() | ||
else: | ||
self.sasl_mechanism = None |
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.
the else clause seems redundant?
# wait and try again | ||
# on travis the brokers sometimes take a while to find themselves | ||
time.sleep(0.5) | ||
self._create_topic_via_admin_api(topic_name, num_partitions, replication_factor, timeout_ms) |
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.
might prefer an outer loop to enable as many retries as needed until timeout
@dpkp Hello! Will this be released soon? |
@dpkp @jeffwidman Any update? |
Alternatively, can I please do something to help? |
Hey @ofek I don't have the rights to cut a new release, but I just pinged @dpkp and he said on IM that he's currently traveling w/o his laptop for the next week so can't look at it until he returns. Sorry about that. If you weren't shipping to end-users and just using internally then it'd be trivial to pin to a specific commit hash rather than needing a release... but alas, you are shipping to end users. |
No worries, thanks! |
Hello again! Any update? |
I'm also waiting on this feature, @dpkp can you give us an update on the expected timeline? |
looking forward for that too ! |
@terence-bigtt This is released. |
closes #1882
This change is