[libvirt] [PATCHv3 0/6] flags cleanup conclusion
by Eric Blake
With this, I think I've addressed all the review comments from v2:
https://www.redhat.com/archives/libvir-list/2011-July/msg00460.html
as well as a side discussion on public API flag filtering:
https://www.redhat.com/archives/libvir-list/2011-July/msg00833.html
Eric Blake (6):
flags: use common dumpxml flags check
flags: simplify qemu migration flags
esx: reject unknown flags
xen: reject unknown flags
libvirt: do not mix internal flags into public API
build: add syntax check for proper flags use
cfg.mk | 30 ++++++++++--
src/conf/domain_conf.c | 25 +++++++--
src/conf/domain_conf.h | 10 ----
src/driver.c | 4 --
src/driver.h | 13 ++---
src/esx/esx_device_monitor.c | 4 +-
src/esx/esx_driver.c | 35 ++++++++++---
src/esx/esx_interface_driver.c | 4 +-
src/esx/esx_network_driver.c | 4 +-
src/esx/esx_nwfilter_driver.c | 4 +-
src/esx/esx_secret_driver.c | 4 +-
src/esx/esx_storage_driver.c | 4 +-
src/fdstream.c | 28 +++++-----
src/fdstream.h | 6 +-
src/libvirt.c | 6 +--
src/libxl/libxl_driver.c | 2 +
src/locking/lock_driver_nop.c | 10 ++--
src/locking/lock_manager.c | 7 ++-
src/lxc/lxc_driver.c | 2 +
src/openvz/openvz_driver.c | 2 +
src/phyp/phyp_driver.c | 2 +
src/qemu/qemu_driver.c | 107 +++++++++-------------------------------
src/qemu/qemu_process.c | 2 +-
src/remote/remote_driver.c | 8 +++-
src/secret/secret_driver.c | 7 ++-
src/storage/storage_driver.c | 2 +-
src/test/test_driver.c | 2 +
src/uml/uml_driver.c | 24 ++++++---
src/util/command.c | 2 +-
src/util/iohelper.c | 18 +++---
src/util/util.c | 14 +++---
src/vbox/vbox_tmpl.c | 18 +++++--
src/vmware/vmware_driver.c | 2 +
src/xen/xen_driver.c | 18 ++++++-
src/xen/xen_driver.h | 7 +++
src/xen/xen_hypervisor.c | 8 ++-
src/xen/xen_inotify.c | 4 +-
src/xen/xend_internal.c | 42 +++++++++++++---
src/xen/xend_internal.h | 3 +-
src/xen/xm_internal.c | 33 ++++++++++---
src/xen/xm_internal.h | 2 +-
src/xen/xs_internal.c | 12 +++-
src/xenapi/xenapi_driver.c | 4 +-
43 files changed, 323 insertions(+), 222 deletions(-)
--
1.7.4.4
13 years, 4 months
[libvirt] [PATCH] virsh: improve option handling
by Eric Blake
The documentation for vshCommandOptString claims that it returns
-1 on a missing required argument, but in reality, that error
message was unreachable (it was buried inside an if clause that
is true only if the argument was present). The code was so hairy
that I decided a rewrite would make it easier to understand,
and actually return the error values we want. In the process,
this guarantees that --option '' will either return -1 or 1,
depending on whether EMPTY_OK was set, reserving 0 solely
for the case of an option not present and not required.
Meanwhile, our construction guarantees that all vshCmdOpt have
a non-null def member, so there are some redundant checks that
can be trimmed.
* tools/virsh.c (vshCommandOpt): Alter signature.
(vshCommandOptInt, vshCommandOptUInt, vshCommandOptUL)
(vshCommandOptString, vshCommandOptLongLong)
(vshCommandOptULongLong, vshCommandOptBool): Adjust all callers.
---
This patch replaces the one mentioned here:
https://www.redhat.com/archives/libvir-list/2011-July/msg00995.html
tools/virsh.c | 302 +++++++++++++++++++++++++++++++++++++++------------------
1 files changed, 206 insertions(+), 96 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index b8f357b..a4a9445 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -162,29 +162,36 @@ typedef struct __vshControl vshControl;
typedef struct __vshCmd vshCmd;
/*
- * vshCmdInfo -- information about command
+ * vshCmdInfo -- name/value pair for information about command
+ *
+ * Commands should have at least the following names:
+ * "name" - command name
+ * "desc" - description of command, or empty string
*/
typedef struct {
- const char *name; /* name of information */
- const char *data; /* information */
+ const char *name; /* name of information, or NULL for list end */
+ const char *data; /* non-NULL information */
} vshCmdInfo;
/*
* vshCmdOptDef - command option definition
*/
typedef struct {
- const char *name; /* the name of option */
+ const char *name; /* the name of option, or NULL for list end */
vshCmdOptType type; /* option type */
int flag; /* flags */
- const char *help; /* help string */
+ const char *help; /* non-NULL help string */
} vshCmdOptDef;
/*
* vshCmdOpt - command options
+ *
+ * After parsing a command, all arguments to the command have been
+ * collected into a list of these objects.
*/
typedef struct vshCmdOpt {
- const vshCmdOptDef *def; /* pointer to relevant option */
- char *data; /* allocated data */
+ const vshCmdOptDef *def; /* non-NULL pointer to option definition */
+ char *data; /* allocated data, or NULL for bool option */
struct vshCmdOpt *next;
} vshCmdOpt;
@@ -199,7 +206,7 @@ enum {
* vshCmdDef - command definition
*/
typedef struct {
- const char *name;
+ const char *name; /* name of command, or NULL for list end */
bool (*handler) (vshControl *, const vshCmd *); /* command handler */
const vshCmdOptDef *opts; /* definition of command options */
const vshCmdInfo *info; /* details about command */
@@ -239,7 +246,7 @@ typedef struct __vshControl {
} __vshControl;
typedef struct vshCmdGrp {
- const char *name;
+ const char *name; /* name of group, or NULL for list end */
const char *keyword; /* help keyword */
const vshCmdDef *commands;
} vshCmdGrp;
@@ -264,7 +271,9 @@ static bool vshCmddefHelp(vshControl *ctl, const char *name);
static const vshCmdGrp *vshCmdGrpSearch(const char *grpname);
static bool vshCmdGrpHelp(vshControl *ctl, const char *name);
-static vshCmdOpt *vshCommandOpt(const vshCmd *cmd, const char *name);
+static int vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *value)
ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
static int vshCommandOptUInt(const vshCmd *cmd, const char *name,
@@ -12300,23 +12309,48 @@ vshCommandFree(vshCmd *cmd)
}
}
-/*
- * Returns option by name
+/**
+ * vshCommandOpt:
+ * @cmd: parsed command line to search
+ * @name: option name to search for
+ * @opt: result of the search
+ *
+ * Look up an option passed to CMD by NAME. Returns 1 with *OPT set
+ * to the option if found, 0 with *OPT set to NULL if the name is
+ * valid and the option is not required, -1 with *OPT set to NULL if
+ * the option is required but not present, and -2 if NAME is not valid
+ * (-2 indicates a programming error). No error messages are issued.
*/
-static vshCmdOpt *
-vshCommandOpt(const vshCmd *cmd, const char *name)
+static int
+vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt)
{
- vshCmdOpt *opt = cmd->opts;
+ vshCmdOpt *candidate = cmd->opts;
+ const vshCmdOptDef *valid = cmd->def->opts;
- while (opt) {
- if (opt->def && STREQ(opt->def->name, name))
- return opt;
- opt = opt->next;
+ /* See if option is present on command line. */
+ while (candidate) {
+ if (STREQ(candidate->def->name, name)) {
+ *opt = candidate;
+ return 1;
+ }
+ candidate = candidate->next;
}
- return NULL;
+
+ /* Option not present, see if command requires it. */
+ *opt = NULL;
+ while (valid) {
+ if (!valid->name)
+ break;
+ if (STREQ(name, valid->name))
+ return (valid->flag & VSH_OFLAG_REQ) == 0 ? 0 : -1;
+ valid++;
+ }
+ /* If we got here, the name is unknown. */
+ return -2;
}
-/*
+/**
+ * vshCommandOptInt:
* @cmd command reference
* @name option name
* @value result
@@ -12324,102 +12358,144 @@ vshCommandOpt(const vshCmd *cmd, const char *name)
* Convert option to int
* Return value:
* >0 if option found and valid (@value updated)
- * 0 in case option not found (@value untouched)
+ * 0 if option not found and not required (@value untouched)
* <0 in all other cases (@value untouched)
*/
static int
vshCommandOptInt(const vshCmd *cmd, const char *name, int *value)
{
- vshCmdOpt *arg = vshCommandOpt(cmd, name);
- int ret = 0, num;
+ vshCmdOpt *arg;
+ int ret;
+ int num;
char *end_p = NULL;
- if ((arg != NULL) && (arg->data != NULL)) {
- num = strtol(arg->data, &end_p, 10);
- ret = -1;
- if ((arg->data != end_p) && (*end_p == 0)) {
- *value = num;
- ret = 1;
- }
+ ret = vshCommandOpt(cmd, name, &arg);
+ if (ret <= 0)
+ return ret;
+ if (!arg->data) {
+ /* only possible on bool, but if name is bool, this is a
+ * programming bug */
+ return -2;
}
- return ret;
+
+ num = strtol(arg->data, &end_p, 10);
+ if ((arg->data != end_p) && (*end_p == 0)) {
+ *value = num;
+ return 1;
+ }
+ return -1;
}
-/*
+/**
+ * vshCommandOptUInt:
+ * @cmd command reference
+ * @name option name
+ * @value result
+ *
* Convert option to unsigned int
* See vshCommandOptInt()
*/
static int
vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value)
{
- vshCmdOpt *arg = vshCommandOpt(cmd, name);
- unsigned int ret = 0, num;
+ vshCmdOpt *arg;
+ int ret;
+ unsigned int num;
char *end_p = NULL;
- if ((arg != NULL) && (arg->data != NULL)) {
- num = strtoul(arg->data, &end_p, 10);
- ret = -1;
- if ((arg->data != end_p) && (*end_p == 0)) {
- *value = num;
- ret = 1;
- }
+ ret = vshCommandOpt(cmd, name, &arg);
+ if (ret <= 0)
+ return ret;
+ if (!arg->data) {
+ /* only possible on bool, but if name is bool, this is a
+ * programming bug */
+ return -2;
}
- return ret;
+
+ num = strtoul(arg->data, &end_p, 10);
+ if ((arg->data != end_p) && (*end_p == 0)) {
+ *value = num;
+ return 1;
+ }
+ return -1;
}
/*
+ * vshCommandOptUL:
+ * @cmd command reference
+ * @name option name
+ * @value result
+ *
* Convert option to unsigned long
* See vshCommandOptInt()
*/
static int
vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value)
{
- vshCmdOpt *arg = vshCommandOpt(cmd, name);
- int ret = 0;
+ vshCmdOpt *arg;
+ int ret;
unsigned long num;
char *end_p = NULL;
- if ((arg != NULL) && (arg->data != NULL)) {
- num = strtoul(arg->data, &end_p, 10);
- ret = -1;
- if ((arg->data != end_p) && (*end_p == 0)) {
- *value = num;
- ret = 1;
- }
+ ret = vshCommandOpt(cmd, name, &arg);
+ if (ret <= 0)
+ return ret;
+ if (!arg->data) {
+ /* only possible on bool, but if name is bool, this is a
+ * programming bug */
+ return -2;
}
- return ret;
+
+ num = strtoul(arg->data, &end_p, 10);
+ if ((arg->data != end_p) && (*end_p == 0)) {
+ *value = num;
+ return 1;
+ }
+ return -1;
}
-/*
+/**
+ * vshCommandOptString:
+ * @cmd command reference
+ * @name option name
+ * @value result
+ *
* Returns option as STRING
- * See vshCommandOptInt()
+ * Return value:
+ * >0 if option found and valid (@value updated)
+ * 0 if option not found and not required (@value untouched)
+ * <0 in all other cases (@value untouched)
*/
static int
vshCommandOptString(const vshCmd *cmd, const char *name, const char **value)
{
- vshCmdOpt *arg = vshCommandOpt(cmd, name);
- int ret = 0;
-
- if (arg && arg->data) {
- if (*arg->data
- || (arg->def && (arg->def->flag & VSH_OFLAG_EMPTY_OK))) {
- *value = arg->data;
- ret = 1;
- } else if (arg->def && ((arg->def->flag) & VSH_OFLAG_REQ)) {
- vshError(NULL, _("Missing required option '%s'"), name);
- ret = -1;
- } else {
- /* Treat "--option ''" as if option had not been specified. */
- ret = 0;
- }
+ vshCmdOpt *arg;
+ int ret;
+
+ ret = vshCommandOpt(cmd, name, &arg);
+ if (ret <= 0)
+ return ret;
+ if (!arg->data) {
+ /* only possible on bool, but if name is bool, this is a
+ * programming bug */
+ return -2;
}
- return ret;
+ if (!*arg->data && !(arg->def->flag & VSH_OFLAG_EMPTY_OK)) {
+ return -1;
+ }
+ *value = arg->data;
+ return 1;
}
-/*
+/**
+ * vshCommandOptLongLong:
+ * @cmd command reference
+ * @name option name
+ * @value result
+ *
* Returns option as long long
* See vshCommandOptInt()
*/
@@ -12427,53 +12503,87 @@ static int
vshCommandOptLongLong(const vshCmd *cmd, const char *name,
long long *value)
{
- vshCmdOpt *arg = vshCommandOpt(cmd, name);
- int ret = 0;
+ vshCmdOpt *arg;
+ int ret;
long long num;
char *end_p = NULL;
- if ((arg != NULL) && (arg->data != NULL)) {
- num = strtoll(arg->data, &end_p, 10);
- ret = -1;
- if ((arg->data != end_p) && (*end_p == 0)) {
- *value = num;
- ret = 1;
- }
+ ret = vshCommandOpt(cmd, name, &arg);
+ if (ret <= 0)
+ return ret;
+ if (!arg->data) {
+ /* only possible on bool, but if name is bool, this is a
+ * programming bug */
+ return -2;
}
- return ret;
+
+ num = strtoll(arg->data, &end_p, 10);
+ if ((arg->data != end_p) && (*end_p == 0)) {
+ *value = num;
+ return 1;
+ }
+ return -1;
}
+/**
+ * vshCommandOptULongLong:
+ * @cmd command reference
+ * @name option name
+ * @value result
+ *
+ * Returns option as long long
+ * See vshCommandOptInt()
+ */
static int
vshCommandOptULongLong(const vshCmd *cmd, const char *name,
unsigned long long *value)
{
- vshCmdOpt *arg = vshCommandOpt(cmd, name);
- int ret = 0;
+ vshCmdOpt *arg;
+ int ret;
unsigned long long num;
char *end_p = NULL;
- if ((arg != NULL) && (arg->data != NULL)) {
- num = strtoull(arg->data, &end_p, 10);
- ret = -1;
- if ((arg->data != end_p) && (*end_p == 0)) {
- *value = num;
- ret = 1;
- }
+ ret = vshCommandOpt(cmd, name, &arg);
+ if (ret <= 0)
+ return ret;
+ if (!arg->data) {
+ /* only possible on bool, but if name is bool, this is a
+ * programming bug */
+ return -2;
}
- return ret;
+
+ num = strtoull(arg->data, &end_p, 10);
+ if ((arg->data != end_p) && (*end_p == 0)) {
+ *value = num;
+ return 1;
+ }
+ return -1;
}
-/*
- * Returns true/false if the option exists
+/**
+ * vshCommandOptBool:
+ * @cmd command reference
+ * @name option name
+ *
+ * Returns true/false if the option exists. Note that this does NOT
+ * validate whether the option is actually boolean, or even whether
+ * name is legal; so that this can be used to probe whether a data
+ * option is present without actually using that data.
*/
static bool
vshCommandOptBool(const vshCmd *cmd, const char *name)
{
- return vshCommandOpt(cmd, name) != NULL;
+ vshCmdOpt *dummy;
+
+ return vshCommandOpt(cmd, name, &dummy) == 1;
}
-/*
+/**
+ * vshCommandOptArgv:
+ * @cmd command reference
+ * @opt starting point for the search
+ *
* Returns the next argv argument after OPT (or the first one if OPT
* is NULL), or NULL if no more are present.
*
--
1.7.4.4
13 years, 4 months
[libvirt] [PATCH] Add sanity checking of basic contraints, key purpose & key usage
by Daniel P. Berrange
Gnutls requires that certificates have basic constraints present
to be used as a CA certificate. OpenSSL doesn't add this data
by default, so add a sanity check to catch this situation. Also
validate that the key usage and key purpose constraints contain
correct data
* src/rpc/virnettlscontext.c: Add sanity checking of certificate
constraints
---
src/rpc/virnettlscontext.c | 130 ++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 126 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 6a06565..7f088d2 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -98,6 +98,7 @@ static void virNetTLSLog(int level, const char *str) {
static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
+ bool isCA,
const char *certFile)
{
gnutls_datum_t data;
@@ -105,6 +106,14 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
char *buf = NULL;
int ret = -1;
time_t now;
+ int status;
+ int i;
+ char *buffer = NULL;
+ size_t size;
+ unsigned int usage;
+
+ VIR_DEBUG("isServer %d isCA %d certFile %s",
+ isServer, isCA, certFile);
if ((now = time(NULL)) == ((time_t)-1)) {
virReportSystemError(errno, "%s",
@@ -145,6 +154,117 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
goto cleanup;
}
+ status = gnutls_x509_crt_get_basic_constraints(cert, NULL, NULL, NULL);
+ VIR_DEBUG("Cert %s basic constraints %d", certFile, status);
+
+ if (status > 0) { /* It is a CA cert */
+ if (!isCA) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("The certificate %s basic constraints show a CA, but we need one for a %s"),
+ certFile, isServer ? "server" : "client");
+ goto cleanup;
+ }
+ } else if (status == 0) { /* It is not a CA cert */
+ if (isCA) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("The certificate %s basic constraints do not show a CA"),
+ certFile);
+ goto cleanup;
+ }
+ } else if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { /* Missing basicConstraints */
+ if (isCA) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("The certificate %s is missing basic constraints for a CA"),
+ certFile);
+ goto cleanup;
+ }
+ } else { /* General error */
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Unable to query certificate %s basic constraints %s"),
+ certFile, gnutls_strerror(status));
+ goto cleanup;
+ }
+
+ status = gnutls_x509_crt_get_key_usage(cert, &usage, NULL);
+
+ VIR_DEBUG("Cert %s key usage status %d usage %d", certFile, status, usage);
+ if (status < 0) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Unable to query certificate %s basic constraints %s"),
+ certFile, gnutls_strerror(status));
+ goto cleanup;
+ }
+
+ if (usage & GNUTLS_KEY_KEY_CERT_SIGN) {
+ if (!isCA) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Certificate %s usage is for certificate signing, but wanted a %s certificate"),
+ certFile, isServer ? "server" : "client");
+ goto cleanup;
+ }
+ } else {
+ if (isCA) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Certificate %s usage is for not certificate signing"),
+ certFile);
+ goto cleanup;
+ }
+ }
+
+ for (i = 0 ; ; i++) {
+ size = 0;
+ status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL);
+
+ if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
+ VIR_DEBUG("No key purpose data available, skipping checks");
+ break;
+ }
+ if (status != GNUTLS_E_SHORT_MEMORY_BUFFER) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Unable to query certificate %s key purpose %s"),
+ certFile, gnutls_strerror(status));
+ goto cleanup;
+ }
+
+ if (VIR_ALLOC_N(buffer, size) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL);
+ if (status < 0) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Unable to query certificate %s key purpose %s"),
+ certFile, gnutls_strerror(status));
+ goto cleanup;
+ }
+
+ VIR_DEBUG("Key purpose %d %s", status, buffer);
+ if (STREQ(buffer, GNUTLS_KP_TLS_WWW_SERVER)) {
+ if (isCA || !isServer) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Certificate %s purpose is TLS server, but wanted a %s certificate"),
+ certFile, isCA ? "CA" : "TLS client");
+ goto cleanup;
+ }
+ } else if (STREQ(buffer, GNUTLS_KP_TLS_WWW_CLIENT)) {
+ if (isCA || isServer) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Certificate %s purpose is TLS client, but wanted a %s certificate"),
+ certFile, isCA ? "CA" : "TLS server");
+ goto cleanup;
+ }
+ } else if (STRNEQ(buffer, GNUTLS_KP_ANY)) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Certificate %s purpose is wrong, wanted a %s certificate"),
+ certFile, isCA ? "CA" : (isServer ? "TLS server" : "TLS client"));
+ goto cleanup;
+ }
+
+ VIR_FREE(buffer);
+ }
+
+
ret = 0;
cleanup:
@@ -152,6 +272,7 @@ cleanup:
gnutls_x509_crt_deinit(cert);
cert = NULL;
}
+ VIR_FREE(buffer);
VIR_FREE(buf);
return cert;
}
@@ -167,11 +288,11 @@ static int virNetTLSContextSanityCheckCredentials(bool isServer,
unsigned int status;
if (access(certFile, R_OK) == 0) {
- if (!(cert = virNetTLSContextSanityCheckCert(isServer, certFile)))
+ if (!(cert = virNetTLSContextSanityCheckCert(isServer, false, certFile)))
goto cleanup;
}
if (access(cacertFile, R_OK) == 0) {
- if (!(cacert = virNetTLSContextSanityCheckCert(isServer, cacertFile)))
+ if (!(cacert = virNetTLSContextSanityCheckCert(isServer, true, cacertFile)))
goto cleanup;
}
@@ -343,9 +464,10 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert,
goto error;
}
- if (virNetTLSContextSanityCheckCredentials(isServer, cacert, cert) < 0)
+ if (requireValidCert &&
+ virNetTLSContextSanityCheckCredentials(isServer, cacert, cert) < 0)
goto error;
-
+
if (virNetTLSContextLoadCredentials(ctxt, isServer, cacert, cacrl, cert, key) < 0)
goto error;
--
1.7.4.4
13 years, 4 months
[libvirt] [PATCH] Add some basic sanity checking of certificates before use
by Daniel P. Berrange
If the libvirt daemon or libvirt client is configured with bogus
certificates, it is very unhelpful to only find out about this
when a TLS connection is actually attempted. Not least because
the error messages you get back for failures are incredibly
obscure.
This adds some basic sanity checking of certificates at the
time the virNetTLSContext object is created. This is at libvirt
startup, or when creating a virNetClient instance.
This checks that the certificate expiry/start dates are valid
and that the certificate is actually signed by the CA that is
loaded.
* src/rpc/virnettlscontext.c: Add certificate sanity checks
---
src/rpc/virnettlscontext.c | 144 ++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 140 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 1120e1e..b778550 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -67,6 +67,7 @@ struct _virNetTLSSession {
bool handshakeComplete;
+ bool isServer;
char *hostname;
gnutls_session_t session;
virNetTLSSessionWriteFunc writeFunc;
@@ -95,6 +96,131 @@ static void virNetTLSLog(int level, const char *str) {
VIR_DEBUG("%d %s", level, str);
}
+
+static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
+ const char *certFile)
+{
+ gnutls_datum_t data;
+ gnutls_x509_crt_t cert = NULL;
+ char *buf = NULL;
+ int ret = -1;
+ time_t now;
+
+ if ((now = time(NULL)) == ((time_t)-1)) {
+ virReportSystemError(errno, "%s",
+ _("cannot get current time"));
+ goto cleanup;
+ }
+
+ if (gnutls_x509_crt_init(&cert) < 0) {
+ virNetError(VIR_ERR_SYSTEM_ERROR, "%s",
+ _("Unable to initialize certificate"));
+ goto cleanup;
+ }
+
+ if (virFileReadAll(certFile, (1<<16), &buf) < 0)
+ goto cleanup;
+
+ data.data = (unsigned char *)buf;
+ data.size = strlen(buf);
+
+ if (gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM) < 0) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Unable to import %s certificate %s"),
+ isServer ? "server" : "client", certFile);
+ goto cleanup;
+ }
+
+ if (gnutls_x509_crt_get_expiration_time(cert) < now) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("The %s certificate %s has expired"),
+ isServer ? "server" : "client", certFile);
+ goto cleanup;
+ }
+
+ if (gnutls_x509_crt_get_activation_time(cert) > now) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("The %s certificate %s is not yet active"),
+ isServer ? "server" : "client", certFile);
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ if (ret != 0) {
+ gnutls_x509_crt_deinit(cert);
+ cert = NULL;
+ }
+ VIR_FREE(buf);
+ return cert;
+}
+
+
+static int virNetTLSContextSanityCheckCredentials(bool isServer,
+ const char *cacertFile,
+ const char *certFile)
+{
+ gnutls_x509_crt_t cert = NULL;
+ gnutls_x509_crt_t cacert = NULL;
+ int ret = -1;
+ unsigned int status;
+
+ if (access(certFile, R_OK) == 0) {
+ if (!(cert = virNetTLSContextSanityCheckCert(isServer, certFile)))
+ goto cleanup;
+ }
+ if (access(cacertFile, R_OK) == 0) {
+ if (!(cacert = virNetTLSContextSanityCheckCert(isServer, cacertFile)))
+ goto cleanup;
+ }
+
+ if (cert && cacert) {
+ if (gnutls_x509_crt_list_verify(&cert, 1,
+ &cacert, 1,
+ NULL, 0,
+ 0, &status) < 0) {
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Unable to verify %s certificate against CA certificate"),
+ isServer ? "server": "client");
+ goto cleanup;
+ }
+
+ if (status != 0) {
+ const char *reason = _("Invalid certificate");
+
+ if (status & GNUTLS_CERT_INVALID)
+ reason = _("The certificate is not trusted.");
+
+ if (status & GNUTLS_CERT_SIGNER_NOT_FOUND)
+ reason = _("The certificate hasn't got a known issuer.");
+
+ if (status & GNUTLS_CERT_REVOKED)
+ reason = _("The certificate has been revoked.");
+
+#ifndef GNUTLS_1_0_COMPAT
+ if (status & GNUTLS_CERT_INSECURE_ALGORITHM)
+ reason = _("The certificate uses an insecure algorithm");
+#endif
+
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("Our own certificate %s failed validation against %s: %s"),
+ certFile, cacertFile, reason);
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+
+cleanup:
+ if (cert)
+ gnutls_x509_crt_deinit(cert);
+ if (cacert)
+ gnutls_x509_crt_deinit(cacert);
+ return ret;
+}
+
+
static int virNetTLSContextLoadCredentials(virNetTLSContextPtr ctxt,
bool isServer,
const char *cacert,
@@ -217,6 +343,9 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert,
goto error;
}
+ if (virNetTLSContextSanityCheckCredentials(isServer, cacert, cert) < 0)
+ goto error;
+
if (virNetTLSContextLoadCredentials(ctxt, isServer, cacert, cacrl, cert, key) < 0)
goto error;
@@ -574,15 +703,20 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt,
}
if (gnutls_x509_crt_get_expiration_time(cert) < now) {
- virNetError(VIR_ERR_SYSTEM_ERROR, "%s",
- _("The client certificate has expired"));
+ /* Warning is reversed from what you expect, since with
+ * this code it is the Server checking the client and
+ * vica-verca */
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("The %s certificate has expired"),
+ sess->isServer ? "client" : "server");
gnutls_x509_crt_deinit(cert);
goto authdeny;
}
if (gnutls_x509_crt_get_activation_time(cert) > now) {
- virNetError(VIR_ERR_SYSTEM_ERROR, "%s",
- _("The client certificate is not yet active"));
+ virNetError(VIR_ERR_SYSTEM_ERROR,
+ _("The %s certificate is not yet active"),
+ sess->isServer ? "client" : "server");
gnutls_x509_crt_deinit(cert);
goto authdeny;
}
@@ -756,6 +890,8 @@ virNetTLSSessionPtr virNetTLSSessionNew(virNetTLSContextPtr ctxt,
gnutls_transport_set_pull_function(sess->session,
virNetTLSSessionPull);
+ sess->isServer = ctxt->isServer;
+
return sess;
error:
--
1.7.4.4
13 years, 4 months
[libvirt] [PATCH] Fix now dead cleanup of VMs on libvirtd restart
by Daniel P. Berrange
From: "Daniel P. Berrange" <berrange(a)redhat.com>
When libvirtd restarts it will attempt to reconnect to existing
LXC containers. If it loads a XML state file for the container
the container will appear running. If we fail to read the PID
file, or fail to connect to the LXC monitor, we should be killing
off the guest, but if the VMs cgroup does not exist any more,
cleanup will get skipped. Reading the PID file is also pointless
since the PID is in the XML statefile
In lxcReconnectVM we do not need to read the PID file. If part
of the reconnect process fails we need to run the VM terminate
code as a safety net.
In lxcVMTerminate, if we can't obtain the VM cgroup, we know
the process has died, but we must still run lxcVMCleanup to
clear out the virDomainObjPtr live state
* src/lxc/lxc_driver.c: Fix cleanup of dead VMs on restart
---
src/lxc/lxc_driver.c | 60 +++++++++++++++++++++++++------------------------
1 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index c8d78d5..03360e5 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1340,22 +1340,26 @@ static int lxcVmTerminate(lxc_driver_t *driver,
return -1;
}
- if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0)
- return -1;
-
- rc = virCgroupKillPainfully(group);
- if (rc < 0) {
- virReportSystemError(-rc, "%s",
- _("Failed to kill container PIDs"));
- rc = -1;
- goto cleanup;
- }
- if (rc == 1) {
- lxcError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Some container PIDs refused to die"));
- rc = -1;
- goto cleanup;
+ if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) == 0) {
+ rc = virCgroupKillPainfully(group);
+ if (rc < 0) {
+ virReportSystemError(-rc, "%s",
+ _("Failed to kill container PIDs"));
+ rc = -1;
+ goto cleanup;
+ }
+ if (rc == 1) {
+ lxcError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Some container PIDs refused to die"));
+ rc = -1;
+ goto cleanup;
+ }
+ } else {
+ /* If cgroup doesn't exist, the VM pids must have already
+ * died and so we're just cleaning up stale state
+ */
}
+
lxcVmCleanup(driver, vm, reason);
rc = 0;
@@ -2172,32 +2176,24 @@ lxcReconnectVM(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque)
lxcDomainObjPrivatePtr priv;
virDomainObjLock(vm);
+ VIR_DEBUG("Reconnect %d %d %d\n", vm->def->id, vm->pid, vm->state.state);
priv = vm->privateData;
- if ((priv->monitor = lxcMonitorClient(driver, vm)) < 0) {
- goto cleanup;
- }
-
- /* Read pid from controller */
- if ((virFileReadPid(lxc_driver->stateDir, vm->def->name, &vm->pid)) != 0) {
- VIR_FORCE_CLOSE(priv->monitor);
- goto cleanup;
- }
if (vm->pid != 0) {
vm->def->id = vm->pid;
virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
VIR_DOMAIN_RUNNING_UNKNOWN);
+ if ((priv->monitor = lxcMonitorClient(driver, vm)) < 0)
+ goto error;
+
if ((priv->monitorWatch = virEventAddHandle(
priv->monitor,
VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP,
lxcMonitorEvent,
- vm, NULL)) < 0) {
- lxcVmTerminate(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
- virDomainAuditStop(vm, "failed");
- goto cleanup;
- }
+ vm, NULL)) < 0)
+ goto error;
} else {
vm->def->id = -1;
VIR_FORCE_CLOSE(priv->monitor);
@@ -2205,6 +2201,12 @@ lxcReconnectVM(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque)
cleanup:
virDomainObjUnlock(vm);
+ return;
+
+error:
+ lxcVmTerminate(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
+ virDomainAuditStop(vm, "failed");
+ goto cleanup;
}
--
1.7.6
13 years, 4 months
[libvirt] [PATCH] flags: fix domain_conf migration regression
by Eric Blake
Commit 461e0f1a broke migration, because there was a code path
that tried to enable an internal flag while still going through
the public function. Split the internal flag into a separate
callback, and validate that flags do not overlap.
* src/conf/domain_conf.c (virDomainDefFormat): Split...
(virDomainDefFormatInternal): ...to separate the flag check.
(virDomainObjFormat): Adjust caller.
---
Two regression in one day; my track record is not good right now.
I guess this goes to show I didn't test a migration :(
src/conf/domain_conf.c | 28 +++++++++++++++++++++-------
1 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 788981f..8c3e44e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9615,8 +9615,18 @@ virDomainHostdevDefFormat(virBufferPtr buf,
}
-char *virDomainDefFormat(virDomainDefPtr def,
- unsigned int flags)
+#define DUMPXML_FLAGS \
+ (VIR_DOMAIN_XML_SECURE | \
+ VIR_DOMAIN_XML_INACTIVE | \
+ VIR_DOMAIN_XML_UPDATE_CPU)
+
+verify((VIR_DOMAIN_XML_INTERNAL_STATUS & DUMPXML_FLAGS) == 0);
+
+/* This internal version can accept VIR_DOMAIN_XML_INTERNAL_STATUS,
+ * whereas the public version cannot. */
+static char *
+virDomainDefFormatInternal(virDomainDefPtr def,
+ unsigned int flags)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
unsigned char *uuid;
@@ -9624,9 +9634,7 @@ char *virDomainDefFormat(virDomainDefPtr def,
const char *type = NULL;
int n, allones = 1;
- virCheckFlags(VIR_DOMAIN_XML_SECURE |
- VIR_DOMAIN_XML_INACTIVE |
- VIR_DOMAIN_XML_UPDATE_CPU, NULL);
+ virCheckFlags(DUMPXML_FLAGS | VIR_DOMAIN_XML_INTERNAL_STATUS, NULL);
if (!(type = virDomainVirtTypeToString(def->virtType))) {
virDomainReportError(VIR_ERR_INTERNAL_ERROR,
@@ -10055,6 +10063,13 @@ char *virDomainDefFormat(virDomainDefPtr def,
return NULL;
}
+char *
+virDomainDefFormat(virDomainDefPtr def, unsigned int flags)
+{
+ virCheckFlags(DUMPXML_FLAGS, NULL);
+ return virDomainDefFormatInternal(def, flags);
+}
+
static char *virDomainObjFormat(virCapsPtr caps,
virDomainObjPtr obj,
@@ -10082,8 +10097,7 @@ static char *virDomainObjFormat(virCapsPtr caps,
((caps->privateDataXMLFormat)(&buf, obj->privateData)) < 0)
goto error;
- if (!(config_xml = virDomainDefFormat(obj->def,
- flags)))
+ if (!(config_xml = virDomainDefFormatInternal(obj->def, flags)))
goto error;
virBufferAdd(&buf, config_xml, strlen(config_xml));
--
1.7.4.4
13 years, 4 months
[libvirt] [PATCH 1/1] Repoint main page links to libvirt driver pages
by Dave Allan
From: David Allan <dallan(a)redhat.com>
The "libvirt supports:" section on the main page of libvirt.org
contains a list of hypervisors with links that point to the sites of
the underlying virt technologies. The entry for KVM points to
http://www.linux-kvm.org/, for example. People coming to libvirt.org
for the first time are likely to know about those sites, and they're
probably interested in how libvirt manages those technolgies. This
patch points those links to the libvirt driver pages instead. It also
consolidates KVM and QEMU as there is only one libvirt driver page for
them. Finally, it adds a line about networking support.
Dave
---
docs/index.html.in | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/docs/index.html.in b/docs/index.html.in
index 64eb84d..536e354 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -35,32 +35,32 @@
<ul>
<li>
- The <a href="http://www.cl.cam.ac.uk/Research/SRG/netos/xen/index.html">Xen</a> hypervisor
- on Linux and Solaris hosts.
+ The <a href="http://libvirt.org/drvqemu.html">KVM/QEMU</a> Linux hypervisor
</li>
<li>
- The <a href="http://wiki.qemu.org/Index.html">QEMU</a> emulator
+ The <a href="http://libvirt.org/drvxen.html">Xen</a> hypervisor
+ on Linux and Solaris hosts.
</li>
<li>
- The <a href="http://www.linux-kvm.org/">KVM</a> Linux hypervisor
+ The <a href="http://libvirt.org/drvlxc.html">LXC</a> Linux container system
</li>
<li>
- The <a href="http://lxc.sourceforge.net/">LXC</a> Linux container system
+ The <a href="http://libvirt.org/drvopenvz.html">OpenVZ</a> Linux container system
</li>
<li>
- The <a href="http://openvz.org/">OpenVZ</a> Linux container system
+ The <a href="http://libvirt.org/drvuml.html">User Mode Linux</a> paravirtualized kernel
</li>
<li>
- The <a href="http://user-mode-linux.sourceforge.net/">User Mode Linux</a> paravirtualized kernel
+ The <a href="http://libvirt.org/drvvbox.html">VirtualBox</a> hypervisor
</li>
<li>
- The <a href="http://www.virtualbox.org/">VirtualBox</a> hypervisor
+ The <a href="http://libvirt.org/drvesx.html">VMware ESX and GSX</a> hypervisors
</li>
<li>
- The <a href="http://www.vmware.com/">VMware ESX and GSX</a> hypervisors
+ The <a href="http://libvirt.org/drvvmware.html">VMware Workstation and Player</a> hypervisors
</li>
<li>
- The <a href="http://www.vmware.com/">VMware Workstation and Player</a> hypervisors
+ Virtual networks using bridging, NAT, VEPA and VN-LINK.
</li>
<li>
Storage on IDE/SCSI/USB disks, FibreChannel, LVM, iSCSI, NFS and filesystems
--
1.7.4.4
13 years, 4 months
[libvirt] [PATCH v3 1/2] vshCommandOptString returns -1 if option is empty and not VSH_OFLAG_EMPTY_OK
by Hu Tao
Pointed out by Eric. Thanks.
---
tools/virsh.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index b7cea58..b43af70 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -12375,8 +12375,13 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value)
vshError(NULL, _("Missing required option '%s'"), name);
ret = -1;
} else {
- /* Treat "--option ''" as if option had not been specified. */
- ret = 0;
+ /* --option '' */
+ if (arg->def->flag & VSH_OFLAG_EMPTY_OK) {
+ ret = 0;
+ } else {
+ vshError(NULL, _("option '%s' is empty"), name);
+ ret = -1;
+ }
}
}
--
1.7.5.1
13 years, 4 months
[libvirt] [PATCH 1/1] Fix error code for storage operations
by Dave Allan
Many volume operations will fail if the volume in question is being
allocated. These operations were returning VIR_ERR_INTERNAL_ERROR
when they should be returning VIR_ERR_OPERATION_INVALID.
---
src/storage/storage_driver.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 4f35be0..997b876 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1486,7 +1486,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
}
if (origvol->building) {
- virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+ virStorageReportError(VIR_ERR_OPERATION_INVALID,
_("volume '%s' is still being allocated."),
origvol->name);
goto cleanup;
@@ -1667,7 +1667,7 @@ storageVolumeUpload(virStorageVolPtr obj,
}
if (vol->building) {
- virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+ virStorageReportError(VIR_ERR_OPERATION_INVALID,
_("volume '%s' is still being allocated."),
vol->name);
goto out;
@@ -1876,7 +1876,7 @@ storageVolumeWipe(virStorageVolPtr obj,
}
if (vol->building) {
- virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+ virStorageReportError(VIR_ERR_OPERATION_INVALID,
_("volume '%s' is still being allocated."),
vol->name);
goto out;
@@ -1936,7 +1936,7 @@ storageVolumeDelete(virStorageVolPtr obj,
}
if (vol->building) {
- virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+ virStorageReportError(VIR_ERR_OPERATION_INVALID,
_("volume '%s' is still being allocated."),
vol->name);
goto cleanup;
--
1.7.4.4
13 years, 4 months
[libvirt] [PATCH 0/3] RFC: setvcpus: support --current option
by Taku Izumi
Hi all,
This patchset adds the --current option to "virsh setvcpus"
command. Currently "virsh setvcpus" command supports
"--live" and "--config" , but "--current" option.
>From view of consistency, it's reasonable to support
"--current" option too.
*[PATCH 1/3] setvcpus: extend virDomainSetVcpusFlags API to support current flag
*[PATCH 2/3] setvcpus: extend qemuDomainSetVcpusFlags() to support current flag
*[PATCH 3/3] setvcpus: add "--current" option to "virsh setvcpus"
Best regards,
Taku Izumi <izumi.taku(a)jp.fujitsu.com>
13 years, 4 months
