Libguestfs - Jun 2016 - [PATCH v2] sysprep: Add --network to enable the network (RHBZ#1345813).

In commit ae6f726ecc3bc1b67fd76e51a7b1e1a33d4dcfc0 we started to use
the virt-customize code to replace various virt-sysprep operations.
This had the effect of adding many more possible operations to
virt-sysprep, but some of them (specifically --install) did not work
unless the appliance network is enabled.  It was not enabled in
virt-sysprep, so these operations never worked.

This change does NOT enable the network by default.  However it adds a
--network flag which may be used in conjunction with --install etc to
make those commands work.

In addition we now emit a warning for certain customize operations if
they fail and if the network is not enabled.  It will print:

  [   4.5] Installing packages: tcpdump
  Error: Cannot retrieve repository metadata (repomd.xml) for repository: base.
Please verify its path and try again
  Could not retrieve mirrorlist
http://mirrorlist.centos.org/?release=6&arch=x86_64&repo=os&infra=stock
error was

  virt-sysprep: warning: the command may have failed because the network is
  disabled.  Try either removing '--no-network' or adding
'--network' on the
  command line.

  virt-sysprep: error: yum -y install 'tcpdump': command exited with an
error

(If the network is already enabled, or if the command is successful,
then the warning is not printed.)

Thanks: Xianghua Chen
---
 customize/customize_run.ml | 22 ++++++++++++++--------
 sysprep/main.ml            |  5 +++++
 sysprep/virt-sysprep.pod   | 13 +++++++++++++
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/customize/customize_run.ml b/customize/customize_run.ml
index d4950a2..b96e40c 100644
--- a/customize/customize_run.ml
+++ b/customize/customize_run.ml
@@ -50,7 +50,7 @@ let run (g : Guestfs.guestfs) root (ops : ops)        warning
(f_"log file %s: %s (ignored)") logfile (Printexc.to_string exn) in
 
   (* Useful wrapper for scripts. *)
-  let do_run ~display cmd +  let do_run ~display ?(warn_failed_no_network =
false) cmd      if not guest_arch_compatible then
       error (f_"host cpu (%s) and guest arch (%s) are not compatible, so
you cannot use command line options that involve running commands in the guest. 
Use --firstboot scripts instead.")
             Guestfs_config.host_cpu guest_arch;
@@ -90,6 +90,11 @@ exec >>%s 2>&1
     with
       Guestfs.Error msg ->
         debug_logfile ();
+        if warn_failed_no_network && not (g#get_network ()) then (
+          prerr_newline ();
+          warning (f_"the command may have failed because the network is
disabled.  Try either removing '--no-network' or adding
'--network' on the command line.");
+          prerr_newline ()
+        );
         error (f_"%s: command exited with an error") display
   in
 
@@ -263,7 +268,7 @@ exec >>%s 2>&1
     | `InstallPackages pkgs ->
       message (f_"Installing packages: %s") (String.concat "
" pkgs);
       let cmd = guest_install_command pkgs in
-      do_run ~display:cmd cmd
+      do_run ~display:cmd ~warn_failed_no_network:true cmd
 
     | `Link (target, links) ->
       List.iter (
@@ -300,11 +305,11 @@ exec >>%s 2>&1
       | Subscription_manager.PoolAuto ->
         message (f_"Attaching to compatible subscriptions");
         let cmd = "subscription-manager attach --auto" in
-        do_run ~display:cmd cmd
+        do_run ~display:cmd ~warn_failed_no_network:true cmd
       | Subscription_manager.PoolId id ->
         message (f_"Attaching to the pool %s") id;
         let cmd = sprintf "subscription-manager attach --pool=%s"
(quote id) in
-        do_run ~display:cmd cmd
+        do_run ~display:cmd ~warn_failed_no_network:true cmd
       )
 
     | `SMRegister ->
@@ -317,17 +322,18 @@ exec >>%s 2>&1
       let cmd = sprintf "subscription-manager register --username=%s
--password=%s"
                   (quote creds.Subscription_manager.sm_username)
                   (quote creds.Subscription_manager.sm_password) in
-      do_run ~display:"subscription-manager register" cmd
+      do_run ~display:"subscription-manager register"
+             ~warn_failed_no_network:true cmd
 
     | `SMRemove ->
       message (f_"Removing all the subscriptions");
       let cmd = "subscription-manager remove --all" in
-      do_run ~display:cmd cmd
+      do_run ~display:cmd ~warn_failed_no_network:true cmd
 
     | `SMUnregister ->
       message (f_"Unregistering with subscription-manager");
       let cmd = "subscription-manager unregister" in
-      do_run ~display:cmd cmd
+      do_run ~display:cmd ~warn_failed_no_network:true cmd
 
     | `SSHInject (user, selector) ->
       (match g#inspect_get_type root with
@@ -362,7 +368,7 @@ exec >>%s 2>&1
     | `Update ->
       message (f_"Updating packages");
       let cmd = guest_update_command () in
-      do_run ~display:cmd cmd
+      do_run ~display:cmd ~warn_failed_no_network:true cmd
 
     | `Upload (path, dest) ->
       message (f_"Uploading: %s to %s") path dest;
diff --git a/sysprep/main.ml b/sysprep/main.ml
index 6f331b5..35a259c 100644
--- a/sysprep/main.ml
+++ b/sysprep/main.ml
@@ -40,6 +40,7 @@ let main ()      let files = ref [] in
     let libvirturi = ref "" in
     let mount_opts = ref "" in
+    let network = ref false in
     let operations = ref None in
 
     let format = ref "auto" in
@@ -131,6 +132,8 @@ let main ()        "--format",  Arg.String
set_format,     s_"format" ^ " " ^ s_"Set format
(default: auto)";
       "--list-operations", Arg.Unit list_operations, " " ^
s_"List supported operations";
       "--mount-options", Arg.Set_string mount_opts,
s_"opts" ^ " " ^ s_"Set mount options (eg
/:noatime;/var:rw,noatime)";
+      "--network", Arg.Set network,           " " ^
s_"Enable appliance network";
+      "--no-network", Arg.Clear network,      " " ^
s_"Disable appliance network (default)";
       "--no-selinux-relabel", Arg.Unit (fun () -> ()),
                                               " " ^
s_"Compatibility option, does nothing";
       "--operation",  Arg.String set_operations, " " ^
s_"Enable/disable specific operations";
@@ -192,6 +195,7 @@ read the man page virt-sysprep(1).
 
     (* Dereference the rest of the args. *)
     let dryrun = !dryrun in
+    let network = !network in
     let operations = !operations in
 
     (* At this point we know which operations are enabled.  So call the
@@ -212,6 +216,7 @@ read the man page virt-sysprep(1).
 
     (* Connect to libguestfs. *)
     let g = open_guestfs () in
+    g#set_network network;
     add g dryrun;
     g#launch ();
 
diff --git a/sysprep/virt-sysprep.pod b/sysprep/virt-sysprep.pod
index d86b1e4..0c1c7c6 100644
--- a/sysprep/virt-sysprep.pod
+++ b/sysprep/virt-sysprep.pod
@@ -219,6 +219,19 @@ Don't print log messages.
 
 To enable detailed logging of individual file operations, use I<-x>.
 
+=item B<--network>
+
+=item B<--no-network>
+
+Enable or disable network access from the guest during the installation.
+
+In virt-sysprep, the network is I<disabled> by default.  You must use
+I<--network> to enable it, in order that options such as
I<--install>
+or I<--update> will work.
+
+L<virt-builder(1)> has more information about the security advantages
+of disabling the network.
+
 =item B<-v>
 
 =item B<--verbose>
-- 
2.7.4

On Monday 13 June 2016 15:41:21 Richard W.M. Jones
wrote:
> In commit ae6f726ecc3bc1b67fd76e51a7b1e1a33d4dcfc0 we started to use
> the virt-customize code to replace various virt-sysprep operations.
> This had the effect of adding many more possible operations to
> virt-sysprep, but some of them (specifically --install) did not work
> unless the appliance network is enabled.  It was not enabled in
> virt-sysprep, so these operations never worked.
> 
> This change does NOT enable the network by default.  However it adds a
> --network flag which may be used in conjunction with --install etc to
> make those commands work.
> 
> In addition we now emit a warning for certain customize operations if
> they fail and if the network is not enabled.  It will print:
> 
>   [   4.5] Installing packages: tcpdump
>   Error: Cannot retrieve repository metadata (repomd.xml) for repository:
base. Please verify its path and try again
>   Could not retrieve mirrorlist
http://mirrorlist.centos.org/?release=6&arch=x86_64&repo=os&infra=stock
error was
> 
>   virt-sysprep: warning: the command may have failed because the network is
>   disabled.  Try either removing '--no-network' or adding
'--network' on the
>   command line.
> 
>   virt-sysprep: error: yum -y install 'tcpdump': command exited
with an error
> 
> (If the network is already enabled, or if the command is successful,
> then the warning is not printed.)
> 
> Thanks: Xianghua Chen
> ---
LGTM, just one note below.
>  customize/customize_run.ml | 22 ++++++++++++++--------
>  sysprep/main.ml            |  5 +++++
>  sysprep/virt-sysprep.pod   | 13 +++++++++++++
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/customize/customize_run.ml b/customize/customize_run.ml
> index d4950a2..b96e40c 100644
> --- a/customize/customize_run.ml
> +++ b/customize/customize_run.ml
> @@ -50,7 +50,7 @@ let run (g : Guestfs.guestfs) root (ops : ops) >      
warning (f_"log file %s: %s (ignored)") logfile (Printexc.to_string
exn) in
>  
>    (* Useful wrapper for scripts. *)
> -  let do_run ~display cmd > +  let do_run ~display
?(warn_failed_no_network = false) cmd >      if not guest_arch_compatible
then
>        error (f_"host cpu (%s) and guest arch (%s) are not compatible,
so you cannot use command line options that involve running commands in the
guest.  Use --firstboot scripts instead.")
>              Guestfs_config.host_cpu guest_arch;
> @@ -90,6 +90,11 @@ exec >>%s 2>&1
>      with
>        Guestfs.Error msg ->
>          debug_logfile ();
> +        if warn_failed_no_network && not (g#get_network ()) then (
> +          prerr_newline ();
> +          warning (f_"the command may have failed because the network
is disabled.  Try either removing '--no-network' or adding
'--network' on the command line.");
> +          prerr_newline ()
> +        );
>          error (f_"%s: command exited with an error") display
>    in
>  
> @@ -263,7 +268,7 @@ exec >>%s 2>&1
>      | `InstallPackages pkgs ->
>        message (f_"Installing packages: %s") (String.concat
" " pkgs);
>        let cmd = guest_install_command pkgs in
> -      do_run ~display:cmd cmd
> +      do_run ~display:cmd ~warn_failed_no_network:true cmd
Maybe just after the "let do_run .. in" can be defined something like

  let do_run_network = do_run ~warn_failed_no_network:true in

just to shorten these various do_run, as syntactic sugar.

Just an idea though, thinking out loud.

Thanks,
-- 
Pino Toscano