On 2010年02月12日 05:30, 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 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<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?
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.
--
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