Libguestfs - Feb 2014 - Re: [PATCH 2/2] builder: use a disposable GPG keyring for every Sigchecker

On Thu, Feb 20, 2014 at 11:53:17AM +0100, Pino Toscano
wrote:
> Create a temporary directory and tell gpg to use it as homedir, so
> imported keys do not get into the user's keyring. This also avoid
> importing the default key when a different one is needed to check the
> signature.
> 
> The only exception is when a non-default fingerprint is used: in this
> case, that key is read from the user's keyring, since it is where it
is.
The mkdtemp part is fine.  You could spin that off into a separate
commit, so it could be a candidate for backporting.

The rest I found a bit confusing.  What does it do exactly?
> diff --git a/builder/sigchecker.ml b/builder/sigchecker.ml
> index b20a186..10e342e 100644
> --- a/builder/sigchecker.ml
> +++ b/builder/sigchecker.ml
> @@ -95,21 +95,38 @@ ZvXkQ3FVJwZoLmHw47vvlVpLD/4gi1SuHWieRvZ+UdDq00E348pm
>  =neBW
>  -----END PGP PUBLIC KEY BLOCK-----
>  "
> -let key_imported = ref false
>  
>  type t = {
>    debug : bool;
>    gpg : string;
>    fingerprint : string;
>    check_signature : bool;
> +  gpghome : string;
> +  mutable key_imported : bool;
>  }
>  
>  let create ~debug ~gpg ~fingerprint ~check_signature > +  (* Create a
temporary directory for gnupg. *)
> +  let tmpdir = Mkdtemp.mkdtemp (Filename.temp_dir_name //
"vb.gpghome.XXXXXX") in
> +  rmdir_on_exit tmpdir;
> +  (* Run gpg once, so it can setup its own home directory, failing
> +   * if it cannot.
> +   *)
> +  let cmd = sprintf "%s --homedir %s --list-keys%s"
> +    gpg tmpdir (if debug then "" else " >/dev/null
2>&1") in
> +  if debug then eprintf "%s\n%!" cmd;
> +  let r = Sys.command cmd in
> +  if r <> 0 then (
> +    eprintf (f_"virt-builder: error: GPG failure: could not run GPG
the first time\nUse the '-v' option and look for earlier error
messages.\n");
> +    exit 1
> +  );
>    {
>      debug = debug;
>      gpg = gpg;
>      fingerprint = fingerprint;
>      check_signature = check_signature;
> +    gpghome = tmpdir;
> +    key_imported = false;
>    }
>  
>  (* Compare two strings of hex digits ignoring whitespace and case. *)
> @@ -159,8 +176,9 @@ and do_verify t args >    let status_file =
Filename.temp_file "vbstat" ".txt" in
>    unlink_on_exit status_file;
>    let cmd > -    sprintf "%s --verify%s --status-file %s %s"
> -        t.gpg (if t.debug then "" else " -q --logger-file
/dev/null")
> +    sprintf "%s --homedir %s --verify%s --status-file %s %s"
> +        t.gpg t.gpghome
> +        (if t.debug then "" else " -q --logger-file
/dev/null")
>          (quote status_file) args in
>    if t.debug then eprintf "%s\n%!" cmd;
>    let r = Sys.command cmd in
> @@ -188,23 +206,40 @@ and do_verify t args >      exit 1
>    )
>  
> -(* Import the default public key. *)
> +(* Import the requested public key. *)
>  and import_key t > -  if not !key_imported then (
> -    let filename, chan = Filename.open_temp_file "vbpubkey"
".asc" in
> -    unlink_on_exit filename;
> -    output_string chan default_pubkey;
> -    close_out chan;
> -
> -    let cmd = sprintf "%s --import %s%s"
> -      t.gpg (quote filename)
> +  if not t.key_imported then (
> +    let keyfile = ref "" in
> +    if equal_fingerprints default_fingerprint t.fingerprint then (
> +      let filename, chan = Filename.open_temp_file "vbpubkey"
".asc" in
> +      unlink_on_exit filename;
> +      output_string chan default_pubkey;
> +      close_out chan;
> +      keyfile := filename
> +    ) else (
> +      let filename = Filename.temp_file "vbpubkey"
".asc" in
> +      unlink_on_exit filename;
> +      let cmd = sprintf "%s --yes --armor --output %s --export
%s%s"
> +        t.gpg (quote filename) (quote t.fingerprint)
> +        (if t.debug then "" else " >/dev/null
2>&1") in
> +      if t.debug then eprintf "%s\n%!" cmd;
> +      let r = Sys.command cmd in
> +      if r <> 0 then (
> +        eprintf (f_"virt-builder: error: could not export public
key\nUse the '-v' option and look for earlier error messages.\n");
> +        exit 1
> +      );
> +      keyfile := filename
> +    );
> +
> +    let cmd = sprintf "%s --homedir %s --import %s%s"
> +      t.gpg t.gpghome (quote !keyfile)
>        (if t.debug then "" else " >/dev/null
2>&1") in
>      let r = Sys.command cmd in
>      if r <> 0 then (
>        eprintf (f_"virt-builder: error: could not import public
key\nUse the '-v' option and look for earlier error messages.\n");
>        exit 1
>      );
> -    key_imported := true
> +    t.key_imported <- true
>    )
>  
>  type csum_t = SHA512 of string
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Friday 21 February 2014 11:10:54 Richard W.M. Jones
wrote:
> On Thu, Feb 20, 2014 at 11:53:17AM +0100, Pino Toscano wrote:
> > Create a temporary directory and tell gpg to use it as homedir, so
> > imported keys do not get into the user's keyring. This also avoid
> > importing the default key when a different one is needed to check
> > the
> > signature.
> > 
> > The only exception is when a non-default fingerprint is used: in
> > this
> > case, that key is read from the user's keyring, since it is where
it
> > is.
> The mkdtemp part is fine.  You could spin that off into a separate
> commit, so it could be a candidate for backporting.
Hm but it would not be used by anything else so far, so not sure what 
would the backport of it actually do.
> The rest I found a bit confusing.  What does it do exactly?
The idea is to use a disposable keyring for each Sigchecker.t, so 
imported keys used for checking won't be imported directly into the 
user's keyring. The "exception" would be when asking to use a 
fingerprint different than the default one, which would be taken from 
the user's keyring.

Currently it does not make much difference, since the only key not in 
user's keyring would be only the default one. In the future, external 
keys stored in own files would be imported in each Sigchecker.t, so not 
tampering the user's keyring.
The current patch is a small step in that direction (the rest is 
basically almost done).

I'm not sure what is confusing in the patch though...

-- 
Pino Toscano

On Fri, Feb 21, 2014 at 01:50:30PM +0100, Pino Toscano
wrote:
> On Friday 21 February 2014 11:10:54 Richard W.M. Jones wrote:
> > On Thu, Feb 20, 2014 at 11:53:17AM +0100, Pino Toscano wrote:
> > > Create a temporary directory and tell gpg to use it as homedir,
so
> > > imported keys do not get into the user's keyring. This also
avoid
> > > importing the default key when a different one is needed to check
> > > the
> > > signature.
> > > 
> > > The only exception is when a non-default fingerprint is used: in
> > > this
> > > case, that key is read from the user's keyring, since it is
where it
> > > is.
> > The mkdtemp part is fine.  You could spin that off into a separate
> > commit, so it could be a candidate for backporting.
> 
> Hm but it would not be used by anything else so far, so not sure what 
> would the backport of it actually do.
Just thinking that we might use the mkdtemp binding somewhere else.

sysprep/sysprep_operation_script.ml is one candidate.
> > The rest I found a bit confusing.  What does it do exactly?
> 
> The idea is to use a disposable keyring for each Sigchecker.t, so 
> imported keys used for checking won't be imported directly into the 
> user's keyring. The "exception" would be when asking to use a
> fingerprint different than the default one, which would be taken from 
> the user's keyring.
> 
> Currently it does not make much difference, since the only key not in 
> user's keyring would be only the default one. In the future, external 
> keys stored in own files would be imported in each Sigchecker.t, so not 
> tampering the user's keyring.
> The current patch is a small step in that direction (the rest is 
> basically almost done).
> 
> I'm not sure what is confusing in the patch though...
OK, I see.

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top