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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2010, 20:30 UTC
Message-ID: <7vaavf4iso.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1265900785-12044-1-git-send-email-mitake@dcl.info.waseda.ac.jp>

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

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

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

Show 18 quoted lines

> 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 <openssl/evp.h>
> +#include <openssl/hmac.h>
>  #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?

Show 9 quoted lines

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

Show 14 quoted lines

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

Show 11 quoted lines

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

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

Show 8 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);
> +	}
> +
> +	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)?

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

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

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

Show 10 quoted lines

> +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...);'?