[PATCH] Add inside-Emacs mode to GUI pinentry programs

[Daiki Ueno]

Hello Neal,

Thanks for the thorough review, responses are inlined below.

By the way, on second thoughts, I ended up with a specialized protocol
for this, to avoid unnecessary character escaping and encoding
conversion (and also reconnecting to a socket every time).  The protocol
is actually a subset of the Pinentry Assuan protocol, and implemented in
pinentry.el, which shall be included in Emacs, maybe under lisp/net/ or
ELPA.

I'm attaching the new patch and pinentry.el.  To test, use "M-x
pinentry-start" instead of "M-x server-start".

"Neal H. Walfield" <neal at walfield.org> writes:

>> +if test "$pinentry_emacs" = "maybe"; then
>> +  AC_MSG_CHECKING([if Unix domain socket is supported])
>
> I think we should check for != "no" here.  If pinentry_emacs is yes,
> but we don't support unix domain sockets, then it would be better to
> error out here than when compiling the code.  Do you agree?

Yes, exactly.  Fixed.

>> +#include <assert.h>
>
> As far as I can tell, this include is gratuitious.

Removed.

>> +  if (strlen (tmpdir) + strlen (socket_name) + 10 + 2
>> +      >= sizeof (emacs_server_address.sun_path))
[...]
>> +  sprintf (emacs_server_address.sun_path, "%s/emacs%lu/%s",
>> +	   tmpdir, (unsigned long) uid, socket_name);
>
> I think the check above is wrong.  It should be something like:
>
>   if (strlen (tmpdir) + strlen("/emacs") + 10 + strlen("/")
>       + strlen (socket_name) + 1
>       >= sizeof (emacs_server_address.sun_path))

Right, thanks for pointing that out.  Fixed using the correct formula.

>> +  if (connect (s, (struct sockaddr *) &emacs_server_address,
>> +	       strlen (emacs_server_address.sun_path) + 2) < 0)
>
> Why don't you just use 'sizeof (emacs_server_address)' here?  I really
> don't like the magic number 2 here.

Fixed using SUN_LEN from GnuPG.

>> +	  if (strprefix ("-print ", p))

> Does it make sense to emit a warning if there is a line with an
> unrecognized prefix?  As far as I can see, lines with unknown commands
> are currently ignored.

Yes, added warnings.

>> +  sprintf (command, "(" CALLBACK_NAME " \"%s\" %s)",
>> +	   name, quoted_value);
>> +  if (quoted_value != nil)
>> +    free (quoted_value);
>
> Here's another instance where I'd strongly prefer something like
> asprintf.

Yes.  Those allocation and copying are no longer necessary with the new
protocol.

>> +  quoted_command = build_command (name, local_value);
>> +  if (local_value != value)
>> +    free (local_value);
>
> I think this comparison is useless.  If value is not NULL, then we
> call pinentry_utf8_to_local, which always allocates a new buffer.

Good point.  Likewise to the above, the extra allocation is not needed
anymore.

> Does emacs not support setting custom button labels?

Yes, it could.  However, for a password query, labels would be too
verbose to be shown in the minibuffer.  So, in pinentry.el, they are
only used for a confirmation query.

> If pe->prompt is NULL, then you should fallback to pe->default_prompt.

Fixed.

> If you have time, it would be nice to add support for the external
> password manager checkbox.  In the very least, please note the lack of
> this support with an XXX comment in the source code.

Sure, added a comment.

>> +	  if (pe->repeat_passphrase)
>> +	    pe->repeat_okay = 1;
>
> Why are you setting this if the user didn't actually enter the
> password twice?

I implemented the repeat feature in the new patch.

>> +int
>> +do_confirm (pinentry_t pe)
[...]
>> +  set_value (pe, "SETPROMPT", pe->prompt);
>
> Again, please fallback to pe->default_prompt.

Fixed.

> Any reason, there is no support for custom button labels?  Also, what
> about support for the other button?  If this is not yet possible,
> please note it with an XXX comment.

Fixed.

>> -#if defined FALLBACK_CURSES || defined PINENTRY_CURSES || defined
>> PINENTRY_GTK
>> +#if defined FALLBACK_CURSES || defined INSIDE_EMACS || defined
>> PINENTRY_CURSES || defined PINENTRY_GTK
>>  char *
>>  pinentry_utf8_to_local (const char *lc_ctype, const char *text)
>>  {
>
> I think we should check if PINENTRY_EMACS is defined, not INSIDE_EMACS
> here.

I reverted this change since the patch no longer uses
pinentry_utf8_to_local.

Regards,
-- 
Daiki Ueno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-inside-Emacs-mode-to-GUI-pinentry-programs.patch
Type: text/x-patch
Size: 33333 bytes
Desc: not available
URL: </pipermail/attachments/20150602/7e54e4ea/attachment-0001.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pinentry.el
URL: </pipermail/attachments/20150602/7e54e4ea/attachment-0001.ksh>