Re: [gmime-devel] [PATCH 5/6] Use pinentry-mode loopback in test suite when using "modern" GnuPG

[Jeffrey Stedfast <fejj gnome org>]

Commentary inline...

On 12/2/2016 2:02 AM, Daniel Kahn Gillmor wrote:
> The "Modern" GnuPG suite (2.1.x or higher) defaults to relying on the
> gpg-agent for secret key material access, which can prompt the user
> for a passphrase.  The test suite uses callbacks to supply the
> passphrase, so in these modern versions it should use "pinentry-mode
> loopback".
>
> Many users of GMime are likely to avoid using the passphrase callback
> and instead to rely on permission from the cryptographic agent
> directly.  We do not currently test these scenarios, though we could
> do so with a fake pinentry.  If we do that, then those scenarios
> should *not* use the loopback pinentry-mode, and we'd need to adjust
> this setup.
>
> In the longer term, the right way to resolve all of this would be to
> use gpgme directly, instead of having our own wrapper that invokes gpg
> manually.

I think my resistance to moving to gpgme has worn out, so I'm on board 
with that.

> ---
>   tests/testsuite.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
>
> diff --git a/tests/testsuite.c b/tests/testsuite.c
> index 9422e93..84079d1 100644
> --- a/tests/testsuite.c
> +++ b/tests/testsuite.c
> @@ -27,6 +27,10 @@
>   #ifdef ENABLE_THREADS
>   #include <pthread.h>
>   #endif
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
>   
>   #include "testsuite.h"
>   
> @@ -346,9 +350,38 @@ g_throw (Exception *ex)
>         longjmp (env->env, 1);
>   }
>   
> +static int
> +is_gpg_modern()
> +{
> +       const char vheader[] = "gpg (GnuPG) ";
> +       FILE *vpipe;
> +       char *vstring;
> +       size_t vlen;
> +       int ret;
> +
> +       if ((vpipe = popen ("gpg --version", "r")) == NULL)
> +               return 0;
> +       vlen = 0;
> +       vstring = NULL;
> +       if (getline (&vstring, &vlen, vpipe) == -1)
> +               return 0;

Shouldn't vpipe be closed in this error condition?

> +       if (strncmp (vstring, vheader, sizeof (vheader) - 1))
> +               return 0;

Same. In fact, I'd probably recommend pclose()ing vpipe as soon as you 
finish reading the output of gpg --version (no reason to keep it open 
after reading it).

> +       ret = (vstring[sizeof (vheader) - 1] > '2') ||
> +               (vstring[sizeof (vheader) - 1] == '2' &&
> +                vstring[sizeof (vheader)] == '.' &&
> +                vstring[sizeof (vheader) + 1] >= '1');

This has the potential of reading past the end of the buffer.

> +       free (vstring);
> +       pclose (vpipe);
> +       return ret;
> +}
> +
>   int
>   testsuite_setup_gpghome ()
>   {
> +       int gpgconf;
> +       const char directive[] = "pinentry-mode loopback\n";
> +       
>         /* reset .gnupg config directory */
>         if (system ("/bin/rm -rf ./tmp") != 0)
>                 return EXIT_FAILURE;
> @@ -362,6 +395,16 @@ testsuite_setup_gpghome ()
>   
>   	if (system ("gpg --list-keys > /dev/null 2>&1") != 0)
>                 return EXIT_FAILURE;
> +       
> +       if (is_gpg_modern()) {
> +               if ((gpgconf = open ("./tmp/.gnupg/gpg.conf", O_WRONLY | O_CREAT | O_TRUNC, 0400)) == -1)
> +                       return EXIT_FAILURE;
> +               if (write (gpgconf, directive, sizeof(directive) - 1) != sizeof(directive) - 1)
> +                       return EXIT_FAILURE;
> +               if (close (gpgconf) != 0)
> +                       return EXIT_FAILURE;
> +       }

This might be cleaner if you used GMimeStreamFs or FILE* since the 
correct thing to do is to loop the write() call until all data is 
written and/or until -1 is returned where errno is not EINTR.

Jeff