From: Hitoshi Mitake Date: Sat, 13 Feb 2010 06:49:11 GMT Subject: Re: [PATCH v4 1/2] git-imap-send: Add CRAM-MD5 authenticate method support Message-ID: <4B764B67.1020402@dcl.info.waseda.ac.jp> In-Reply-To: <7vljeyp1rj.fsf@alter.siamese.dyndns.org> On 2010年02月13日 06:44, Junio C Hamano wrote: > Hitoshi Mitake writes: > >> This patch makes git-imap-send CRAM-MD5 authenticate method ready. >> In theory, CRAM-MD5 authenticate method doesn't depend on SSL. >> But for easiness of maintainance, this patch uses base64 and md5 stuffs of OpenSSL. >> So if you want to use CRAM-MD5 authenticate method, >> you have to build git-imap-send with OpenSSL library. >> >> --- >> v3: Erik noticed that there were some garbage lines in this patch. >> I removed these. And 2/2 wasn't changed, I'm sending 1/2 only. >> >> v4: Based on Junio's indication, I cleaned up some points of imap-send.c . >> >> Cc: Erik Faye-Lund >> Cc: Jakub Narebski >> Cc: Linus Torvalds >> Cc: Jeff King >> Signed-off-by: Hitoshi Mitake >> --- > > I actually meant that the comment regarding the history of patch > iterations should go after the three-dash. Most notably, we want your > S-o-b in the commit log. That is: > > ---------------------------------------------------------------- > Subject: well thought out summary of what the patch is about > > Problem description, and rationale to justify the particular > solution you chose. > > Signed-off-by: Your Name > --- > > Comments that may help the reviewers while the patch is > going through the review cycle, but would not be useful > after this particular version is applied and the commit > is shown in "git log" output > > diffstat > > diff --git ... patch text ... > ---------------------------------------------------------------- > I didn't know that Git has such a convenient feature. I'll move version specific comments to below of three-dash line. >> + >> + /* challenge must be shorter than challenge_64 >> + * because we are decoding base64*/ > > Just a style thing but > > /* > * We prefer to write our multi-line > * comments like this. > */ > OK, I'll modify this comment. >> + challenge = xcalloc(strlen(challenge_64) + 1, sizeof(char)); > > Why not xmalloc()? Does EVP_DecodeBlock() want a zero-filled buffer? > Because strlen(challenge_64) is the upper limit of length of challenge. So tail part of challenge may not be filled by EVP_DecodeBlock(), non-zero filled buffer produces not NULL terminated string. I've confused once by this problem before. >> + EVP_DecodeBlock((unsigned char *)challenge, (unsigned char *)challenge_64, strlen(challenge_64)); > > Does EVP_DecodeBlock diagnose an error in the input and return an error > code? How are you supposed to protect yourself from the server giving you > a corrupt challenge that does not decode properly? > I've forgot processing return value from EVP_DecodeBlock(). I'll fix it. >> + HMAC_Init(&hmac, (unsigned char *)pass, strlen(pass), EVP_md5()); >> + HMAC_Update(&hmac, (unsigned char *)challenge, strlen(challenge)); >> + HMAC_Final(&hmac, hash, NULL); > > Is there any clean-up necessary after you are done with hmac? I've forgot calling HMAC_CTX_cleanup(). > EVP_md5() > returns a pointer to EVP_MD but how and when is that resource released? prototype of EVP_md5() is const EVP_MD *EVP_md5(void); EVP_md5() only returns const value. There is no new resource allocation. e.g. EVP_md() == EVP_md() is true. > > By the way, HMAC_Init() seems to be deprecated and kept only for 0.9.6b > compatibility. > > http://www.openssl.org/docs/crypto/hmac.html The document of OpenSSL doesn't describe HMAC_init_ex() well. I can't know that what the parameter ENGINE *impl means... So I'd like to use HMAC_Init(), if it is for compatibility. > >> + hex[32] = 0; >> + for (i = 0; i< 16; i++) { >> + hex[2 * i] = hexchar((hash[i]>> 4)& 0xf); >> + hex[2 * i + 1] = hexchar(hash[i]& 0xf); >> + } >> + >> + /* length: " " */ >> + resp_len = strlen(user) + 1 + strlen(hex) + 1; >> + response = xcalloc(resp_len, sizeof(char)); >> + snprintf(response, resp_len, "%s %s", user, hex); >> + >> + response_64 = xcalloc(ENCODED_SIZE(resp_len), sizeof(char)); > > Why not xmalloc()? Does EVP_EncodeBlock() want a zero-filled buffer? > The reason using xcalloc() here is like one of above. ENCODED_SIZE() calculates upper limit of encoded size. >> + EVP_EncodeBlock((unsigned char *)response_64, (unsigned char *)response, strlen(response)); > > I wouldn't worry too much about error response from this function as I > would for EVP_DecodeBlock() I mentioned earlier. I'll modify it, too. > > By the way, I made a couple of small fix-ups to your [2/2] (I think they > were just style and unnecessary use of xcalloc()) and queued. Thanks. Where can I find them? # But as I mentioned above, use of xcalloc() is necessary. And again, thanks for your very detailed review!