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

scram: fix handling of bytes responses, add support for sha512 digests #4

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

Bogdanp
Copy link
Sponsor Contributor

@Bogdanp Bogdanp commented Dec 2, 2022

SCRAM-SHA-512 isn't standardized, but it is used in the wild (eg. in Apache Kafka).
I don't think there's much downside to allowing the extra digest.

The string change is a fix to make scram correctly support sasl-receive-message's contract, which is (or/c bytes? string?), whereas before this change it would fail if given a bytes? message.

`sasl-receive-message` accepts `(or/c bytes? string?)`, but the scram
implementation wasn't handling the `bytes?` case correctly.
@rmculpepper
Copy link
Contributor

The SHA-512 support looks good.

My intention with the (or/c string? bytes?) contract was that each mechanism defines whether it represents messages as strings or bytes, and you must give it the appropriate type. That is, (or/c string? bytes?) is a superset of the allowed values. But IIRC, I didn't document that very well, and I didn't add explicit dynamic checks, and I'm not sure there are any non-string-based mechanisms anyway. So I'm fine with changing it.

@Bogdanp
Copy link
Sponsor Contributor Author

Bogdanp commented Dec 5, 2022

My intention with the (or/c string? bytes?) contract was that each mechanism defines whether it represents messages as strings or bytes, and you must give it the appropriate type. That is, (or/c string? bytes?) is a superset of the allowed values. But IIRC, I didn't document that very well, and I didn't add explicit dynamic checks, and I'm not sure there are any non-string-based mechanisms anyway. So I'm fine with changing it.

Thanks for the explanation! That hadn't occurred to me. I know you've already agreed to change it, but just to give a concrete example of why I think it's worth doing: in my Kafka client, I allow the user to pass in any SASL context and step through the state machine on their behalf. So, they might pass in a context type my library doesn't know about, in which case my library wouldn't know what type to pass in. I could force the user to do the stepping themselves, but that seems worse than having this library behave more generically.

Looking at the other modules, sasl/cram-md5 already accepts both bytes and strings, sasl/plain and sasl/saslprep don't receive any messages, so it's fine for their contracts to be more limited. So, I think this change to sasl/scram is enough to make everything work a little more generically.

@Neustradamus
Copy link

@Bogdanp: Good job!

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.

None yet

3 participants