Hello Steve,

> 1) I would recommend splitting your change set up into three separate
> patches. I found this a bit strange when I joined curl but it allows for
> easier track-ability of changes and reverting them if we accidentally break
> something. As such I would recommend the following patches:
>
>
OK, I will do. Next time, I will divide long patches into logical units.

> 2) Is there any reason why AUTH_DIGEST_MD5 appears before AUTH_CRAM? The
> code isn't at fault... I just wondered what the rational for this was and
> what would a server prefer if it supported both? Would the server send "250
> AUTH DIGEST-MD5 CRAM-MD5" or "250 AUTH CRAM-MD5 DIGEST-MD5". Obviously I'm
> hoping to change this when I make the modification to curl to allow it to
> honour the server's preferred order of mechanisms but in the meantime I
> have
> tried to order the mechanisms based on priority / security level but I
> honestly don't know how it compares to cram.
>

The same reason CRAM-MD5 is selected before in the current code. According
to RFC 2831, DIGEST-MD5 is more secure than CRAM-MD5 as it prevents "chosen
plaintext attacks".

>
> 3) A few general style issues:
>
>
I agree with them and will change them later today.

>
> 4) Other changes that I think we should consider:
>
>
Agreed too.

> If you fancy doing both of these I would recommend that it is done in your
> second patch (as mentioned earlier) as a "smtp: Pre digest-md5 tidy up"
> patch if you like ;-)
>
>
Second patch sounds nice :-)

> Sorry if some of that seems petty or ruthless - that's not my intention.
>
>
Not a chance, thanks for the review and tips. Greatly appreciated!!

Cheers

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2012-01-10