Libguestfs - Mar 2022 - [PATCH v2v v2] lib: Use an ACL to allow qemu to access the v2v directory

On 03/22/22 16:48, Richard W.M. Jones wrote:
> When using the libvirt backend and running as root, libvirt will run
> qemu as a non-root user (eg. qemu:qemu).  The v2v directory stores NBD
> endpoints that qemu must be able to open and so we set the directory
> to mode 0711.  Unfortunately this permits any non-root user to open
> the sockets (since, by design, they have predictable names within the
> directory).
> 
> So instead of using directory permissions, use an ACL which allows us
> to precisely give access to the qemu user and no one else.
> 
> Reported-by: Xiaodai Wang
> Thanks: Dr David Gilbert
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2066773
> ---
>  lib/utils.ml | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/utils.ml b/lib/utils.ml
> index 757bc73c8e..48e6b6c82d 100644
> --- a/lib/utils.ml
> +++ b/lib/utils.ml
> @@ -146,6 +146,26 @@ let backend_is_libvirt () >    let backend = fst
(String.split ":" backend) in
>    backend = "libvirt"
>  
> +(* Get the local user that libvirt uses to run qemu when we are
> + * running as root.  This is returned as an optional string
> + * containing either the UID or username.
> + * https://listman.redhat.com/archives/libguestfs/2022-March/028450.html
> + *)
> +let libvirt_qemu_uid () > +  let conn =
Libvirt.Connect.connect_readonly () in
> +  let xml = Libvirt.Connect.get_capabilities conn in
Huh, I didn't know this existed already :)
> +  let doc = Xml.parse_memory xml in
> +  let xpathctx = Xml.xpath_new_context doc in
> +  let expr =
"//secmodel[./model=\"dac\"]/baselabel[@type=\"kvm\"]/text()"
in
So if you run "virsh capabilities", you get:

    <secmodel>
      <model>dac</model>
      <doi>0</doi>
      <baselabel type='kvm'>+107:+107</baselabel>
      <baselabel type='qemu'>+107:+107</baselabel>
    </secmodel>

In case libvirt starts the domain with TCG, then I think the
@type='qemu' filter should apply.

(Or am I confusing myself here? I know that the libguestfs appliance may
use TCG, but maybe that is irrelevant here? Sorry!)
> +  let uid_gid = Xpath_helpers.xpath_string xpathctx expr in
> +  match uid_gid with
> +  | None -> None
> +  | Some uid_gid ->
> +     (* The string will be something like "+107:+107", return
> +      * just the UID part.
> +      *)
> +     Some (fst (String.split ":" uid_gid))
> +
>  (* When using the SSH driver in qemu (currently) this requires
>   * ssh-agent authentication.  Give a clear error if this hasn't been
>   * set up (RHBZ#1139973).  This might improve if we switch to libssh1.
> @@ -158,8 +178,20 @@ let error_if_no_ssh_agent () >  (* Create the
directory containing inX and outX sockets. *)
>  let create_v2v_directory () >    let d = Mkdtemp.temp_dir
"v2v." in
> +  (* If running as root, and if the backend is libvirt, libvirt
> +   * will run qemu as a non-root user.  Allow qemu to open the directory.
> +   *)
>    let running_as_root = Unix.geteuid () = 0 in
> -  if running_as_root then Unix.chmod d 0o711;
> +  if running_as_root && backend_is_libvirt () then (
> +    try
> +      let uid = Option.default "qemu" (libvirt_qemu_uid ()) in
> +      let cmd = sprintf "setfacl -m user:%s:rwx %s" (quote uid)
(quote d) in
Is it OK if we pass "+107" to "setfacl" here?

Thanks,
Laszlo
> +      debug "%s" cmd;
> +      ignore (Sys.command cmd)
> +    with
> +    | exn -> (* Print exception, but continue. *)
> +       debug "could not set ACL on v2v directory: %s"
(Printexc.to_string exn)
> +  );
>    On_exit.rmdir d;
>    d
>  
>

On Tue, Mar 22, 2022 at 05:34:25PM +0100, Laszlo Ersek
wrote:
> On 03/22/22 16:48, Richard W.M. Jones wrote:
> > When using the libvirt backend and running as root, libvirt will run
> > qemu as a non-root user (eg. qemu:qemu).  The v2v directory stores NBD
> > endpoints that qemu must be able to open and so we set the directory
> > to mode 0711.  Unfortunately this permits any non-root user to open
> > the sockets (since, by design, they have predictable names within the
> > directory).
> > 
> > So instead of using directory permissions, use an ACL which allows us
> > to precisely give access to the qemu user and no one else.
> > 
> > Reported-by: Xiaodai Wang
> > Thanks: Dr David Gilbert
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2066773
> > ---
> >  lib/utils.ml | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/utils.ml b/lib/utils.ml
> > index 757bc73c8e..48e6b6c82d 100644
> > --- a/lib/utils.ml
> > +++ b/lib/utils.ml
> > @@ -146,6 +146,26 @@ let backend_is_libvirt () > >    let
backend = fst (String.split ":" backend) in
> >    backend = "libvirt"
> >  
> > +(* Get the local user that libvirt uses to run qemu when we are
> > + * running as root.  This is returned as an optional string
> > + * containing either the UID or username.
> > + *
https://listman.redhat.com/archives/libguestfs/2022-March/028450.html
> > + *)
> > +let libvirt_qemu_uid () > > +  let conn =
Libvirt.Connect.connect_readonly () in
> > +  let xml = Libvirt.Connect.get_capabilities conn in
> 
> Huh, I didn't know this existed already :)
> 
> > +  let doc = Xml.parse_memory xml in
> > +  let xpathctx = Xml.xpath_new_context doc in
> > +  let expr =
"//secmodel[./model=\"dac\"]/baselabel[@type=\"kvm\"]/text()"
in
> 
> So if you run "virsh capabilities", you get:
> 
>     <secmodel>
>       <model>dac</model>
>       <doi>0</doi>
>       <baselabel type='kvm'>+107:+107</baselabel>
>       <baselabel type='qemu'>+107:+107</baselabel>
>     </secmodel>
> 
> In case libvirt starts the domain with TCG, then I think the
> @type='qemu' filter should apply.
If you want to be perfectly correct you can distinguish KVM vs QEMU,
but in practice these are hardcoded to be identical for the 'dac'
driver. They only differ for the 'selinux' secmodel driver today.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, Mar 22, 2022 at 05:34:25PM +0100, Laszlo Ersek
wrote:
> Is it OK if we pass "+107" to "setfacl" here?
Yes it works fine.  The "+" is a surprise feature of getpwnam(3) in
fact,
albeit one which doesn't appear to be documented in the man page.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org