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

[Laszlo Ersek]

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
>  
>