Hitoshi Mitake <mitake@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Cc: Jakub Narebski <jnareb@xxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Jeff King <peff@xxxxxxxx>
> Signed-off-by: Hitoshi Mitake <mitake@xxxxxxxxxxxxxxxxxxxxx>
> ---
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@xxxxxxxxxxxxxxxxxxx>
---
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
> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html