search

racket / sasl

Other
2 stars 9 forks source link

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

Closed Bogdanp closed 1 year ago

Bogdanp commented 1 year ago

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.

rmculpepper commented 1 year ago

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 commented 1 year ago

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 commented 8 months ago

@Bogdanp: Good job!

Linked to: