Libguestfs - Sep 2019 - [PATCH] v2v: -o rhv-upload: make -oo rhv-cafile optional

It makes little sense to require the oVirt certificate, especially when
the verification of the connection (-oo rhv-verifypeer) is disabled by
default.  The only work done with the certificate in that case is
checking that it is a valid certificate file.

Hence, make -oo rhv-cafile optional, requiring it only when
-oo rhv-verifypeer is enabled.
---
 v2v/output_rhv_upload.ml    | 16 +++++++++-------
 v2v/virt-v2v-output-rhv.pod |  2 ++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
index 24a289169..d08502826 100644
--- a/v2v/output_rhv_upload.ml
+++ b/v2v/output_rhv_upload.ml
@@ -28,7 +28,7 @@ open Types
 open Utils
 
 type rhv_options = {
-  rhv_cafile : string;
+  rhv_cafile : string option;
   rhv_cluster : string option;
   rhv_direct : bool;
   rhv_verifypeer : bool;
@@ -76,15 +76,13 @@ let parse_output_options options         error (f_"-o
rhv-upload: unknown output option ‘-oo %s’") k
   ) options;
 
-  let rhv_cafile -    match !rhv_cafile with
-    | Some s -> s
-    | None ->
-       error (f_"-o rhv-upload: must use ‘-oo rhv-cafile’ to supply the
path to the oVirt or RHV user’s ‘ca.pem’ file") in
+  let rhv_cafile = !rhv_cafile in
   let rhv_cluster = !rhv_cluster in
   let rhv_direct = !rhv_direct in
   let rhv_verifypeer = !rhv_verifypeer in
   let rhv_disk_uuids = Option.map List.rev !rhv_disk_uuids in
+  if rhv_verifypeer && rhv_cafile = None then
+     error (f_"-o rhv-upload: must use ‘-oo rhv-cafile’ to supply the path
to the oVirt or RHV user’s ‘ca.pem’ file");
 
   { rhv_cafile; rhv_cluster; rhv_direct; rhv_verifypeer; rhv_disk_uuids }
 
@@ -92,6 +90,10 @@ let nbdkit_python_plugin =
Config.virt_v2v_nbdkit_python_plugin
 let pidfile_timeout = 30
 let finalization_timeout = 5*60
 
+let json_optstring = function
+  | Some s -> JSON.String s
+  | None -> JSON.Null
+
 class output_rhv_upload output_alloc output_conn
                         output_password output_storage
                         rhv_options @@ -195,7 +197,7 @@ See also the
virt-v2v-output-rhv(1) manual.")
     "output_sparse", JSON.Bool (match output_alloc with
                                 | Sparse -> true
                                 | Preallocated -> false);
-    "rhv_cafile", JSON.String rhv_options.rhv_cafile;
+    "rhv_cafile", json_optstring rhv_options.rhv_cafile;
     "rhv_cluster",
       JSON.String (Option.default "Default" rhv_options.rhv_cluster);
     "rhv_direct", JSON.Bool rhv_options.rhv_direct;
diff --git a/v2v/virt-v2v-output-rhv.pod b/v2v/virt-v2v-output-rhv.pod
index e840ca78d..04a894268 100644
--- a/v2v/virt-v2v-output-rhv.pod
+++ b/v2v/virt-v2v-output-rhv.pod
@@ -101,6 +101,8 @@ The storage domain.
 The F<ca.pem> file (Certificate Authority), copied from
 F</etc/pki/ovirt-engine/ca.pem> on the oVirt engine.
 
+This option must be specified if I<-oo rhv-verifypeer> is enabled.
+
 =item I<-oo rhv-cluster=>C<CLUSTERNAME>
 
 Set the RHV Cluster Name.  If not given it uses C<Default>.
-- 
2.21.0

On Fri, Sep 27, 2019 at 02:23:29PM +0200, Pino Toscano
wrote:
> It makes little sense to require the oVirt certificate, especially when
> the verification of the connection (-oo rhv-verifypeer) is disabled by
> default.  The only work done with the certificate in that case is
> checking that it is a valid certificate file.
> 
> Hence, make -oo rhv-cafile optional, requiring it only when
> -oo rhv-verifypeer is enabled.
> ---
>  v2v/output_rhv_upload.ml    | 16 +++++++++-------
>  v2v/virt-v2v-output-rhv.pod |  2 ++
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
> index 24a289169..d08502826 100644
> --- a/v2v/output_rhv_upload.ml
> +++ b/v2v/output_rhv_upload.ml
> @@ -28,7 +28,7 @@ open Types
>  open Utils
>  
>  type rhv_options = {
> -  rhv_cafile : string;
> +  rhv_cafile : string option;
>    rhv_cluster : string option;
>    rhv_direct : bool;
>    rhv_verifypeer : bool;
> @@ -76,15 +76,13 @@ let parse_output_options options >         error
(f_"-o rhv-upload: unknown output option ‘-oo %s’") k
>    ) options;
>  
> -  let rhv_cafile > -    match !rhv_cafile with
> -    | Some s -> s
> -    | None ->
> -       error (f_"-o rhv-upload: must use ‘-oo rhv-cafile’ to supply
the path to the oVirt or RHV user’s ‘ca.pem’ file") in
> +  let rhv_cafile = !rhv_cafile in
>    let rhv_cluster = !rhv_cluster in
>    let rhv_direct = !rhv_direct in
>    let rhv_verifypeer = !rhv_verifypeer in
>    let rhv_disk_uuids = Option.map List.rev !rhv_disk_uuids in
> +  if rhv_verifypeer && rhv_cafile = None then
> +     error (f_"-o rhv-upload: must use ‘-oo rhv-cafile’ to supply the
path to the oVirt or RHV user’s ‘ca.pem’ file");
>  
>    { rhv_cafile; rhv_cluster; rhv_direct; rhv_verifypeer; rhv_disk_uuids }
>  
> @@ -92,6 +90,10 @@ let nbdkit_python_plugin =
Config.virt_v2v_nbdkit_python_plugin
>  let pidfile_timeout = 30
>  let finalization_timeout = 5*60
>  
> +let json_optstring = function
> +  | Some s -> JSON.String s
> +  | None -> JSON.Null
> +
>  class output_rhv_upload output_alloc output_conn
>                          output_password output_storage
>                          rhv_options > @@ -195,7 +197,7 @@ See also the
virt-v2v-output-rhv(1) manual.")
>      "output_sparse", JSON.Bool (match output_alloc with
>                                  | Sparse -> true
>                                  | Preallocated -> false);
> -    "rhv_cafile", JSON.String rhv_options.rhv_cafile;
> +    "rhv_cafile", json_optstring rhv_options.rhv_cafile;
>      "rhv_cluster",
>        JSON.String (Option.default "Default"
rhv_options.rhv_cluster);
>      "rhv_direct", JSON.Bool rhv_options.rhv_direct;
> diff --git a/v2v/virt-v2v-output-rhv.pod b/v2v/virt-v2v-output-rhv.pod
> index e840ca78d..04a894268 100644
> --- a/v2v/virt-v2v-output-rhv.pod
> +++ b/v2v/virt-v2v-output-rhv.pod
> @@ -101,6 +101,8 @@ The storage domain.
>  The F<ca.pem> file (Certificate Authority), copied from
>  F</etc/pki/ovirt-engine/ca.pem> on the oVirt engine.
>  
> +This option must be specified if I<-oo rhv-verifypeer> is enabled.
> +
>  =item I<-oo rhv-cluster=>C<CLUSTERNAME>
>  
>  Set the RHV Cluster Name.  If not given it uses C<Default>.
> -- 
> 2.21.0
Yes, as discussed on IRC this makes sense, thanks.

ACK.

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