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

Hitoshi Mitake [off-list ref] 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.

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.

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?

What did you change here?  Indentation?

Do you need the above helper function outside "#ifndef NO_OPENSSL" 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]?

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

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

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