2:30 p.m.
The qemu.conf indicates that for the various *_tls_x509_cert_dir settings
that "If the provided path does not exist then the default_tls_x509_cert_dir
path will be used."; however, the code didn't quite do that.
The default for each was set during virQEMUDriverConfigNew; however, that
was overwritten if the various config settings were used, but whether the
directory actually existed and falling back to the default action of using
the default configuration was not done.
So that's what these patches do.
John Ferlan (2):
qemu: Clean up long lines in virQEMUDriverConfigLoadFile
qemu: Check for existence of provided *_tls_x509_cert_dir
src/qemu/qemu.conf | 9 +++-
src/qemu/qemu_conf.c | 123 +++++++++++++++++++++++++++++++++++++--------------
2 files changed, 98 insertions(+), 34 deletions(-)
--
2.9.4
2:30 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: Clean up long lines in virQEMUDriverConfigLoadFile
Keep line length <= 80 cols.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_conf.c | 93 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 62 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 73c33d6..94e00b2 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -467,23 +467,28 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (!(conf = virConfReadFile(filename, 0)))
goto cleanup;
- if (virConfGetValueString(conf, "default_tls_x509_cert_dir",
&cfg->defaultTLSx509certdir) < 0)
+ if (virConfGetValueString(conf, "default_tls_x509_cert_dir",
+ &cfg->defaultTLSx509certdir) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "default_tls_x509_verify",
&cfg->defaultTLSx509verify) < 0)
+ if (virConfGetValueBool(conf, "default_tls_x509_verify",
+ &cfg->defaultTLSx509verify) < 0)
goto cleanup;
if (virConfGetValueString(conf, "default_tls_x509_secret_uuid",
&cfg->defaultTLSx509secretUUID) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "vnc_auto_unix_socket",
&cfg->vncAutoUnixSocket) < 0)
+ if (virConfGetValueBool(conf, "vnc_auto_unix_socket",
+ &cfg->vncAutoUnixSocket) < 0)
goto cleanup;
if (virConfGetValueBool(conf, "vnc_tls", &cfg->vncTLS) < 0)
goto cleanup;
- if ((rv = virConfGetValueBool(conf, "vnc_tls_x509_verify",
&cfg->vncTLSx509verify)) < 0)
+ if ((rv = virConfGetValueBool(conf, "vnc_tls_x509_verify",
+ &cfg->vncTLSx509verify)) < 0)
goto cleanup;
if (rv == 0)
cfg->vncTLSx509verify = cfg->defaultTLSx509verify;
- if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir",
&cfg->vncTLSx509certdir) < 0)
+ if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir",
+ &cfg->vncTLSx509certdir) < 0)
goto cleanup;
if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) <
0)
goto cleanup;
@@ -493,16 +498,20 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
if (virConfGetValueString(conf, "vnc_sasl_dir", &cfg->vncSASLdir)
< 0)
goto cleanup;
- if (virConfGetValueBool(conf, "vnc_allow_host_audio",
&cfg->vncAllowHostAudio) < 0)
+ if (virConfGetValueBool(conf, "vnc_allow_host_audio",
+ &cfg->vncAllowHostAudio) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "nographics_allow_host_audio",
&cfg->nogfxAllowHostAudio) < 0)
+ if (virConfGetValueBool(conf, "nographics_allow_host_audio",
+ &cfg->nogfxAllowHostAudio) < 0)
goto cleanup;
- if (virConfGetValueStringList(conf, "security_driver", true,
&cfg->securityDriverNames) < 0)
+ if (virConfGetValueStringList(conf, "security_driver", true,
+ &cfg->securityDriverNames) < 0)
goto cleanup;
- for (i = 0; cfg->securityDriverNames && cfg->securityDriverNames[i] !=
NULL; i++) {
+ for (i = 0;
+ cfg->securityDriverNames && cfg->securityDriverNames[i] != NULL;
i++) {
for (j = i + 1; cfg->securityDriverNames[j] != NULL; j++) {
if (STREQ(cfg->securityDriverNames[i],
cfg->securityDriverNames[j])) {
@@ -514,14 +523,17 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
}
}
- if (virConfGetValueBool(conf, "security_default_confined",
&cfg->securityDefaultConfined) < 0)
+ if (virConfGetValueBool(conf, "security_default_confined",
+ &cfg->securityDefaultConfined) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "security_require_confined",
&cfg->securityRequireConfined) < 0)
+ if (virConfGetValueBool(conf, "security_require_confined",
+ &cfg->securityRequireConfined) < 0)
goto cleanup;
if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0)
goto cleanup;
- if (virConfGetValueString(conf, "spice_tls_x509_cert_dir",
&cfg->spiceTLSx509certdir) < 0)
+ if (virConfGetValueString(conf, "spice_tls_x509_cert_dir",
+ &cfg->spiceTLSx509certdir) < 0)
goto cleanup;
if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) <
0)
goto cleanup;
@@ -531,7 +543,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
if (virConfGetValueString(conf, "spice_password",
&cfg->spicePassword) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "spice_auto_unix_socket",
&cfg->spiceAutoUnixSocket) < 0)
+ if (virConfGetValueBool(conf, "spice_auto_unix_socket",
+ &cfg->spiceAutoUnixSocket) < 0)
goto cleanup;
#define GET_CONFIG_TLS_CERTINFO(val) \
@@ -564,7 +577,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
#undef GET_CONFIG_TLS_CERTINFO
- if (virConfGetValueUInt(conf, "remote_websocket_port_min",
&cfg->webSocketPortMin) < 0)
+ if (virConfGetValueUInt(conf, "remote_websocket_port_min",
+ &cfg->webSocketPortMin) < 0)
goto cleanup;
if (cfg->webSocketPortMin < QEMU_WEBSOCKET_PORT_MIN) {
/* if the port is too low, we can't get the display name
@@ -577,7 +591,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
}
- if (virConfGetValueUInt(conf, "remote_websocket_port_max",
&cfg->webSocketPortMax) < 0)
+ if (virConfGetValueUInt(conf, "remote_websocket_port_max",
+ &cfg->webSocketPortMax) < 0)
goto cleanup;
if (cfg->webSocketPortMax > QEMU_WEBSOCKET_PORT_MAX ||
cfg->webSocketPortMax < cfg->webSocketPortMin) {
@@ -595,7 +610,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
}
- if (virConfGetValueUInt(conf, "remote_display_port_min",
&cfg->remotePortMin) < 0)
+ if (virConfGetValueUInt(conf, "remote_display_port_min",
+ &cfg->remotePortMin) < 0)
goto cleanup;
if (cfg->remotePortMin < QEMU_REMOTE_PORT_MIN) {
/* if the port is too low, we can't get the display name
@@ -608,7 +624,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
}
- if (virConfGetValueUInt(conf, "remote_display_port_max",
&cfg->remotePortMax) < 0)
+ if (virConfGetValueUInt(conf, "remote_display_port_max",
+ &cfg->remotePortMax) < 0)
goto cleanup;
if (cfg->remotePortMax > QEMU_REMOTE_PORT_MAX ||
cfg->remotePortMax < cfg->remotePortMin) {
@@ -626,7 +643,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
}
- if (virConfGetValueUInt(conf, "migration_port_min",
&cfg->migrationPortMin) < 0)
+ if (virConfGetValueUInt(conf, "migration_port_min",
+ &cfg->migrationPortMin) < 0)
goto cleanup;
if (cfg->migrationPortMin <= 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -635,7 +653,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
}
- if (virConfGetValueUInt(conf, "migration_port_max",
&cfg->migrationPortMax) < 0)
+ if (virConfGetValueUInt(conf, "migration_port_max",
+ &cfg->migrationPortMax) < 0)
goto cleanup;
if (cfg->migrationPortMax > 65535 ||
cfg->migrationPortMax < cfg->migrationPortMin) {
@@ -656,7 +675,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (group && virGetGroupID(group, &cfg->group) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "dynamic_ownership",
&cfg->dynamicOwnership) < 0)
+ if (virConfGetValueBool(conf, "dynamic_ownership",
+ &cfg->dynamicOwnership) < 0)
goto cleanup;
if (virConfGetValueStringList(conf, "cgroup_controllers", false,
@@ -681,18 +701,23 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
&cfg->cgroupDeviceACL) < 0)
goto cleanup;
- if (virConfGetValueString(conf, "save_image_format",
&cfg->saveImageFormat) < 0)
+ if (virConfGetValueString(conf, "save_image_format",
+ &cfg->saveImageFormat) < 0)
goto cleanup;
- if (virConfGetValueString(conf, "dump_image_format",
&cfg->dumpImageFormat) < 0)
+ if (virConfGetValueString(conf, "dump_image_format",
+ &cfg->dumpImageFormat) < 0)
goto cleanup;
- if (virConfGetValueString(conf, "snapshot_image_format",
&cfg->snapshotImageFormat) < 0)
+ if (virConfGetValueString(conf, "snapshot_image_format",
+ &cfg->snapshotImageFormat) < 0)
goto cleanup;
if (virConfGetValueString(conf, "auto_dump_path",
&cfg->autoDumpPath) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "auto_dump_bypass_cache",
&cfg->autoDumpBypassCache) < 0)
+ if (virConfGetValueBool(conf, "auto_dump_bypass_cache",
+ &cfg->autoDumpBypassCache) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "auto_start_bypass_cache",
&cfg->autoStartBypassCache) < 0)
+ if (virConfGetValueBool(conf, "auto_start_bypass_cache",
+ &cfg->autoStartBypassCache) < 0)
goto cleanup;
if (virConfGetValueStringList(conf, "hugetlbfs_mount", true,
@@ -718,7 +743,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
}
}
- if (virConfGetValueString(conf, "bridge_helper",
&cfg->bridgeHelperName) < 0)
+ if (virConfGetValueString(conf, "bridge_helper",
+ &cfg->bridgeHelperName) < 0)
goto cleanup;
if (virConfGetValueBool(conf, "mac_filter", &cfg->macFilter) <
0)
@@ -726,9 +752,11 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueBool(conf, "relaxed_acs_check", &cfg->relaxedACS)
< 0)
goto cleanup;
- if (virConfGetValueBool(conf, "clear_emulator_capabilities",
&cfg->clearEmulatorCapabilities) < 0)
+ if (virConfGetValueBool(conf, "clear_emulator_capabilities",
+ &cfg->clearEmulatorCapabilities) < 0)
goto cleanup;
- if (virConfGetValueBool(conf, "allow_disk_format_probing",
&cfg->allowDiskFormatProbing) < 0)
+ if (virConfGetValueBool(conf, "allow_disk_format_probing",
+ &cfg->allowDiskFormatProbing) < 0)
goto cleanup;
if (virConfGetValueBool(conf, "set_process_name",
&cfg->setProcessName) < 0)
goto cleanup;
@@ -777,7 +805,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueUInt(conf, "max_queued", &cfg->maxQueuedJobs)
< 0)
goto cleanup;
- if (virConfGetValueInt(conf, "keepalive_interval",
&cfg->keepAliveInterval) < 0)
+ if (virConfGetValueInt(conf, "keepalive_interval",
+ &cfg->keepAliveInterval) < 0)
goto cleanup;
if (virConfGetValueUInt(conf, "keepalive_count",
&cfg->keepAliveCount) < 0)
goto cleanup;
@@ -830,7 +859,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
}
}
- if (virConfGetValueUInt(conf, "gluster_debug_level",
&cfg->glusterDebugLevel) < 0)
+ if (virConfGetValueUInt(conf, "gluster_debug_level",
+ &cfg->glusterDebugLevel) < 0)
goto cleanup;
if (virConfGetValueStringList(conf, "namespaces", false, &namespaces)
< 0)
@@ -871,7 +901,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
}
}
- if (virConfGetValueString(conf, "memory_backing_dir",
&cfg->memoryBackingDir) < 0)
+ if (virConfGetValueString(conf, "memory_backing_dir",
+ &cfg->memoryBackingDir) < 0)
goto cleanup;
ret = 0;
--
2.9.4
6:17 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Clean up long lines in virQEMUDriverConfigLoadFile
On Wed, 2017-06-28 at 15:30 -0400, John Ferlan wrote:
Keep line length <= 80 cols.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_conf.c | 93 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 62 insertions(+), 31 deletions(-)
I'm pretty sure we agreed that the 80 column limit is not
something we're interested in enforcing: if anything, we're
actually moving away from it when changing existing code.
--
Andrea Bolognani / Red Hat / Virtualization
6:53 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Clean up long lines in virQEMUDriverConfigLoadFile
On 06/29/2017 07:17 AM, Andrea Bolognani wrote:
On Wed, 2017-06-28 at 15:30 -0400, John Ferlan wrote:
> Keep line length <= 80 cols.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_conf.c | 93 ++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 62 insertions(+), 31 deletions(-)
I'm pretty sure we agreed that the 80 column limit is not
something we're interested in enforcing: if anything, we're
actually moving away from it when changing existing code.
--
Andrea Bolognani / Red Hat / Virtualization
The collective "we"? Not sure I agree. I was under the impression that
it wasn't a requirement to change existing code to wrap at 80 cols,
rather it was something that one could do and if they did so it should
be a separate patch rather than interspersing within other changes.
I think if it's one or two characters, then it's not such a huge deal,
but having long lines that wrap in code is unreadable especially for
those of us using 80xNN windows. Those that use 100xNN or 132xNN with
8pt fonts will eventually need an 80xNN window with 18pt fonts ;-).
Still, I'm not really interested in a long debate on this, so I can drop
this patch even though I find the code to be unreadable as is.
John
7:57 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Clean up long lines in virQEMUDriverConfigLoadFile
On Thu, Jun 29, 2017 at 13:17:56 +0200, Andrea Bolognani wrote:
On Wed, 2017-06-28 at 15:30 -0400, John Ferlan wrote:
> Keep line length <= 80 cols.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_conf.c | 93 ++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 62 insertions(+), 31 deletions(-)
I'm pretty sure we agreed that the 80 column limit is not
something we're interested in enforcing: if anything, we're
actually moving away from it when changing existing code.
That very much depends on the circumstances. If the line can be easily
and cleanly broken we should stick to 80. If it would break coding style
and/or not align properly I'm fine with exceding it.
In the patch above, most of the changes breaking the argument on a
second line are fine, but the line which breaks up the 'for' loop is not
okay in this regard, since most for loops are with all three arguments
on a single line.
The 80 col boudnary has a good buffer zone until it starts running into
border of various users and still motivates us to keep the lines short.
2:30 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check for existence of provided *_tls_x509_cert_dir
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. Then if a setting was provided, validating the existence of
the directory and overwriting the default set by virQEMUDriverConfigNew.
Also update the qemu.conf description for default to indicate the consequences
if the default directory does not exist.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu.conf | 9 ++++++++-
src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
2 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index e6c0832..737fa46 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,13 @@
#
# 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 directory alter the following. If the
+# provided directory does not exist, then the setting reverts back to the
+# default /etc/pki/qemu.
+#
#default_tls_x509_cert_dir = "/etc/pki/qemu"
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 94e00b2..a52349f 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -440,6 +440,32 @@ 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)) {
+ VIR_INFO("%s, directory '%s' does not exist, retain default",
+ setting, tlsCertDir);
+ VIR_FREE(tlsCertDir);
+ } else {
+ VIR_FREE(*value);
+ VIR_STEAL_PTR(*value, tlsCertDir);
+ }
+
+ return 0;
+}
+
+
int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
const char *filename,
bool privileged)
@@ -467,8 +493,8 @@ 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)
@@ -487,8 +513,8 @@ 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;
@@ -532,8 +558,8 @@ 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;
@@ -554,8 +580,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
2:24 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check for existence of provided *_tls_x509_cert_dir
On Wed, Jun 28, 2017 at 15:30:28 -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. Then if a setting was provided, validating the existence of
the directory and overwriting the default set by virQEMUDriverConfigNew.
Also update the qemu.conf description for default to indicate the consequences
if the default directory does not exist.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu.conf | 9 ++++++++-
src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
2 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index e6c0832..737fa46 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,13 @@
#
# 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 directory alter the following. If the
+# provided directory does not exist, then the setting reverts back to the
+# default /etc/pki/qemu.
+#
I don't think this is a good idea. We should use the directory a user
specified in qemu.conf. If it doesn't exist, well things won't work.
Sure, we can complain about it in the logs, but we should not fallback
to any magic default in that case. Anyone setting a custom directory for
TLS certificates does this because they want to use it. If the directory
does not exist, it's either because they forgot to create it or they
made a typo somewhere. It's very unlikely someone actually wants to use
a default directory even though they set a custom one.
NACK
Jirka
5:33 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check for existence of provided *_tls_x509_cert_dir
On 06/29/2017 03:24 AM, Jiri Denemark wrote:
On Wed, Jun 28, 2017 at 15:30:28 -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. Then if a setting was provided, validating the existence of
> the directory and overwriting the default set by virQEMUDriverConfigNew.
>
> Also update the qemu.conf description for default to indicate the consequences
> if the default directory does not exist.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu.conf | 9 ++++++++-
> src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
> 2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index e6c0832..737fa46 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,13 @@
> #
> # 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 directory alter the following. If the
> +# provided directory does not exist, then the setting reverts back to the
> +# default /etc/pki/qemu.
> +#
I don't think this is a good idea. We should use the directory a user
specified in qemu.conf. If it doesn't exist, well things won't work.
Sure, we can complain about it in the logs, but we should not fallback
to any magic default in that case. Anyone setting a custom directory for
TLS certificates does this because they want to use it. If the directory
does not exist, it's either because they forgot to create it or they
made a typo somewhere. It's very unlikely someone actually wants to use
a default directory even though they set a custom one.
NACK
Jirka
It's simple enough to fail, but that's in code which you snipped and not
in text description which essentially matched what the code is/was
doing. Your feeling is then that :
+ if (!virFileExists(tlsCertDir)) {
+ VIR_INFO("%s, directory '%s' does not exist, retain default",
+ setting, tlsCertDir);
+ VIR_FREE(tlsCertDir);
should fail at libvirtd startup instead of a VIR_INFO, true? Is that
true for default, vnc, spice, chardev, and migrate?
That's different from the current environment which only would fail for
domains that use TLS which is why I was hesitant to make it fail. Also
note that if /etc/pki/qemu doesn't exist and someone used a TLS marker,
then even without changing any of the *cert_dir values, only the domain
will fail to start. That is, /etc/pki/qemu is not checked for existence
and startup failure.
Altering text in qemu.conf to match what really happens will be simple,
but ensuring the approach is agreed upon is what I need to clear up.
Tks -
John
5:40 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check for existence of provided *_tls_x509_cert_dir
On Thu, Jun 29, 2017 at 09:24:30AM +0200, Jiri Denemark wrote:
On Wed, Jun 28, 2017 at 15:30:28 -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. Then if a setting was provided, validating the existence of
> the directory and overwriting the default set by virQEMUDriverConfigNew.
>
> Also update the qemu.conf description for default to indicate the consequences
> if the default directory does not exist.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu.conf | 9 ++++++++-
> src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
> 2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index e6c0832..737fa46 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,13 @@
> #
> # 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 directory alter the following. If the
> +# provided directory does not exist, then the setting reverts back to the
> +# default /etc/pki/qemu.
> +#
I don't think this is a good idea. We should use the directory a user
specified in qemu.conf. If it doesn't exist, well things won't work.
Sure, we can complain about it in the logs, but we should not fallback
to any magic default in that case. Anyone setting a custom directory for
TLS certificates does this because they want to use it. If the directory
does not exist, it's either because they forgot to create it or they
made a typo somewhere. It's very unlikely someone actually wants to use
a default directory even though they set a custom one.
NACK
Agreed, I think we need to distinguish between the default dirs for each
settings, vs user specified dir for each setting.
ie, if the user has *not* set 'chardev_tls_x509_cert_dir' then its default
value is '/etc/pki/libvirt-chardev'. If that directory does not exist,
then falling back to "default_tls_x509_cert_dir" is good.
If the user *has* set 'chardev_tls_x509_cert_dir' and it does nto exist,
then we should report an hard error, preferrably at startup so the admin
sees their mistake immediately.
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 :|
6:45 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check for existence of provided *_tls_x509_cert_dir
On 06/29/2017 06:40 AM, Daniel P. Berrange wrote:
On Thu, Jun 29, 2017 at 09:24:30AM +0200, Jiri Denemark wrote:
> On Wed, Jun 28, 2017 at 15:30:28 -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. Then if a setting was provided, validating the existence of
>> the directory and overwriting the default set by virQEMUDriverConfigNew.
>>
>> Also update the qemu.conf description for default to indicate the consequences
>> if the default directory does not exist.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/qemu/qemu.conf | 9 ++++++++-
>> src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
>> 2 files changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index e6c0832..737fa46 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,13 @@
>> #
>> # 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 directory alter the following. If the
>> +# provided directory does not exist, then the setting reverts back to the
>> +# default /etc/pki/qemu.
>> +#
>
> I don't think this is a good idea. We should use the directory a user
> specified in qemu.conf. If it doesn't exist, well things won't work.
> Sure, we can complain about it in the logs, but we should not fallback
> to any magic default in that case. Anyone setting a custom directory for
> TLS certificates does this because they want to use it. If the directory
> does not exist, it's either because they forgot to create it or they
> made a typo somewhere. It's very unlikely someone actually wants to use
> a default directory even though they set a custom one.
>
> NACK
Agreed, I think we need to distinguish between the default dirs for each
settings, vs user specified dir for each setting.
ie, if the user has *not* set 'chardev_tls_x509_cert_dir' then its default
value is '/etc/pki/libvirt-chardev'. If that directory does not exist,
then falling back to "default_tls_x509_cert_dir" is good.
This essentially what happens in virQEMUDriverConfigNew. The caveat here
is that the default value for default_tls_x509_cert_dir (e.g.
/etc/pki/qemu) is not checked for existence, rather it's assumed.
If the user *has* set 'chardev_tls_x509_cert_dir' and it does nto exist,
then we should report an hard error, preferrably at startup so the admin
sees their mistake immediately.
OK and that answers most of the questions I had... If the user changes
"default_*" (as processed in virQEMUDriverConfigLoadFile) and it does
not exist, then should startup fail?
John
Regards,
Daniel
6:49 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check for existence of provided *_tls_x509_cert_dir
On Thu, Jun 29, 2017 at 07:45:06AM -0400, John Ferlan wrote:
On 06/29/2017 06:40 AM, Daniel P. Berrange wrote:
> On Thu, Jun 29, 2017 at 09:24:30AM +0200, Jiri Denemark wrote:
>> On Wed, Jun 28, 2017 at 15:30:28 -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. Then if a setting was provided, validating the existence of
>>> the directory and overwriting the default set by virQEMUDriverConfigNew.
>>>
>>> Also update the qemu.conf description for default to indicate the
consequences
>>> if the default directory does not exist.
>>>
>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>>> ---
>>> src/qemu/qemu.conf | 9 ++++++++-
>>> src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
>>> 2 files changed, 42 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>>> index e6c0832..737fa46 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,13 @@
>>> #
>>> # 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 directory alter the following. If the
>>> +# provided directory does not exist, then the setting reverts back to the
>>> +# default /etc/pki/qemu.
>>> +#
>>
>> I don't think this is a good idea. We should use the directory a user
>> specified in qemu.conf. If it doesn't exist, well things won't work.
>> Sure, we can complain about it in the logs, but we should not fallback
>> to any magic default in that case. Anyone setting a custom directory for
>> TLS certificates does this because they want to use it. If the directory
>> does not exist, it's either because they forgot to create it or they
>> made a typo somewhere. It's very unlikely someone actually wants to use
>> a default directory even though they set a custom one.
>>
>> NACK
>
> Agreed, I think we need to distinguish between the default dirs for each
> settings, vs user specified dir for each setting.
>
> ie, if the user has *not* set 'chardev_tls_x509_cert_dir' then its default
> value is '/etc/pki/libvirt-chardev'. If that directory does not exist,
> then falling back to "default_tls_x509_cert_dir" is good.
This essentially what happens in virQEMUDriverConfigNew. The caveat here
is that the default value for default_tls_x509_cert_dir (e.g.
/etc/pki/qemu) is not checked for existence, rather it's assumed.
As long as we get an error message it is ok, but it might be wise to
explicitly check /etc/pki/qemu if it would give a better error
message to the user.
> If the user *has* set 'chardev_tls_x509_cert_dir' and it
does nto exist,
> then we should report an hard error, preferrably at startup so the admin
> sees their mistake immediately.
OK and that answers most of the questions I had... If the user changes
"default_*" (as processed in virQEMUDriverConfigLoadFile) and it does
not exist, then should startup fail?
Yep
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 :|
7:04 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check for existence of provided *_tls_x509_cert_dir
On 06/29/2017 07:49 AM, Daniel P. Berrange wrote:
On Thu, Jun 29, 2017 at 07:45:06AM -0400, John Ferlan wrote:
>
>
> On 06/29/2017 06:40 AM, Daniel P. Berrange wrote:
>> On Thu, Jun 29, 2017 at 09:24:30AM +0200, Jiri Denemark wrote:
>>> On Wed, Jun 28, 2017 at 15:30:28 -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. Then if a setting was provided, validating the existence of
>>>> the directory and overwriting the default set by virQEMUDriverConfigNew.
>>>>
>>>> Also update the qemu.conf description for default to indicate the
consequences
>>>> if the default directory does not exist.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>>>> ---
>>>> src/qemu/qemu.conf | 9 ++++++++-
>>>> src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
>>>> 2 files changed, 42 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>>>> index e6c0832..737fa46 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,13 @@
>>>> #
>>>> # 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 directory alter the following. If
the
>>>> +# provided directory does not exist, then the setting reverts back to
the
>>>> +# default /etc/pki/qemu.
>>>> +#
>>>
>>> I don't think this is a good idea. We should use the directory a user
>>> specified in qemu.conf. If it doesn't exist, well things won't work.
>>> Sure, we can complain about it in the logs, but we should not fallback
>>> to any magic default in that case. Anyone setting a custom directory for
>>> TLS certificates does this because they want to use it. If the directory
>>> does not exist, it's either because they forgot to create it or they
>>> made a typo somewhere. It's very unlikely someone actually wants to use
>>> a default directory even though they set a custom one.
>>>
>>> NACK
>>
>> Agreed, I think we need to distinguish between the default dirs for each
>> settings, vs user specified dir for each setting.
>>
>> ie, if the user has *not* set 'chardev_tls_x509_cert_dir' then its
default
>> value is '/etc/pki/libvirt-chardev'. If that directory does not exist,
>> then falling back to "default_tls_x509_cert_dir" is good.
>
> This essentially what happens in virQEMUDriverConfigNew. The caveat here
> is that the default value for default_tls_x509_cert_dir (e.g.
> /etc/pki/qemu) is not checked for existence, rather it's assumed.
As long as we get an error message it is ok, but it might be wise to
explicitly check /etc/pki/qemu if it would give a better error
message to the user.
Currently we'd get an error "eventually" when someone tries to start a
guest:
error: internal error: process exited while connecting to monitor:
2017-06-28T19:24:18.810938Z qemu-system-x86_64: -object
tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/qemu,endpoint=client,verify-peer=no:
Unable to access credentials /etc/pki/qemu/ca-cert.pem: No such file or
directory
The problem with checking and generating an error at startup is that we
don't create the /etc/pki/qemu directory during install (at least as far
as I can tell). If we do, then something's broken because I have a
couple of environments without it.
John
>> If the user *has* set 'chardev_tls_x509_cert_dir' and
it does nto exist,
>> then we should report an hard error, preferrably at startup so the admin
>> sees their mistake immediately.
>
> OK and that answers most of the questions I had... If the user changes
> "default_*" (as processed in virQEMUDriverConfigLoadFile) and it does
> not exist, then should startup fail?
Yep
Regards,
Daniel
2706
days inactive
2707
days old
11 comments
5 participants
participants (5)
-
Andrea Bolognani -
Daniel P. Berrange -
Jiri Denemark -
John Ferlan -
Peter Krempa