On 2010年02月13日 06:44, Junio C Hamano wrote:
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 ...
----------------------------------------------------------------
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: "<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!
--
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