[libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir

John Ferlan posted 1 patch 6 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170629143235.26216-1-jferlan@redhat.com
There is a newer version of this series
src/qemu/qemu.conf   | 29 ++++++++++++++++++++---------
src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++-----
2 files changed, 53 insertions(+), 14 deletions(-)
[libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir
Posted by John Ferlan 6 years, 9 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1458630

Introduce virQEMUDriverConfigSetCertDir which will handle reading the
qemu.conf config file specific setting for default, vnc, spice, chardev,
and migrate. If a setting is provided, then validate the existence of the
directory and overwrite the default set by virQEMUDriverConfigNew.

Update the qemu.conf description for default to describe the consequences
if the default directory path does not exist and as well as the descriptions
for each of the *_tls_x509_cert_dir entries.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---

v1: https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html

- Dropped the former 1/2 patch

- Alter the logic of virQEMUDriverConfigSetCertDir to fail instead of
  VIR_INFO if an uncommented entry for one of the *_tls_x509_cert_dir
  has a path that does not exist. This will cause a libvirtd startup
  failure as opposed to the previous logic which would have failed only
  when a domain using TLS was started.

- Alter the description for each of the values to more accurately describe
  what happens.

 src/qemu/qemu.conf   | 29 ++++++++++++++++++++---------
 src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index e6c0832..b0ccffb 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -3,7 +3,7 @@
 # defaults are used.
 
 # Use of TLS requires that x509 certificates be issued. The default is
-# to keep them in /etc/pki/qemu. This directory must contain
+# to keep them in /etc/pki/qemu. This directory must exist and contain:
 #
 #  ca-cert.pem - the CA master certificate
 #  server-cert.pem - the server certificate signed with ca-cert.pem
@@ -13,6 +13,12 @@
 #
 #  dh-params.pem - the DH params configuration file
 #
+# If the directory does not exist or does not contain the necessary files,
+# QEMU domains will fail to start if they are configured to use TLS.
+#
+# In order to overwrite the default path alter the following. If the provided
+# path does not exist, then startup will fail.
+#
 #default_tls_x509_cert_dir = "/etc/pki/qemu"
 
 
@@ -79,8 +85,9 @@
 
 # In order to override the default TLS certificate location for
 # vnc certificates, supply a valid path to the certificate directory.
-# If the provided path does not exist then the default_tls_x509_cert_dir
-# path will be used.
+# If the default listed here does not exist, then the default /etc/pki/qemu
+# is used. If uncommented and the provided path does not exist, then startup
+# will fail.
 #
 #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
 
@@ -164,8 +171,9 @@
 
 # In order to override the default TLS certificate location for
 # spice certificates, supply a valid path to the certificate directory.
-# If the provided path does not exist then the default_tls_x509_cert_dir
-# path will be used.
+# If the default listed here does not exist, then the default /etc/pki/qemu
+# is used. If uncommented and the provided path does not exist, then startup
+# will fail.
 #
 #spice_tls_x509_cert_dir = "/etc/pki/libvirt-spice"
 
@@ -216,8 +224,9 @@
 
 # In order to override the default TLS certificate location for character
 # device TCP certificates, supply a valid path to the certificate directory.
-# If the provided path does not exist then the default_tls_x509_cert_dir
-# path will be used.
+# If the default listed here does not exist, then the default /etc/pki/qemu
+# is used. If uncommented and the provided path does not exist, then startup
+# will fail.
 #
 #chardev_tls_x509_cert_dir = "/etc/pki/libvirt-chardev"
 
@@ -252,8 +261,10 @@
 
 # In order to override the default TLS certificate location for migration
 # certificates, supply a valid path to the certificate directory. If the
-# provided path does not exist then the default_tls_x509_cert_dir path
-# will be used. Once/if a default certificate is enabled/defined, migration
+# default listed here does not exist, then the default /etc/pki/qemu is used.
+# If uncommented and the provided path does not exist, then startup will fail.
+#
+# Once/if a default certificate is enabled/defined, migration
 # will then be able to use the certificate via migration API flags.
 #
 #migrate_tls_x509_cert_dir = "/etc/pki/libvirt-migrate"
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 73c33d6..4eb6f0c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
 }
 
 
+static int
+virQEMUDriverConfigSetCertDir(virConfPtr conf,
+                              const char *setting,
+                              char **value)
+{
+    char *tlsCertDir = NULL;
+
+    if (virConfGetValueString(conf, setting, &tlsCertDir) < 0)
+        return -1;
+
+    if (!tlsCertDir)
+        return 0;
+
+    if (!virFileExists(tlsCertDir)) {
+        virReportError(VIR_ERR_CONF_SYNTAX,
+                       _("directory '%s' does not exist for setting '%s'"),
+                       tlsCertDir, setting);
+        VIR_FREE(tlsCertDir);
+        return -1;
+    } else {
+        VIR_FREE(*value);
+        VIR_STEAL_PTR(*value, tlsCertDir);
+    }
+
+    return 0;
+}
+
+
 int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
                                 const char *filename,
                                 bool privileged)
@@ -467,7 +495,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
     if (!(conf = virConfReadFile(filename, 0)))
         goto cleanup;
 
-    if (virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir) < 0)
+    if (virQEMUDriverConfigSetCertDir(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir) < 0)
         goto cleanup;
     if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0)
         goto cleanup;
@@ -483,7 +511,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
         goto cleanup;
     if (rv == 0)
         cfg->vncTLSx509verify = cfg->defaultTLSx509verify;
-    if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0)
+    if (virQEMUDriverConfigSetCertDir(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0)
         goto cleanup;
     if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0)
         goto cleanup;
@@ -521,7 +549,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 
     if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0)
         goto cleanup;
-    if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0)
+    if (virQEMUDriverConfigSetCertDir(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0)
         goto cleanup;
     if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) < 0)
         goto cleanup;
@@ -541,8 +569,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
             goto cleanup;                                                   \
         if (rv == 0)                                                        \
             cfg->val## TLSx509verify = cfg->defaultTLSx509verify;           \
-        if (virConfGetValueString(conf, #val "_tls_x509_cert_dir",          \
-                                  &cfg->val## TLSx509certdir) < 0)          \
+        if (virQEMUDriverConfigSetCertDir(conf, #val "_tls_x509_cert_dir",  \
+                                          &cfg->val## TLSx509certdir) < 0)  \
             goto cleanup;                                                   \
         if (virConfGetValueString(conf,                                     \
                                   #val "_tls_x509_secret_uuid",             \
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir
Posted by John Ferlan 6 years, 8 months ago
ping?

Tks -

John

On 06/29/2017 10:32 AM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1458630
> 
> Introduce virQEMUDriverConfigSetCertDir which will handle reading the
> qemu.conf config file specific setting for default, vnc, spice, chardev,
> and migrate. If a setting is provided, then validate the existence of the
> directory and overwrite the default set by virQEMUDriverConfigNew.
> 
> Update the qemu.conf description for default to describe the consequences
> if the default directory path does not exist and as well as the descriptions
> for each of the *_tls_x509_cert_dir entries.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> 
> v1: https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html
> 
> - Dropped the former 1/2 patch
> 
> - Alter the logic of virQEMUDriverConfigSetCertDir to fail instead of
>   VIR_INFO if an uncommented entry for one of the *_tls_x509_cert_dir
>   has a path that does not exist. This will cause a libvirtd startup
>   failure as opposed to the previous logic which would have failed only
>   when a domain using TLS was started.
> 
> - Alter the description for each of the values to more accurately describe
>   what happens.
> 
>  src/qemu/qemu.conf   | 29 ++++++++++++++++++++---------
>  src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index e6c0832..b0ccffb 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -3,7 +3,7 @@
>  # defaults are used.
>  
>  # Use of TLS requires that x509 certificates be issued. The default is
> -# to keep them in /etc/pki/qemu. This directory must contain
> +# to keep them in /etc/pki/qemu. This directory must exist and contain:
>  #
>  #  ca-cert.pem - the CA master certificate
>  #  server-cert.pem - the server certificate signed with ca-cert.pem
> @@ -13,6 +13,12 @@
>  #
>  #  dh-params.pem - the DH params configuration file
>  #
> +# If the directory does not exist or does not contain the necessary files,
> +# QEMU domains will fail to start if they are configured to use TLS.
> +#
> +# In order to overwrite the default path alter the following. If the provided
> +# path does not exist, then startup will fail.
> +#
>  #default_tls_x509_cert_dir = "/etc/pki/qemu"
>  
>  
> @@ -79,8 +85,9 @@
>  
>  # In order to override the default TLS certificate location for
>  # vnc certificates, supply a valid path to the certificate directory.
> -# If the provided path does not exist then the default_tls_x509_cert_dir
> -# path will be used.
> +# If the default listed here does not exist, then the default /etc/pki/qemu
> +# is used. If uncommented and the provided path does not exist, then startup
> +# will fail.
>  #
>  #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
>  
> @@ -164,8 +171,9 @@
>  
>  # In order to override the default TLS certificate location for
>  # spice certificates, supply a valid path to the certificate directory.
> -# If the provided path does not exist then the default_tls_x509_cert_dir
> -# path will be used.
> +# If the default listed here does not exist, then the default /etc/pki/qemu
> +# is used. If uncommented and the provided path does not exist, then startup
> +# will fail.
>  #
>  #spice_tls_x509_cert_dir = "/etc/pki/libvirt-spice"
>  
> @@ -216,8 +224,9 @@
>  
>  # In order to override the default TLS certificate location for character
>  # device TCP certificates, supply a valid path to the certificate directory.
> -# If the provided path does not exist then the default_tls_x509_cert_dir
> -# path will be used.
> +# If the default listed here does not exist, then the default /etc/pki/qemu
> +# is used. If uncommented and the provided path does not exist, then startup
> +# will fail.
>  #
>  #chardev_tls_x509_cert_dir = "/etc/pki/libvirt-chardev"
>  
> @@ -252,8 +261,10 @@
>  
>  # In order to override the default TLS certificate location for migration
>  # certificates, supply a valid path to the certificate directory. If the
> -# provided path does not exist then the default_tls_x509_cert_dir path
> -# will be used. Once/if a default certificate is enabled/defined, migration
> +# default listed here does not exist, then the default /etc/pki/qemu is used.
> +# If uncommented and the provided path does not exist, then startup will fail.
> +#
> +# Once/if a default certificate is enabled/defined, migration
>  # will then be able to use the certificate via migration API flags.
>  #
>  #migrate_tls_x509_cert_dir = "/etc/pki/libvirt-migrate"
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 73c33d6..4eb6f0c 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
>  }
>  
>  
> +static int
> +virQEMUDriverConfigSetCertDir(virConfPtr conf,
> +                              const char *setting,
> +                              char **value)
> +{
> +    char *tlsCertDir = NULL;
> +
> +    if (virConfGetValueString(conf, setting, &tlsCertDir) < 0)
> +        return -1;
> +
> +    if (!tlsCertDir)
> +        return 0;
> +
> +    if (!virFileExists(tlsCertDir)) {
> +        virReportError(VIR_ERR_CONF_SYNTAX,
> +                       _("directory '%s' does not exist for setting '%s'"),
> +                       tlsCertDir, setting);
> +        VIR_FREE(tlsCertDir);
> +        return -1;
> +    } else {
> +        VIR_FREE(*value);
> +        VIR_STEAL_PTR(*value, tlsCertDir);
> +    }
> +
> +    return 0;
> +}
> +
> +
>  int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>                                  const char *filename,
>                                  bool privileged)
> @@ -467,7 +495,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>      if (!(conf = virConfReadFile(filename, 0)))
>          goto cleanup;
>  
> -    if (virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir) < 0)
> +    if (virQEMUDriverConfigSetCertDir(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir) < 0)
>          goto cleanup;
>      if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0)
>          goto cleanup;
> @@ -483,7 +511,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>          goto cleanup;
>      if (rv == 0)
>          cfg->vncTLSx509verify = cfg->defaultTLSx509verify;
> -    if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0)
> +    if (virQEMUDriverConfigSetCertDir(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0)
>          goto cleanup;
>      if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0)
>          goto cleanup;
> @@ -521,7 +549,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>  
>      if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0)
>          goto cleanup;
> -    if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0)
> +    if (virQEMUDriverConfigSetCertDir(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0)
>          goto cleanup;
>      if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) < 0)
>          goto cleanup;
> @@ -541,8 +569,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>              goto cleanup;                                                   \
>          if (rv == 0)                                                        \
>              cfg->val## TLSx509verify = cfg->defaultTLSx509verify;           \
> -        if (virConfGetValueString(conf, #val "_tls_x509_cert_dir",          \
> -                                  &cfg->val## TLSx509certdir) < 0)          \
> +        if (virQEMUDriverConfigSetCertDir(conf, #val "_tls_x509_cert_dir",  \
> +                                          &cfg->val## TLSx509certdir) < 0)  \
>              goto cleanup;                                                   \
>          if (virConfGetValueString(conf,                                     \
>                                    #val "_tls_x509_secret_uuid",             \
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir
Posted by Ján Tomko 6 years, 8 months ago
On Thu, Jun 29, 2017 at 10:32:35AM -0400, John Ferlan wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1458630
>
>Introduce virQEMUDriverConfigSetCertDir which will handle reading the
>qemu.conf config file specific setting for default, vnc, spice, chardev,
>and migrate. If a setting is provided, then validate the existence of the
>directory and overwrite the default set by virQEMUDriverConfigNew.
>
>Update the qemu.conf description for default to describe the consequences
>if the default directory path does not exist and as well as the descriptions
>for each of the *_tls_x509_cert_dir entries.
>
>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
>
>v1: https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html
>
>- Dropped the former 1/2 patch
>
>- Alter the logic of virQEMUDriverConfigSetCertDir to fail instead of
>  VIR_INFO if an uncommented entry for one of the *_tls_x509_cert_dir
>  has a path that does not exist. This will cause a libvirtd startup
>  failure as opposed to the previous logic which would have failed only
>  when a domain using TLS was started.
>
>- Alter the description for each of the values to more accurately describe
>  what happens.
>
> src/qemu/qemu.conf   | 29 ++++++++++++++++++++---------
> src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++-----
> 2 files changed, 53 insertions(+), 14 deletions(-)
>
>diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>index e6c0832..b0ccffb 100644
>--- a/src/qemu/qemu.conf
>+++ b/src/qemu/qemu.conf
>@@ -3,7 +3,7 @@
> # defaults are used.
>
> # Use of TLS requires that x509 certificates be issued. The default is
>-# to keep them in /etc/pki/qemu. This directory must contain
>+# to keep them in /etc/pki/qemu. This directory must exist and contain:

I suspect the POSIX specification mandates that the directory must exist
in order to contain files. :)

> #
> #  ca-cert.pem - the CA master certificate
> #  server-cert.pem - the server certificate signed with ca-cert.pem
>@@ -13,6 +13,12 @@
> #
> #  dh-params.pem - the DH params configuration file
> #
>+# If the directory does not exist or does not contain the necessary files,
>+# QEMU domains will fail to start if they are configured to use TLS.
>+#

Isn't this sufficiently covered by 'Use of TLS requires that x509
certificates be issued'?

>+# In order to overwrite the default path alter the following. If the provided
>+# path does not exist, then startup will fail.
>+#

To apply the configuration, you need to restart the daemon. And since
daemon startup will fail, I think the user will be able to notice it.
We should error out on incorrect paths as soon as we can, without
mentioning it in the documentation.

> #default_tls_x509_cert_dir = "/etc/pki/qemu"
>
>
>@@ -79,8 +85,9 @@
>
> # In order to override the default TLS certificate location for
> # vnc certificates, supply a valid path to the certificate directory.
>-# If the provided path does not exist then the default_tls_x509_cert_dir
>-# path will be used.
>+# If the default listed here does not exist, then the default /etc/pki/qemu
>+# is used.

If I override default_tls_x509_cert_dir, without overriding all the
specific *_tls_x509_cert_dir values, I expect they will all use my
value, not the hardcoded default of /etc/pki/qemu.
So the behavior described by the original comment makes more sense.

> If uncommented and the provided path does not exist, then startup
>+# will fail.
> #
> #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
>
>diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>index 73c33d6..4eb6f0c 100644
>--- a/src/qemu/qemu_conf.c
>+++ b/src/qemu/qemu_conf.c
>@@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
> }
>
>
>+static int
>+virQEMUDriverConfigSetCertDir(virConfPtr conf,
>+                              const char *setting,
>+                              char **value)
>+{
>+    char *tlsCertDir = NULL;
>+
>+    if (virConfGetValueString(conf, setting, &tlsCertDir) < 0)
>+        return -1;
>+
>+    if (!tlsCertDir)
>+        return 0;
>+
>+    if (!virFileExists(tlsCertDir)) {
>+        virReportError(VIR_ERR_CONF_SYNTAX,

This is not a syntax error.
Validating these settings does not belong in virQEMUDriverConfigLoadFile;
qemuStateInitialize or a function called from it would be a better
place.

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir
Posted by John Ferlan 6 years, 8 months ago

On 07/19/2017 09:56 AM, Ján Tomko wrote:
> On Thu, Jun 29, 2017 at 10:32:35AM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1458630
>>
>> Introduce virQEMUDriverConfigSetCertDir which will handle reading the
>> qemu.conf config file specific setting for default, vnc, spice, chardev,
>> and migrate. If a setting is provided, then validate the existence of the
>> directory and overwrite the default set by virQEMUDriverConfigNew.
>>
>> Update the qemu.conf description for default to describe the consequences
>> if the default directory path does not exist and as well as the
>> descriptions
>> for each of the *_tls_x509_cert_dir entries.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>
>> v1: https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html
>>
>> - Dropped the former 1/2 patch
>>
>> - Alter the logic of virQEMUDriverConfigSetCertDir to fail instead of
>>  VIR_INFO if an uncommented entry for one of the *_tls_x509_cert_dir
>>  has a path that does not exist. This will cause a libvirtd startup
>>  failure as opposed to the previous logic which would have failed only
>>  when a domain using TLS was started.
>>
>> - Alter the description for each of the values to more accurately
>> describe
>>  what happens.
>>
>> src/qemu/qemu.conf   | 29 ++++++++++++++++++++---------
>> src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++-----
>> 2 files changed, 53 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index e6c0832..b0ccffb 100644
>> --- a/src/qemu/qemu.conf
>> +++ b/src/qemu/qemu.conf
>> @@ -3,7 +3,7 @@
>> # defaults are used.
>>
>> # Use of TLS requires that x509 certificates be issued. The default is
>> -# to keep them in /etc/pki/qemu. This directory must contain
>> +# to keep them in /etc/pki/qemu. This directory must exist and contain:
> 
> I suspect the POSIX specification mandates that the directory must exist
> in order to contain files. :)
> 

Noted, point taken and I'll drop this hunk.

>> #
>> #  ca-cert.pem - the CA master certificate
>> #  server-cert.pem - the server certificate signed with ca-cert.pem
>> @@ -13,6 +13,12 @@
>> #
>> #  dh-params.pem - the DH params configuration file
>> #
>> +# If the directory does not exist or does not contain the necessary
>> files,
>> +# QEMU domains will fail to start if they are configured to use TLS.
>> +#
> 
> Isn't this sufficiently covered by 'Use of TLS requires that x509
> certificates be issued'?
> 

One would think/assume, but then again the point of the bz was about the
comments being too vague, so I've taken the opposite approach to be
somewhat overly verbose.

If the directory exists, but doesn't contain the right files, libvirt
startup will succeed. However, any QEMU domain that's started *and* uses
TLS in some way will fail when QEMU starts if the right files don't
exist. The libvirt config code doesn't check that the specific files
exist in the directory.

It's perhaps "notable" that /etc/pki/qemu is never checked for
existence. So if things were commented out and someone tried to use TLS
without first creating an /etc/pki/qemu, then domain startup would fail
if it was configured to use TLS (or used for migration). That's a quirk
in the "default" setting because we don't provide the /etc/pki/qemu
directory.

>> +# In order to overwrite the default path alter the following. If the
>> provided
>> +# path does not exist, then startup will fail.
>> +#
> 
> To apply the configuration, you need to restart the daemon. And since
> daemon startup will fail, I think the user will be able to notice it.
> We should error out on incorrect paths as soon as we can, without
> mentioning it in the documentation.
> 

Again, since the bz indicated it wasn't clear, I was trying be overly
obvious. Sometimes being overly succinct in documentation has advantages
with respect to setting expectations.

How about if I change them to :

# In order to overwrite the default path alter the following. This path
# definition will be used as the default path for other *_tls_x509_cert_dir
# configuration settings if they are not specifically set.

(and assuming the changes descibed in [1] below)

>> #default_tls_x509_cert_dir = "/etc/pki/qemu"
>>
>>
>> @@ -79,8 +85,9 @@
>>
>> # In order to override the default TLS certificate location for
>> # vnc certificates, supply a valid path to the certificate directory.
>> -# If the provided path does not exist then the default_tls_x509_cert_dir
>> -# path will be used.
>> +# If the default listed here does not exist, then the default
>> /etc/pki/qemu
>> +# is used.
> 
> If I override default_tls_x509_cert_dir, without overriding all the
> specific *_tls_x509_cert_dir values, I expect they will all use my
> value, not the hardcoded default of /etc/pki/qemu.
> So the behavior described by the original comment makes more sense.
> 

But that doesn't reflect the actuality of the code. So are you expecting
the code to change too?

During virQEMUDriverConfigNew (SET_TLS_X509_CERT_DEFAULT), if the
"/etc/pki/libvirt-*" doesn't exist (where * would be vnc, spice,
chardev, migrate), then by default it is set to /etc/pki/qemu.

If someone then later changes default_tls_x509_cert_dir, then that
directory is used instead of the default /etc/pki/qemu. If the other
settings remain commented out, then their defaults are /etc/pki/qemu.

That being said - the virQEMUDriverConfigSetCertDir could change to add
code that could check if "default" was set to something other than the
default, then copy in "default" (and assume it was already checked) [1].

>> If uncommented and the provided path does not exist, then startup
>> +# will fail.
>> #
>> #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
>>

Strange snipping seems to have missed a [...] since the patch definitely
had more here, but they're all the same...

>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 73c33d6..4eb6f0c 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr
>> hugetlbfs,
>> }
>>
>>
>> +static int
>> +virQEMUDriverConfigSetCertDir(virConfPtr conf,
>> +                              const char *setting,
>> +                              char **value)
>> +{
>> +    char *tlsCertDir = NULL;
>> +
>> +    if (virConfGetValueString(conf, setting, &tlsCertDir) < 0)
>> +        return -1;
>> +
>> +    if (!tlsCertDir)

[1] (from above comments)...

If the entry was commented out, then if cfg->defaultTLSx509certdir is
not /etc/pki/qemu (the default), then STRDUP into *value:

    if (!tlsCertDir) {
        if (STRNEQ_NULLABLE(defaultTLS, *value)) {
            if (VIR_STRDUP(*value, defaultTLS) < 0)
                return -1
        }
        return 0;
    }

where defaultTLS is either cfg->defaultTLSx509certdir or NULL for
default. Since for the default case, the STRDUP isn't necessary;
however, for others (vnc, spice, chardev, and migrate) the *value would
be set to whatever cfg->defaultTLSx509certdir is.

If this happens, then the keeping the original comment is fine and the
bz would change it's "expected results" perhaps although it isn't clear
because it's not "default" that's changing.

>> +        return 0;
>> +
>> +    if (!virFileExists(tlsCertDir)) {
>> +        virReportError(VIR_ERR_CONF_SYNTAX,
> 
> This is not a syntax error.

And your suggestion for a better error is what? Should I create a new
one? Should I use virReportSystemError(ENOENT, ???)?

IDC really, but please don't make me guess!

> Validating these settings does not belong in virQEMUDriverConfigLoadFile;
> qemuStateInitialize or a function called from it would be a better
> place.
> 
> Jan

So this is different in what way than securityDriverNames, controllers,
migrateHost, migrationAddress, or namespaces?

I think creating some new validation routine to be run afterwards is
outside the scope of what's being done here especially considering there
already is validation taking place.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir
Posted by Ján Tomko 6 years, 8 months ago
On Wed, Jul 19, 2017 at 04:02:59PM -0400, John Ferlan wrote:
>On 07/19/2017 09:56 AM, Ján Tomko wrote:
>> On Thu, Jun 29, 2017 at 10:32:35AM -0400, John Ferlan wrote:
>>> #
>>> #  ca-cert.pem - the CA master certificate
>>> #  server-cert.pem - the server certificate signed with ca-cert.pem
>>> @@ -13,6 +13,12 @@
>>> #
>>> #  dh-params.pem - the DH params configuration file
>>> #
>>> +# If the directory does not exist or does not contain the necessary
>>> files,
>>> +# QEMU domains will fail to start if they are configured to use TLS.
>>> +#
>>
>> Isn't this sufficiently covered by 'Use of TLS requires that x509
>> certificates be issued'?
>>
>
>One would think/assume, but then again the point of the bz was about the
>comments being too vague, so I've taken the opposite approach to be
>somewhat overly verbose.
>

The bz was about a comment not matching the behavior.

>>> +# In order to overwrite the default path alter the following. If the
>>> provided
>>> +# path does not exist, then startup will fail.
>>> +#
>>
>> To apply the configuration, you need to restart the daemon. And since
>> daemon startup will fail, I think the user will be able to notice it.
>> We should error out on incorrect paths as soon as we can, without
>> mentioning it in the documentation.
>>
>
>Again, since the bz indicated it wasn't clear, I was trying be overly
>obvious. Sometimes being overly succinct in documentation has advantages
>with respect to setting expectations.
>
>How about if I change them to :
>
># In order to overwrite the default path alter the following. This path
># definition will be used as the default path for other *_tls_x509_cert_dir
># configuration settings if they are not specifically set.
>

Looks good.

>(and assuming the changes descibed in [1] below)
>
>>> #default_tls_x509_cert_dir = "/etc/pki/qemu"
>>>
>>>
>>> @@ -79,8 +85,9 @@
>>>
>>> # In order to override the default TLS certificate location for
>>> # vnc certificates, supply a valid path to the certificate directory.
>>> -# If the provided path does not exist then the default_tls_x509_cert_dir
>>> -# path will be used.
>>> +# If the default listed here does not exist, then the default
>>> /etc/pki/qemu
>>> +# is used.
>>
>> If I override default_tls_x509_cert_dir, without overriding all the
>> specific *_tls_x509_cert_dir values, I expect they will all use my
>> value, not the hardcoded default of /etc/pki/qemu.
>> So the behavior described by the original comment makes more sense.
>>
>
>But that doesn't reflect the actuality of the code. So are you expecting
>the code to change too?
>

Yes.

>During virQEMUDriverConfigNew (SET_TLS_X509_CERT_DEFAULT), if the
>"/etc/pki/libvirt-*" doesn't exist (where * would be vnc, spice,
>chardev, migrate), then by default it is set to /etc/pki/qemu.
>
>If someone then later changes default_tls_x509_cert_dir, then that
>directory is used instead of the default /etc/pki/qemu. If the other
>settings remain commented out, then their defaults are /etc/pki/qemu.
>
>That being said - the virQEMUDriverConfigSetCertDir could change to add
>code that could check if "default" was set to something other than the
>default, then copy in "default" (and assume it was already checked) [1].
>
>>> If uncommented and the provided path does not exist, then startup
>>> +# will fail.
>>> #
>>> #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
>>>
>
>Strange snipping seems to have missed a [...] since the patch definitely
>had more here, but they're all the same...
>

I will work on destrangifying my snipping.

>>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>>> index 73c33d6..4eb6f0c 100644
>>> --- a/src/qemu/qemu_conf.c
>>> +++ b/src/qemu/qemu_conf.c
>>> @@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr
>>> hugetlbfs,
>>> }
>>>
>>>
>>> +static int
>>> +virQEMUDriverConfigSetCertDir(virConfPtr conf,
>>> +                              const char *setting,
>>> +                              char **value)
>>> +{
>>> +    char *tlsCertDir = NULL;
>>> +
>>> +    if (virConfGetValueString(conf, setting, &tlsCertDir) < 0)
>>> +        return -1;
>>> +
>>> +    if (!tlsCertDir)
>
>[1] (from above comments)...
>
>If the entry was commented out, then if cfg->defaultTLSx509certdir is
>not /etc/pki/qemu (the default), then STRDUP into *value:
>
>    if (!tlsCertDir) {
>        if (STRNEQ_NULLABLE(defaultTLS, *value)) {
>            if (VIR_STRDUP(*value, defaultTLS) < 0)
>                return -1
>        }
>        return 0;

If the default values should be propagated, it would be simpler
to let the parser only fill the specified paths first and then
fill out the defaults. That way all the auto-magic logic would
be in one place.

>    }
>
>where defaultTLS is either cfg->defaultTLSx509certdir or NULL for
>default. Since for the default case, the STRDUP isn't necessary;
>however, for others (vnc, spice, chardev, and migrate) the *value would
>be set to whatever cfg->defaultTLSx509certdir is.
>
>If this happens, then the keeping the original comment is fine and the
>bz would change it's "expected results" perhaps although it isn't clear
>because it's not "default" that's changing.
>
>>> +        return 0;
>>> +
>>> +    if (!virFileExists(tlsCertDir)) {
>>> +        virReportError(VIR_ERR_CONF_SYNTAX,
>>
>> This is not a syntax error.
>
>And your suggestion for a better error is what? Should I create a new
>one? Should I use virReportSystemError(ENOENT, ???)?
>
>IDC really, but please don't make me guess!
>

It's not really a system error, nor an internal error. So any of
VIR_ERR_CONF_SYNTAX, VIR_ERR_INTERNAL_ERROR or SYSTEM_ERROR should do.

The issue I have with this is that it makes the parser function
dependent on the host state.

>> Validating these settings does not belong in virQEMUDriverConfigLoadFile;
>> qemuStateInitialize or a function called from it would be a better
>> place.
>>
>> Jan
>
>So this is different in what way than securityDriverNames, controllers,
>migrateHost, migrationAddress, or namespaces?

Apart from namespaces, these are checked against duplicates/compared
against some known strings, all things that could be theoretically
represented by some schema document.

For namespaces, we also check them against host state, which does not
belong in a parser.

Jan

>
>I think creating some new validation routine to be run afterwards is
>outside the scope of what's being done here especially considering there
>already is validation taking place.
>
>John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list