From: Hitoshi Mitake Date: Fri, 12 Feb 2010 11:23:12 GMT Subject: Re: [PATCH v3 1/2] git-imap-send: Add CRAM-MD5 authenticate method support Message-ID: <4B753A20.5060006@dcl.info.waseda.ac.jp> In-Reply-To: <7vaavf4iso.fsf@alter.siamese.dyndns.org> On 2010年02月12日 05:30, Junio C Hamano wrote: > Hitoshi Mitake 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 easy 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. > > Except for some grammer and length of the third line this description > looks good. It concisely explains the design decision. Thanks, I'll separate and fix the third line. > >> v3: Erik's 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. > > Please put these two lines below three-dash lines. People reading "git > log" output 6 months from now won't know nor care what v2 looked like. Do you mean like this? --- v3: Erik's 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. Sorry, I don't know well about custom of Git. > >> diff --git a/imap-send.c b/imap-send.c >> index ba72fa4..caa4e1b 100644 >> --- a/imap-send.c >> +++ b/imap-send.c >> @@ -25,10 +25,16 @@ >> #include "cache.h" >> #include "exec_cmd.h" >> #include "run-command.h" >> + >> #ifdef NO_OPENSSL >> typedef void *SSL; >> +#else >> +#include >> +#include >> #endif >> >> +static int login; >> + > > Does this variable have a meaning? login what? > > - "login attempted--if we have failed, the authenticator is wrong---no > point retrying"? > > - "login attempt succeeded and we are now authenticated"? "logged_in" > would be a better name if this is the case. > > Something else? This is remnant of my dirty code. I removed it. > >> @@ -526,8 +548,9 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx, >> get_cmd_result(ctx, NULL); >> >> bufl = nfsnprintf(buf, sizeof(buf), cmd->cb.data ? CAP(LITERALPLUS) ? >> - "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n", >> - cmd->tag, cmd->cmd, cmd->cb.dlen); >> + "%d %s{%d+}\r\n" : "%d %s{%d}\r\n" : "%d %s\r\n", >> + cmd->tag, cmd->cmd, cmd->cb.dlen); >> + > > What did you change here? Indentation? It is accidentally indentation, removed. > >> @@ -949,6 +972,72 @@ static void imap_close_store(struct store *ctx) >> free(ctx); >> } >> >> +/* >> + * hexchar() and cram() functions are >> + * based on ones of isync project ... http://isync.sf.net/ >> + * Thanks! >> + */ >> +static char hexchar(unsigned int b) >> +{ >> + return b< 10 ? '0' + b : 'a' + (b - 10); >> +} >> + > > Do you need the above helper function outside "#ifndef NO_OPENSSL" block? Clearly not... I moved it to inside of #ifdef ... #endif block. > >> +#ifndef NO_OPENSSL >> + >> +static char *cram(const char *challenge_64, const char *user, const char *pass) >> +{ >> + int i; >> + HMAC_CTX hmac; >> + char hash[16], hex[33], challenge[256], response[256]; >> + char *response_64; >> + >> + memset(challenge, 0, 256); >> + EVP_DecodeBlock((unsigned char *)challenge, (unsigned char *)challenge_64, strlen(challenge_64)); > > In this codepath, is there anything that guarantees that the decoded > result is short enough to fit in challenge[256]? I was too optimistic, my next patch caliculate exact size of these buffer. > >> + HMAC_Init(&hmac, (unsigned char *)pass, strlen(pass), EVP_md5()); >> + HMAC_Update(&hmac, (unsigned char *)challenge, strlen(challenge)); >> + HMAC_Final(&hmac, (unsigned char *)hash, NULL); > > Hmph, if challenge needs to be always casted to (unsigned char*), perhaps > is it better declared as such? (not rhetorical---doing so might need cast > in other calls but I am too lazy to check myself so instead I am asking). If I declare them as unsigned char *, then another casting for strlen() required. And these are more than current casting(current casts:6, calling strlen:7). > >> + 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); >> + } >> + >> + memset(response, 0, 256); >> + snprintf(response, 256, "%s %s", user, hex); > > "hex" would be of a limited and known length, but username could be overly > long, no? Is it Ok to truncate that silently using snprintf while > creating response (not rhetorical---your caller may be barfing on overlong > user name, but I am too lazy to check, so instead I am asking)? > Exact calculation of required length of buffer is possible, I implemented. >> + response_64 = calloc(256 , sizeof(char)); > > Do you need to allocate this, or just have the caller supply a pointer > into an array on its stack as an argument to this function? Calculating size of response_64 before calling cram() is possible. But doing ENCODED_SIZE(strlen(user) + 1 + strlen(hex) + 1) is not read for readability, I think. # Please think that ENCODED_SIZE(n) is maximum required buffer size # encoding n bytes by base64. > >> + EVP_EncodeBlock((unsigned char *)response_64, (unsigned char *)response, strlen(response)); > > Again, is there anything that guarantees response would fit after encoding > in respose_64 in this codepath? > >> + return response_64; > >> +#else >> + >> +static char *cram(const char *challenge_64 __used, const char *user __used, const char *pass __used) > > Does everybody understand __used annotation these days, or just gcc? This was custom of perf of linux kernel, and improper for Git, I removed these. > >> +{ >> + fprintf(stderr, "If you want to use CRAM-MD5 authenticate method," >> + "you have to build git-imap-send with OpenSSL library\n"); >> + exit(0); > > Should this exit with "success"? Cleary not... > >> +static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const char *prompt) >> +{ >> + int ret; >> + char *response; >> + >> + response = cram(prompt, server.user, server.pass); >> + ret = socket_write(&ctx->imap->buf.sock, response, strlen(response)); >> + if (ret != strlen(response)) { >> + fprintf(stderr, "IMAP error: sending response failed\n"); >> + return -1; > > Perhaps 'return error("message fmt without LF at the end", args...);'? I didn't know error(), I'll use it. Thanks for your detailed review! I'll send v4 later.