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?