[Dovecot] [PATCH] Add SCRAM-SHA-1 password scheme
Hello,
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Ideally I'd want doveadm pw's rounds flag to apply to this, but that's currently specific to the crypt password scheme, so I left it out for now.
Regards, Florian Zeitz
On 3.10.2012, at 0.05, Florian Zeitz wrote:
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Ideally I'd want doveadm pw's rounds flag to apply to this, but that's currently specific to the crypt password scheme, so I left it out for now.
Looks pretty good. But you could improve the error handling a bit. Instead of atoi() use str_to_uint() and verify the error value. Also verify that t_strsplit() returns the correct number of values. And there should be some sanity check for the iter count also.. I'm not sure what, but currently it's possible for Hi() to go to infinite loop.
Am 02.10.2012 23:27, schrieb Timo Sirainen:
On 3.10.2012, at 0.05, Florian Zeitz wrote:
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Ideally I'd want doveadm pw's rounds flag to apply to this, but that's currently specific to the crypt password scheme, so I left it out for now.
Looks pretty good. But you could improve the error handling a bit. Instead of atoi() use str_to_uint() and verify the error value. Also verify that t_strsplit() returns the correct number of values. And there should be some sanity check for the iter count also.. I'm not sure what, but currently it's possible for Hi() to go to infinite loop.
I shall. For the iteration count the endless loop should be fixed by restricting the largest value to UINT_MAX-1, right? I'm not too fond of stopping people from wasting their CPU time on Hi calculation beyond this. I can try to guestimate a "sane" upper limit, but given time I have an icky feeling that it will end up being too low. Thoughts?
On 3.10.2012, at 1.12, Florian Zeitz wrote:
Am 02.10.2012 23:27, schrieb Timo Sirainen:
On 3.10.2012, at 0.05, Florian Zeitz wrote:
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Ideally I'd want doveadm pw's rounds flag to apply to this, but that's currently specific to the crypt password scheme, so I left it out for now.
Looks pretty good. But you could improve the error handling a bit. Instead of atoi() use str_to_uint() and verify the error value. Also verify that t_strsplit() returns the correct number of values. And there should be some sanity check for the iter count also.. I'm not sure what, but currently it's possible for Hi() to go to infinite loop.
I shall. For the iteration count the endless loop should be fixed by restricting the largest value to UINT_MAX-1, right?
Yeah.
I'm not too fond of stopping people from wasting their CPU time on Hi calculation beyond this. I can try to guestimate a "sane" upper limit, but given time I have an icky feeling that it will end up being too low. Thoughts?
Looks like RFC 5802 doesn't give any kind of a limit. But since it gets sent to various client implementations, INT_MAX is probably a good limit? Also 0 isn't a valid iteration count.
On 3.10.2012, at 0.05, Florian Zeitz wrote:
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Oh, and SCRAM-SHA1 or SCRAM-SHA-1? I'd think SCRAM-SHA1 as the scheme is now called, but elsewhere in the code (including user-visible strings) it says SCRAM-SHA-1.
Am 03.10.2012 01:42, schrieb Timo Sirainen:
On 3.10.2012, at 0.05, Florian Zeitz wrote:
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Oh, and SCRAM-SHA1 or SCRAM-SHA-1? I'd think SCRAM-SHA1 as the scheme is now called, but elsewhere in the code (including user-visible strings) it says SCRAM-SHA-1.
Well, I usually prefer SCRAM-SHA-1, as that is how it is called in the RFC, and SHA-1 is the hash name registered with IANA [1]. I did call the password scheme SCRAM-SHA1 to be consistent with other current password schemes. I'm not 100% sure which one to use, or whether a mix might even be the way to go ("correct" messages, but minimum user confusion for password schemes).
[1] https://www.iana.org/assignments/hash-function-text-names/hash-function-text...
On 3.10.2012, at 2.54, Florian Zeitz wrote:
Am 03.10.2012 01:42, schrieb Timo Sirainen:
On 3.10.2012, at 0.05, Florian Zeitz wrote:
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Oh, and SCRAM-SHA1 or SCRAM-SHA-1? I'd think SCRAM-SHA1 as the scheme is now called, but elsewhere in the code (including user-visible strings) it says SCRAM-SHA-1.
Well, I usually prefer SCRAM-SHA-1, as that is how it is called in the RFC, and SHA-1 is the hash name registered with IANA [1]. I did call the password scheme SCRAM-SHA1 to be consistent with other current password schemes. I'm not 100% sure which one to use, or whether a mix might even be the way to go ("correct" messages, but minimum user confusion for password schemes).
Hmm. Probably not worth it to have both SCRAM-SHA1 and SCRAM-SHA-1. And now I see that the user-visible strings are about SCRAM-SHA-1 mechanism, not the hash. So yeah, I guess the best way to avoid confusion is to call it SCRAM-SHA-1 everywhere.
Am 03.10.2012 01:58, schrieb Timo Sirainen:
On 3.10.2012, at 2.54, Florian Zeitz wrote:
Am 03.10.2012 01:42, schrieb Timo Sirainen:
On 3.10.2012, at 0.05, Florian Zeitz wrote:
attached is an hg export on top of the current dovecot-2.2 branch, which adds support for a SCRAM-SHA-1 password scheme.
Oh, and SCRAM-SHA1 or SCRAM-SHA-1? I'd think SCRAM-SHA1 as the scheme is now called, but elsewhere in the code (including user-visible strings) it says SCRAM-SHA-1.
Well, I usually prefer SCRAM-SHA-1, as that is how it is called in the RFC, and SHA-1 is the hash name registered with IANA [1]. I did call the password scheme SCRAM-SHA1 to be consistent with other current password schemes. I'm not 100% sure which one to use, or whether a mix might even be the way to go ("correct" messages, but minimum user confusion for password schemes).
Hmm. Probably not worth it to have both SCRAM-SHA1 and SCRAM-SHA-1. And now I see that the user-visible strings are about SCRAM-SHA-1 mechanism, not the hash. So yeah, I guess the best way to avoid confusion is to call it SCRAM-SHA-1 everywhere.
Seems sensible.
Attached is a new export incorporating your feedback. The iteration count is now limited to [4096, INT_MAX]. The lower bound is a recommendation of the RFC.
Am 03.10.2012 02:50, schrieb Timo Sirainen:
On Wed, 2012-10-03 at 02:10 +0200, Florian Zeitz wrote:
Attached is a new export incorporating your feedback.
Committed. Also what do you think about the attached patch? (Compiles, untested.)
Moving the passdb parsing into a separate function seems like a nice idea to me. Style changes and removing an unused variable is obviously fine (I'm a bit surprised I got no compiler warning about the latter, but oh well).
I did a quick test. Login and error checking seem to still work fine with this patch in place. Wouldn't have seen anything in the code to suggest otherwise either.
participants (2)
-
Florian Zeitz
-
Timo Sirainen