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?
