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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 12, 2010, 21:44 UTC
Message-ID: <7vljeyp1rj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1265974572-6282-1-git-send-email-mitake@dcl.info.waseda.ac.jp>

Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp> writes:

Show 18 quoted lines

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

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

> +	challenge = xcalloc(strlen(challenge_64) + 1, sizeof(char));

Why not xmalloc()?  Does EVP_DecodeBlock() want a zero-filled buffer?

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

By the way, this is probably easier to the eye if you split it into two lines:

	EVP_DecodeBlock((unsigned char *)challenge,
			(unsigned char *)challenge_64, strlen(challenge_64));

> +	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? EVP_md5() returns a pointer to EVP_MD but how and when is that resource released?

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

Show 12 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?

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

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.