On 2010年02月12日 05:30, Junio C Hamano wrote:
quoted text
> Hitoshi Mitake<mitake@dcl.info.waseda.ac.jp>  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.

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

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

quoted text
>
>> @@ -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.

quoted text
>
>> @@ -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.

quoted text
>
>> +#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.

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

quoted text
>
>> +	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.

quoted text
>> +	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.

quoted text
>
>> +	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.

quoted text
>
>> +{
>> +	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...

quoted text
>
>> +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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html