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

Enable SCRAM-SHA-256 and SCRAM-SHA-512 for sasl #1918

Merged
merged 46 commits into from
Dec 29, 2019
Merged

Enable SCRAM-SHA-256 and SCRAM-SHA-512 for sasl #1918

merged 46 commits into from
Dec 29, 2019

Conversation

swenzel
Copy link
Contributor

@swenzel swenzel commented Oct 2, 2019

closes #1882


This change is Reviewable

@swenzel
Copy link
Contributor Author

swenzel commented Oct 2, 2019

Documentation is still missing, I'll add something on Friday.

Swen Wenzel added 29 commits October 16, 2019 15:10
@swenzel
Copy link
Contributor Author

swenzel commented Oct 17, 2019

@dpkp @jeffwidman ready to be reviewed now.
That one test that is still failing seems to only fail some of the times. My best guess is that the broker itself hasn't completely fetched the data yet and that a small sleep might fix this.
But I think I have already fixed more than I should 😅
Looking forward to reading your feedback :)

@ofek
Copy link
Contributor

ofek commented Nov 25, 2019

@dpkp @jeffwidman We are extremely interested in this as well. Could you please take a look?

@israelglar
Copy link

Would be great to have this feature. Is there a timeframe for when we can merge it?

Thanks!

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.

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
Copy link
Owner

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
Copy link
Owner

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)
Copy link
Owner

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:
Copy link
Owner

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()
Copy link
Owner

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()
Copy link
Owner

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()
Copy link
Owner

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()
Copy link
Owner

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
Copy link
Owner

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)
Copy link
Owner

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

This was referenced Dec 29, 2019
@dpkp dpkp merged commit ee1c4a4 into dpkp:master Dec 29, 2019
@ofek
Copy link
Contributor

ofek commented Jan 9, 2020

@dpkp Hello! Will this be released soon?

@ofek
Copy link
Contributor

ofek commented Jan 16, 2020

@dpkp @jeffwidman Any update?

@ofek
Copy link
Contributor

ofek commented Jan 16, 2020

Alternatively, can I please do something to help?

@jeffwidman
Copy link
Collaborator

jeffwidman commented Jan 16, 2020

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.

@ofek
Copy link
Contributor

ofek commented Jan 16, 2020

No worries, thanks!

@ofek
Copy link
Contributor

ofek commented Jan 28, 2020

Hello again! Any update?

@jfpatenaude
Copy link

I'm also waiting on this feature, @dpkp can you give us an update on the expected timeline?

@swenzel swenzel deleted the enable-sasl branch March 2, 2020 15:18
@terence-bigtt
Copy link

looking forward for that too !

@ofek
Copy link
Contributor

ofek commented May 19, 2020

@terence-bigtt This is released.

@Neustradamus
Copy link

@swenzel, @dpkp: Thanks a lot for all about SCRAM.

Can you look, a guy has found a bug?

Linked to:

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.

Support for sasl mechanism: SCRAM-SHA-256
8 participants