-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for SCRAM-SHA-* #5
base: main
Are you sure you want to change the base?
Conversation
No -PLUS yet, no support for channel binding. Also supports SCRAM-* for any OpenSSL-supported digest function with just one line of code in the future.
Thanks! This is great! I haven't really given this project the attention I'd hoped. But I'm glad someone else sees the value in it. I'll take a closer look at all three PRs next week! |
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.
FYI, I merged most of your changes with a bunch of updates of my own in some ruby/net-imap branches. I've pushed one PR but I'll send in some others that correspond to all of your other PRs.
Adding in a pure-ruby saslprep has been the main thing holding me back from finishing all this. The other big thing holding me back is that I have some fundamental (but backwards-compatible) API changes I needed to make, so the SASL gem can more generally useful than the embedded net/imap API: standardizing properties across all mechanisms, and property callbacks. I wanted to finish than before I could merge your PRs. Actually, I've used your PRs to help guide the design choices.
Once I'm happy with the net-imap PRs, I'll merge all of the changes back into net-sasl. I'll keep your PRs open until then. :) Thanks!
@@ -28,5 +28,6 @@ Gem::Specification.new do |spec| | |||
spec.require_paths = ["lib"] | |||
|
|||
spec.add_dependency "digest" | |||
spec.add_dependency "idn-ruby" |
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.
I'd prefer a pure-ruby saslprep, to avoid dependencies. I'm hoping to replace the SASL implementations in net/imap and net/pop and net/smtp, which are all bundled gems. Adding another dependency might prevent that from happening; especially a C dependency, which isn't ideal for for JRuby and TruffleRuby.
My plan is to allow a swap-able saslprep library, for performance. But that will need benchmarks.
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.
FWIW, I wrote this up a while back, but never submitted the review! Oops. I recently finished a pure ruby SASLprep (I want some more test-cases before I'm happy with it), which generates Regexps directly from the stringprep RFC's tables.
I think I'll have time to address all of these comments before the end of the week!
|
||
# responds to the server's challenges | ||
def process(challenge) | ||
return "n,#{'a=' + @authzid if @authzid},#{initial_message}" if challenge.nil? |
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.
We can't rely on nil
, because not every protocol supports initial response. E.g. IMAP4rev1
without the SASL-IR
extension. Per the SCRAM RFC, it should just be an empty string in that case. But it's still better to just explicitly track the state, IMO. The first time the method is called, challenge
will be ignored and the initial response will be sent.
|
||
bare = "c=biws,r=#{sparams['r']}" | ||
salted_password = OpenSSL::KDF.pbkdf2_hmac( | ||
IDN::Stringprep.with_profile(@password.encode("utf-8"), "SASLprep"), |
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.
I have a pure ruby stdlib implementation of SASLprep in mind for this but it isn't ready yet. nearly done, but not quite. The RFC does give us an official way to cheat: only allow ASCII strings. ;) So this could just guard raise unless password.ascii_only?
and I'd replace it with my Net::SASL::SASLprep.saslprep
when it's done.
protected | ||
|
||
def initial_message | ||
"n=#{@username},r=#{@cnonce}" |
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.
we should do some error checking or formatting on these values, here or in #initialize? eg "\0" or "," chars.
add_authenticator "SCRAM-SHA-224", ScramAuthenticator.for("SHA224") | ||
add_authenticator "SCRAM-SHA-256", ScramAuthenticator.for("SHA256") | ||
add_authenticator "SCRAM-SHA-384", ScramAuthenticator.for("SHA384") | ||
add_authenticator "SCRAM-SHA-512", ScramAuthenticator.for("SHA512") |
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.
I like that it only takes one line of code. But, even though plugging in alternate hashes is fairly straightforward and predictable, I'd prefer to only include official SASL mechanisms in the default set. I think that the IETF drafts for 512 (or at least, the ones that I knew about) recently expired, although I haven't researched why.
For any that are left out, we can include documentation so that users can add it themselves, if they really need it sooner.
# This should generally be instantiated via Net::SASL.authenticator. | ||
def initialize(username, password, authzid = nil, hash:, **options) | ||
super | ||
@hash = OpenSSL::Digest.new(hash) |
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.
I think I'd prefer to define hash on the subclasses. It loses the nice one liner for new SCRAM-*
mechanisms, but it'll be simpler and only like... three lines.
class SCRAMAuthenticator
def hash; @hash ||= OpenSSL::Digest.new(self.class::HASH) end
end
# Defined in RFC5802, etc etc etc
class SCRAM256Authenticator < SCRAMAuthenticator
HASH = "SHA256"
end
The one-liner could be recreated with only a little bit of extra work. But I'm not sure it's worth it, especially because I prefer an explicitly named class constant to which we can add rdoc for each specific mechanism.
Based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
I know it's taken me forever to finish this, but ruby/net-imap#172 should be merged shortly. After I still need to fix
|
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
Loosely based on the implementation by @singpolyma at nevans/net-sasl#5 New authenticators for any digest algorithms supported can be added by subclassing ScramAuthenticator and adding a DIGEST_NAME constant (and then registering with an Authenticators registry). Co-authored-by: Stephen Paul Weber <[email protected]>
@nevans: Good job for your work in net-imap!
But it is not possible to have an official net-sasl? cc: @singpolyma. |
I'll update this gem with the latest |
That's the goal, but it would add a new bundled gem to ruby. I wanted to release the basic implementation in the context of one project and then port it to at least one other before I seriously advance that proposal. I've actually been using a fork of |
No -PLUS yet, no support for channel binding.
Also supports SCRAM-* for any OpenSSL-supported digest function with just one
line of code in the future.