Re: [PATCH v4 1/2] git-imap-send: Add CRAM-MD5 authenticate method support

From: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Date: Feb 13, 2010, 06:49 UTC
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:

Show 44 quoted lines

> Hitoshi Mitake<mitake@dcl.info.waseda.ac.jp>  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<kusmabite@googlemail.com>
>> Cc: Jakub Narebski<jnareb@gmail.com>
>> Cc: Linus Torvalds<torvalds@linux-foundation.org>
>> Cc: Jeff King<peff@peff.net>
>> Signed-off-by: Hitoshi Mitake<mitake@dcl.info.waseda.ac.jp>
>> ---
>
> 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<your@address.example.com>
> 	---
>
> 	 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.

Show 11 quoted lines

>> +
>> +	/* 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.

Show 6 quoted lines

>> +	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.

Show 5 quoted lines

>> +	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.

Show 5 quoted lines

>
> 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.

Show 16 quoted lines

>
>> +	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: "<user>  <digest in hex>" */
>> +	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!