[Dovecot] [PATCH] SCRAM-SHA-1 authentication
Hy,
over the last days I have implemented SCRAM-SHA-1 in Dovecot's 2.1 branch. It does not do SCRAM-SHA-1-PLUS, but should be extendable enough to introduce it later.
There are some checks for the message format which (assuming the client acts correclty) are not strictly necessary during parsing. This is partially in the hope that it might aid client implementers, partially because it (IMHO) improves readability when checking against the RFC.
Also errors found in this way could be sent to the client, this is however strictly OPTIONAL in the RFC, for now they are just logged.
Some of the variable names are rather long. This is in order to have them match the terms introduced in the RFC, again I expect it to help readability (maybe my recent Objective-C programming showing though).
I do feel somewhat insecure about my usage of some lib functions. Hopefully no API has been abused too much.
I also note that there are a lot of fields in the scram_auth_request struct. I think they are all there for a reason, however feel free to prove me wrong.
Attached is a hg export. It also includes a hmac-sha1 implementation, an adaption off of the hmac-md5 implementation already in Dovecot. I guess those should eventually be merged into a hash-independent hmac implementation, but I figured this would have to do for now.
The implementation has been tested against GNU SASL and does appear to
work fine. (The command line was gsasl -m SCRAM-SHA-1 -a user -p pass --imap host for those curious)
Regards Florian "Florob" Zeitz
On Fri, 2011-09-16 at 02:47 +0200, Florian Zeitz wrote:
over the last days I have implemented SCRAM-SHA-1 in Dovecot's 2.1 branch. It does not do SCRAM-SHA-1-PLUS, but should be extendable enough to introduce it later.
Looks pretty good. Below are a few things I noticed. I could fix these myself next week also, or you can do them during weekend if you want to. :)
I also note that there are a lot of fields in the scram_auth_request struct. I think they are all there for a reason, however feel free to prove me wrong.
The username wouldn't necessarily have to be there. Also its name was confusing me for a while since I thought you were setting auth_request->user directly.
- snonce[i] = (snonce[i] % ('~' - '!')) + '!';
- if (snonce[i] == ',')
- snonce[i] = '.';
Here '~' is actually never used. But a nice solution would be to simply replace ',' with '~' so '.' isn't more likely to occur than others.
- fields = t_strsplit((const char*)data, ",");
Not safe. data isn't guaranteed to be NUL-terminated. One simple solution would be: t_strsplit(t_strndup(data, size), ",")
And others:
Could be nicer if client->proof was stored base64-decoded, so its validity could be checked and also later there wouldn't be need to base64-encode signature when testing it.
There's no log message is authentication fails due to wrong password?
Doesn't verify_credentials() need to check the credentials in any way that it contains expected (sized) data? Anything is allowed?
Am 16.09.2011 14:40, schrieb Timo Sirainen:
On Fri, 2011-09-16 at 02:47 +0200, Florian Zeitz wrote:
Looks pretty good. Below are a few things I noticed. I could fix these myself next week also, or you can do them during weekend if you want to. :)
I decided to do it myself, hope this fixes all issues.
Could be nicer if client->proof was stored base64-decoded, so its validity could be checked and also later there wouldn't be need to base64-encode signature when testing it.
Doesn't verify_credentials() need to check the credentials in any way that it contains expected (sized) data? Anything is allowed?
I don't think it needs to. The password read from the database can legitimately have any length and from the client it just takes a base64 encoded SHA-1 hash. The correct size of that was previously implicitly checked when comparing the base64 encoded data (strings of different length don't compare equal). It's now explicitly checked after base64 decoding the client proof.
On Sun, 2011-09-18 at 03:44 +0200, Florian Zeitz wrote:
Am 16.09.2011 14:40, schrieb Timo Sirainen:
On Fri, 2011-09-16 at 02:47 +0200, Florian Zeitz wrote:
Looks pretty good. Below are a few things I noticed. I could fix these myself next week also, or you can do them during weekend if you want to. :)
I decided to do it myself, hope this fixes all issues.
Committed finally to http://hg.dovecot.org/dovecot-2.1 with some cleanups.
On 11/23/2011 9:57 PM, Timo Sirainen wrote:
On Sun, 2011-09-18 at 03:44 +0200, Florian Zeitz wrote:
Am 16.09.2011 14:40, schrieb Timo Sirainen:
On Fri, 2011-09-16 at 02:47 +0200, Florian Zeitz wrote:
Looks pretty good. Below are a few things I noticed. I could fix these myself next week also, or you can do them during weekend if you want to. :)
I decided to do it myself, hope this fixes all issues. Committed finally to http://hg.dovecot.org/dovecot-2.1 with some cleanups.
Great, the Pigeonhole ManageSieve implementation is now suddenly fully RFC compliant, without any effort on my part! :)
Regards,
Stephan.
On Wed, 2011-11-23 at 22:07 +0100, Stephan Bosch wrote:
On 11/23/2011 9:57 PM, Timo Sirainen wrote:
On Sun, 2011-09-18 at 03:44 +0200, Florian Zeitz wrote:
Am 16.09.2011 14:40, schrieb Timo Sirainen:
On Fri, 2011-09-16 at 02:47 +0200, Florian Zeitz wrote:
Looks pretty good. Below are a few things I noticed. I could fix these myself next week also, or you can do them during weekend if you want to. :)
I decided to do it myself, hope this fixes all issues. Committed finally to http://hg.dovecot.org/dovecot-2.1 with some cleanups.
Great, the Pigeonhole ManageSieve implementation is now suddenly fully RFC compliant, without any effort on my part! :)
I'm not actually sure about that :) The final replying is probably wrong, since ManageSieve supports sending it to client, unlike IMAP/POP3..
participants (3)
-
Florian Zeitz
-
Stephan Bosch
-
Timo Sirainen