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

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

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

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

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

quoted text
> +	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));

quoted text
> +	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

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

quoted text
> +	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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html