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

[Neal H. Walfield]

At Sun, 14 Jun 2015 10:24:04 +0900,
Daiki Ueno wrote:
> - the UID value check is now replaced with simple cast to uint32_t,
>   using configure time check

Is this really safe?  Before you caught the error, now we silently
take the value mod 2^32.

> > Thus, if allow-emacs-pinentry is not set (1) and we don't forward the
> > INSIDE_EMACS environment variable (2), then pinentry won't try the
> > emacs pinentry (3).
> >
> > Therefore, teaching pinentry about this option is not required.
> >
> > Does that make sense or do you have another use case in mind that I'm
> > missing?
> 
> As Werner already pointed, (2) is not always the case with the current
> code.
> 
> However, perhaps the above scenario might be actually possible by
> explicitly unsetenv'ing INSIDE_EMACS in gpg-agent, when
> allow-emacs-pinentry is not set in gpg-agent.conf (like we do for
> DISPLAY, when keep-display is not set).

I seem to remember Werner suggesting that this would require a large
change, but if it is easy, I would welcome this approach.

> diff --git a/emacs/pinentry-emacs.c b/emacs/pinentry-emacs.c
> new file mode 100644
> index 0000000..de4ca05
> --- /dev/null
> +++ b/emacs/pinentry-emacs.c
...
> pinentry_cmd_handler_t fallback_cmd_handler;

Is there any reason this isn't marked static?

> +static int
> +initial_emacs_cmd_handler (pinentry_t pe)
> +{
> +  if (pe->allow_emacs_prompt && emacs_socket < 0)
> +    pinentry_emacs_init ();
> +
> +  /* If we have successfully connected to Emacs, substitute
> +     pinentry_cmd_handler to emacs_cmd_handler.  Otherwise, set it
> +     back to fallback_cmd_handler.  */
> +  if (emacs_socket < 0)
> +    pinentry_cmd_handler = fallback_cmd_handler;
> +  else
> +    pinentry_cmd_handler = emacs_cmd_handler;
> +
> +  return (* pinentry_cmd_handler) (pe);
> +}
...
> +int
> +pinentry_emacs_init (void)
> +{
> +  char buffer[256];
> +  assuan_error_t error;
> +
> +  assert (emacs_socket < 0);
> +
> +  /* Check if we can connect to the Emacs server socket.  */
> +  if (!set_socket ("pinentry"))
> +    return 0;
> +
> +  /* Check if the server responds.  */
> +  read_from_emacs (emacs_socket, INITIAL_TIMEOUT, buffer, sizeof (buffer),
> +		   &error);
> +  return error == ASSUAN_No_Error;
> +}

If set_socket is successful, but read_from_emacs fails, then
emacs_socket will be valid (>= 0) and the test in
initial_emacs_cmd_handler will fail.  I think
initial_emacs_cmd_handler either needs to test the return result of
pinentry_emacs_init or pinentry_emacs_init needs to deinit the socket
if emacs doesn't have pinentry support.

> diff --git a/pinentry/pinentry.c b/pinentry/pinentry.c
> index 6f7a62c..9619696 100644
> --- a/pinentry/pinentry.c
> +++ b/pinentry/pinentry.c
> @@ -855,6 +861,13 @@ option_handler (ASSUAN_CONTEXT ctx, const char *key, const char *value)
>        pinentry.allow_external_password_cache = 1;
>        pinentry.tried_password_cache = 0;
>      }
> +  else if (!strcmp (key, "allow-emacs-prompt") && !*value)
> +    {
> +      pinentry.allow_emacs_prompt = 1;
> +#ifdef INSIDE_EMACS
> +      pinentry_enable_emacs_cmd_handler ();
> +#endif
> +    }
>    else
>      return ASSUAN_Invalid_Option;
>    return 0;

pinentry_enable_emacs_cmd_handler can be called multiple times, but
this function isn't designed for this.

  void
  pinentry_enable_emacs_cmd_handler (void)
  {
    const char *envvar;
  
    /* Check if INSIDE_EMACS envvar is set.  */
    envvar = getenv ("INSIDE_EMACS");
    if (!envvar || !*envvar)
      return;
  
    /* Substitute pinentry_cmd_handler to initial_emacs_cmd_handler.  */
    fallback_cmd_handler = pinentry_cmd_handler;
    pinentry_cmd_handler = initial_emacs_cmd_handler;
  }

On the second call, fallback_cmd_handler is set to
initial_emacs_cmd_handler, which is not what we want.  Consider:

  $ INSIDE_EMACS=1 curses/pinentry-curses 
  OK Your orders please
  option allow-emacs-prompt
  OK
  getpin
  stat: No such file or directory
  D 123
  OK
  $ INSIDE_EMACS=1 curses/pinentry-curses 
  OK Your orders please
  option allow-emacs-prompt
  OK
  option allow-emacs-prompt
  OK
  getpin
  stat: No such file or directory
  stat: No such file or directory
  stat: No such file or directory
  ...

Also, since pinentry_enable_emacs_cmd_handler is only called from the
option handler, I don't think we need the field allow_emacs_prompt in
the pinentry structure.

Thanks!

:) Neal