git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
HMHitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Date
Feb 12, 2010, 11:23 UTC
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:
Show 10 quoted lines
> 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.
Show 6 quoted lines
>
>> 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.
Show 29 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?
This is remnant of my dirty code. I removed it.
Show 12 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?
It is accidentally indentation, removed.
Show 17 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?
Clearly not... I moved it to inside of #ifdef ... #endif block.
Show 15 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]?

I was too optimistic, my next patch caliculate exact size of these buffer.

Show 8 quoted lines
>
>> +	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).

Show 15 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)?
>

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.

Show 13 quoted lines
>
>> +	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.

Show 7 quoted lines
>
>> +{
>> +	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...
Show 13 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...);'?
I didn't know error(), I'll use it.

Thanks for your detailed review! I'll send v4 later.

Previous: Junio C HamanoNext: Hitoshi Mitake
Message 15 of 49 in “imap.preformattedHTML and imap.sslverify”
  1. Junio C HamanoFeb 6, 2010
  2. Jeremy WhiteFeb 8, 2010
  3. Junio C HamanoFeb 8, 2010
  4. 0/4 Some improvements for git-imap-sendHitoshi Mitake, Feb 9, 2010
  5. Jeff KingFeb 9, 2010
  6. Erik Faye-LundFeb 9, 2010
  7. Jeff KingFeb 9, 2010
  8. Erik Faye-LundFeb 9, 2010
  9. Jeff KingFeb 9, 2010
  10. 1/2 git-imap-send: Add CRAM-MD5 authenticate method supportHitoshi Mitake, Feb 11, 2010
  11. Erik Faye-LundFeb 11, 2010
  12. Hitoshi MitakeFeb 11, 2010
  13. 1/2 git-imap-send: Add CRAM-MD5 authenticate method supportHitoshi Mitake, Feb 11, 2010
  14. Junio C HamanoFeb 11, 2010
  15. Hitoshi MitakeFeb 12, 2010
  16. 2/2 git-imap-send: Convert LF to CRLF before storing patch to draft boxHitoshi Mitake, Feb 11, 2010
  17. Junio C HamanoFeb 11, 2010
  18. Hitoshi MitakeFeb 12, 2010
  19. 1/2 git-imap-send: Add CRAM-MD5 authenticate method supportHitoshi Mitake, Feb 11, 2010
  20. 2/2 git-imap-send: Convert LF to CRLF before storing patch to draft boxHitoshi Mitake, Feb 11, 2010
  21. 1/2 git-imap-send: Add CRAM-MD5 authenticate method supportHitoshi Mitake, Feb 12, 2010
  22. Erik Faye-LundFeb 12, 2010
  23. Hitoshi MitakeFeb 13, 2010
  24. Junio C HamanoFeb 12, 2010
  25. Hitoshi MitakeFeb 13, 2010
  26. 1/2 git-imap-send: Add CRAM-MD5 authenticate method supportHitoshi Mitake, Feb 13, 2010
  27. Junio C HamanoFeb 13, 2010
  28. Junio C HamanoFeb 16, 2010
  29. Hitoshi MitakeFeb 17, 2010
  30. 1/2 git-imap-send: Add CRAM-MD5 authenticate method supportHitoshi Mitake, Feb 17, 2010
  31. Junio C HamanoFeb 17, 2010
  32. Hitoshi MitakeFeb 18, 2010
  33. Hitoshi MitakeFeb 17, 2010
  34. 2/2 git-imap-send: Convert LF to CRLF before storing patch to draft boxHitoshi Mitake, Feb 12, 2010
  35. 1/4 Add base64 encoder and decoderHitoshi Mitake, Feb 9, 2010
  36. Erik Faye-LundFeb 9, 2010
  37. Hitoshi MitakeFeb 11, 2010
  38. 2/4 Add stuffs for MD5 hash algorithmHitoshi Mitake, Feb 9, 2010
  39. 3/4 git-imap-send: Implement CRAM-MD5 auth methodHitoshi Mitake, Feb 9, 2010
  40. Erik Faye-LundFeb 9, 2010
  41. Hitoshi MitakeFeb 11, 2010
  42. Erik Faye-LundFeb 11, 2010
  43. Hitoshi MitakeFeb 11, 2010
  44. 4/4 git-imap-send: Add method to convert from LF to CRLFHitoshi Mitake, Feb 9, 2010
  45. Jakub NarebskiFeb 9, 2010
  46. Hitoshi MitakeFeb 11, 2010
  47. Linus TorvaldsFeb 9, 2010
  48. Junio C HamanoFeb 9, 2010
  49. Hitoshi MitakeFeb 11, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.