[libvirt] [PATCH 0/9] Add support for (qcow*) volume encryption
Hello, the following patches add full support for qcow/qcow2 volume encryption, assuming a client that supports it. New XML tags are defined to represent encryption parameters (currently format and passphrase, more can be added in the future), e.g. <encryption format='qcow'> <passphrase>c2lsbHk=</passphrase> </encryption> (passphrase content uses base64) The <encryption> tag can be added to a <volume> node passed to virStorageVolCreateXML() to create an encrypted volume, or to a <disk> node inside a <domain> to specify what encryption parameters to use for a domain. If the domain is persistent, the parameters (including the passphrase) will be saved unencrypted in /etc/libvirtd; the primary use case is to store the parameters outside of libvirtd, (perhaps by virt-manager in a GNOME keyring). Possible enhancements: - Documentation and test cases. I'll write both if the code is acceptable, I wanted to make the code available for review first. - Support for "dumb" clients that don't know anything about encryption formats and the required parameters: adding an encryption format to libvirt would automatically make it supported in all clients. Such a client would only request that a volume should be created when creating it, and libvirt would choose an appropriate format, parameters and passphrase/key and return it to the client, who could later pass it unmodified inside a <domain>. This requires public API additions to let libvirt return the encryption information as one of the results of a volume creation operation. - Support for storing the passphrases/keys used by persistent domains outside of the main XML files, e.g. in a separate passphrase-encrypted file that must be entered on libvirtd startup.
Define an <encryption> tag specifying volume encryption format and format-depenedent parameters (e.g. passphrase, cipher name, key length, key). In most cases, the "secrets" (passphrases/keys) should only be transferred from libvirt users to libvirt, not the other way around. (Volume creation, when libvirt generates secrets for the user, is the only planned exception). Permanent storage of the secrets should be implemented outside of libvirt, although virDomainDefineXML() will cause libvirtd to store the secret locally with a domain. Only the qcow/qcow2 encryption format is currently supported, with the key/passphrase represented using base64. This patch does not add any users; the <encryption> tag is added in the following patches to both volumes (to support encrypted volume creation) and domains. --- bootstrap | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 5 + src/storage_encryption.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++ src/storage_encryption.h | 62 +++++++++++ 6 files changed, 322 insertions(+), 0 deletions(-) create mode 100644 src/storage_encryption.c create mode 100644 src/storage_encryption.h diff --git a/bootstrap b/bootstrap index 8b81e0e..885b299 100755 --- a/bootstrap +++ b/bootstrap @@ -65,6 +65,7 @@ gnulib_tool=$GNULIB_SRCDIR/gnulib-tool <$gnulib_tool || exit modules=' +base64 c-ctype close connect diff --git a/po/POTFILES.in b/po/POTFILES.in index 0ea21fd..cc99b48 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -37,6 +37,7 @@ src/storage_backend_logical.c src/storage_backend_scsi.c src/storage_conf.c src/storage_driver.c +src/storage_encryption.c src/test.c src/uml_conf.c src/uml_driver.c diff --git a/src/Makefile.am b/src/Makefile.am index 9b662ae..6c628bd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -51,6 +51,7 @@ UTIL_SOURCES = \ memory.c memory.h \ pci.c pci.h \ qparams.c qparams.h \ + storage_encryption.h storage_encryption.c \ threads.c threads.h \ threads-pthread.h \ threads-win32.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 59c78d5..9850daa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -324,6 +324,11 @@ virStoragePartedFsTypeTypeToString; virStoragePoolObjLock; virStoragePoolObjUnlock; +virStorageEncryptionFree; +virStorageEncryptionDropSecrets; +virStorageEncryptionParseNode; +virStorageEncryptionFormat; + # threads.h virMutexInit; diff --git a/src/storage_encryption.c b/src/storage_encryption.c new file mode 100644 index 0000000..c3e3219 --- /dev/null +++ b/src/storage_encryption.c @@ -0,0 +1,252 @@ +/* + * storage_encryption.h: volume encryption information + * + * Copyright (C) 2009 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Red Hat Author: Miloslav Trmač <mitr@redhat.com> + */ + +#include <config.h> + +#include "internal.h" + +#include "base64.h" +#include "buf.h" +#include "memory.h" +#include "storage_conf.h" +#include "storage_encryption.h" +#include "util.h" +#include "xml.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +VIR_ENUM_IMPL(virStorageEncryptionFormat, + VIR_STORAGE_ENCRYPTION_FORMAT_LAST, "unencrypted", "qcow") + +void +virStorageEncryptionFree(virStorageEncryptionPtr enc) +{ + if (!enc) + return; + + switch (enc->format) { + case VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED: + break; + + case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: + if (enc->v.qcow.passphrase != NULL) { + memset(enc->v.qcow.passphrase, 0, + strlen(enc->v.qcow.passphrase)); + VIR_FREE(enc->v.qcow.passphrase); + } + break; + + default: + virStorageReportError(NULL, VIR_ERR_INTERNAL_ERROR, + _("unhandled volume encryption format %d"), + enc->format); + break; + } + VIR_FREE(enc); +} + +void +virStorageEncryptionDropSecrets(virStorageEncryptionPtr enc) +{ + if (!enc) + return; + + switch (enc->format) { + case VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED: + break; + + case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: + if (enc->v.qcow.passphrase != NULL) { + memset(enc->v.qcow.passphrase, 0, + strlen(enc->v.qcow.passphrase)); + VIR_FREE(enc->v.qcow.passphrase); + } + break; + + default: + virStorageReportError(NULL, VIR_ERR_INTERNAL_ERROR, + _("unhandled volume encryption format %d"), + enc->format); + break; + } +} + +static virStorageEncryptionPtr +virStorageEncryptionParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt) +{ + virStorageEncryptionPtr ret; + char *format_str; + int format; + + if (VIR_ALLOC(ret) < 0) { + virReportOOMError(conn); + return NULL; + } + + format_str = virXPathString(conn, "string(./@format)", ctxt); + if (format_str == NULL) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + _("unknown volume encryption format")); + goto cleanup; + } + format = virStorageEncryptionFormatTypeFromString(format_str); + if (format < 0) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + _("unknown volume encryption format type %s"), + format_str); + VIR_FREE(format_str); + goto cleanup; + } + VIR_FREE(format_str); + ret->format = format; + + switch (ret->format) { + case VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED: + break; + + case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW:{ + char *base64; + + base64 = virXPathString(conn, "string(./passphrase)", ctxt); + if (base64 != NULL) { + size_t base64_len, raw_len; + char *raw; + bool base64_ok; + + base64_len = strlen(base64); + base64_ok = base64_decode_alloc(base64, base64_len, &raw, + &raw_len); + memset(base64, 0, base64_len); + VIR_FREE(base64); + + if (!base64_ok) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + _("invalid base64 in passphrase")); + goto cleanup; + } + if (raw == NULL) { + virReportOOMError(conn); + goto cleanup; + } + if (VIR_ALLOC_N(ret->v.qcow.passphrase, raw_len + 1) < 0) { + virReportOOMError(conn); + memset(raw, 0, raw_len); + free(raw); + goto cleanup; + } + memcpy(ret->v.qcow.passphrase, raw, raw_len); + memset(raw, 0, raw_len); + free(raw); + } + break; + } + + default: + virStorageReportError(NULL, VIR_ERR_INTERNAL_ERROR, + _("unhandled volume encryption format %d"), + ret->format); + break; + } + + return ret; + + cleanup: + virStorageEncryptionFree(ret); + return NULL; +} + +virStorageEncryptionPtr +virStorageEncryptionParseNode(virConnectPtr conn, + xmlDocPtr xml, xmlNodePtr root) +{ + xmlXPathContextPtr ctxt = NULL; + virStorageEncryptionPtr enc = NULL; + + if (STRNEQ((const char *) root->name, "encryption")) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("unknown root element for volume " + "encryption information")); + goto cleanup; + } + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(conn); + goto cleanup; + } + + ctxt->node = root; + enc = virStorageEncryptionParseXML(conn, ctxt); + + cleanup: + xmlXPathFreeContext(ctxt); + return enc; +} + +int +virStorageEncryptionFormat(virConnectPtr conn, + virBufferPtr buf, + virStorageEncryptionPtr enc, bool with_secrets) +{ + const char *format; + + format = virStorageEncryptionFormatTypeToString(enc->format); + if (!format) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + "%s", _("unexpected encryption format")); + return -1; + } + virBufferVSprintf(buf, " <encryption format='%s'>\n", format); + + switch (enc->format) { + case VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED: + break; + + case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW: + if (with_secrets && enc->v.qcow.passphrase != NULL) { + char *base64; + + base64_encode_alloc(enc->v.qcow.passphrase, + strlen(enc->v.qcow.passphrase), + &base64); + if (base64 == NULL) { + virReportOOMError(conn); + return -1; + } + virBufferVSprintf(buf, " <passphrase>%s</passphrase>\n", + base64); + free(base64); + } + break; + + default: + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unhandled volume encryption format %d"), + enc->format); + return -1; + } + + virBufferAddLit(buf, " </encryption>\n"); + + return 0; +} diff --git a/src/storage_encryption.h b/src/storage_encryption.h new file mode 100644 index 0000000..bd0b417 --- /dev/null +++ b/src/storage_encryption.h @@ -0,0 +1,62 @@ +/* + * storage_encryption.h: volume encryption information + * + * Copyright (C) 2009 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Red Hat Author: Miloslav Trmač <mitr@redhat.com> + */ + +#ifndef __VIR_STORAGE_ENCRYPTION_H__ +#define __VIR_STORAGE_ENCRYPTION_H__ + +#include "internal.h" +#include "buf.h" +#include "util.h" + +#include <stdbool.h> +#include <libxml/tree.h> + +enum virStorageEncryptionFormat { + VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED = 0, + VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, /* Both qcow and qcow2 */ + + VIR_STORAGE_ENCRYPTION_FORMAT_LAST, +}; +VIR_ENUM_DECL(virStorageEncryptionFormat) + +typedef struct _virStorageEncryption virStorageEncryption; +typedef virStorageEncryption *virStorageEncryptionPtr; +struct _virStorageEncryption { + int format; /* enum virStorageEncryptionFormat */ + + union { /* Format-specific data */ + struct { + char *passphrase; + } qcow; + } v; +}; + +void virStorageEncryptionFree(virStorageEncryptionPtr enc); +void virStorageEncryptionDropSecrets(virStorageEncryptionPtr enc); +virStorageEncryptionPtr virStorageEncryptionParseNode(virConnectPtr conn, + xmlDocPtr xml, + xmlNodePtr root); +int virStorageEncryptionFormat(virConnectPtr conn, virBufferPtr buf, + virStorageEncryptionPtr enc, + bool with_secrets); + +#endif /* __VIR_STORAGE_ENCRYPTION_H__ */ -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:11:57PM +0200, Miloslav Trma?? wrote:
Define an <encryption> tag specifying volume encryption format and format-depenedent parameters (e.g. passphrase, cipher name, key length, key).
In most cases, the "secrets" (passphrases/keys) should only be transferred from libvirt users to libvirt, not the other way around. (Volume creation, when libvirt generates secrets for the user, is the only planned exception).
Permanent storage of the secrets should be implemented outside of libvirt, although virDomainDefineXML() will cause libvirtd to store the secret locally with a domain.
Only the qcow/qcow2 encryption format is currently supported, with the key/passphrase represented using base64.
This patch does not add any users; the <encryption> tag is added in the following patches to both volumes (to support encrypted volume creation) and domains.
[snip]
+#include <stdbool.h> +#include <libxml/tree.h> + +enum virStorageEncryptionFormat { + VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED = 0, + VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, /* Both qcow and qcow2 */ + + VIR_STORAGE_ENCRYPTION_FORMAT_LAST, +}; +VIR_ENUM_DECL(virStorageEncryptionFormat) + +typedef struct _virStorageEncryption virStorageEncryption; +typedef virStorageEncryption *virStorageEncryptionPtr; +struct _virStorageEncryption { + int format; /* enum virStorageEncryptionFormat */ + + union { /* Format-specific data */ + struct { + char *passphrase; + } qcow; + } v; +};
As with the XML format, I'd like to avoid encoding qcow as a structural element here. Instead go for a generic storage of secrets. enum virStorageEncryptionSecret { VIR_STORAGE_ENCRYPTION_SECRET_PASSPHRASE, }; struct virStorageSecret{ int type; /* enum virStorageSecret */ union { char *passphrase; } data; }; struct _virStorageEncryption { unsigned encrypted : 1; int nsecrets; virStorageSecret *secrets; } This allows for > 1 secret should we need that (eg, for LUKS/cryptsetup volume) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
----- "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jul 21, 2009 at 01:11:57PM +0200, Miloslav Trma?? wrote:
+#include <stdbool.h> +#include <libxml/tree.h> + +enum virStorageEncryptionFormat { + VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED = 0, + VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, /* Both qcow and qcow2 */ + + VIR_STORAGE_ENCRYPTION_FORMAT_LAST, +}; +VIR_ENUM_DECL(virStorageEncryptionFormat) + +typedef struct _virStorageEncryption virStorageEncryption; +typedef virStorageEncryption *virStorageEncryptionPtr; +struct _virStorageEncryption { + int format; /* enum virStorageEncryptionFormat */ + + union { /* Format-specific data */ + struct { + char *passphrase; + } qcow; + } v; +};
As with the XML format, I'd like to avoid encoding qcow as a structural element here. Instead go for a generic storage of secrets.
enum virStorageEncryptionSecret { VIR_STORAGE_ENCRYPTION_SECRET_PASSPHRASE, };
struct virStorageSecret{ int type; /* enum virStorageSecret */
union { char *passphrase; } data; };
struct _virStorageEncryption { unsigned encrypted : 1;
int nsecrets; virStorageSecret *secrets; }
This allows for > 1 secret should we need that (eg, for LUKS/cryptsetup volume) As argued in the 0/9 e-mail, the "encryption format" is an independent piece of information that needs to be stored, and the set of other information that needs to be stored can differ among formats. Should we ever need support for more than one secret in LUKS, that a separate struct { ... } luks should be added to the union: it seems to me that support for storing more than one passphrase is not necessary for libvirt's use of LUKS (although LUKS supports it) - on the other hand, if such support would be necessary, it would most likely be necessary to store the slot used by each passphrase as well.
We know that the type and amount of information that will need to be stored will vary depending on the "encryption format"; on the other hand, expecting that the information consists of independent "secrets", each with a simple and format-independent definition, is probably too optimistic. (As mentioned above, we might need a key slot ID associated with a LUKS passphrase; we might also need a password hash algorithm associated with a dm-crypt passphrase. The encryption formats used in practice are often complex enough that a "simple passphrase" can not be used.) Once we have to assume that each secret can have an "encryption format"-dependent format, I think it is much clearer to use something like enum { FORMAT_1, FORMAT_2, FORMAT_3 } format; union { struct { char *secret_1; } format_1; struct { struct { char *secret, id; } secrets[N]; } format_2; struct { int param_1, param_2, param_3; char *secret_1, *secret_2 }; format_2; } which explicitly defines what information is requested for each format, and how it is related, (and which allows showing the data related to a single format in a debugger by simply choosing a single member of the union) than something like struct { enum { SECRET_FMT_1, SECRET_FMT_2, PARAM_FMT_1, PARAM_FMT_2 } type; union { char *secret_fmt_1; struct { char *secret, id} secret_fmt_2; int param_fmt_1; char *param_fmt_2; } } [...] with implicit assumptions about the secret and parameter formats used by various "encryption formats". Even if the XML uses one or more generic-looking <secret> elements, the information will have to be categorized depending on its meaning and relations before being used; it seems to me cleaner to do it once, when parsing the XML, than to parse the XML without assigning much semantic value to it, and then re-parse the internal representation to categorize the information. Categorizing the information already when parsing the XML will also make it easy to detect invalid or missing data already when parsing the XML, before any "real" operations that might be costly or difficult to undo are started, or before the information is used or stored by other components of libvirt. Mirek
On Fri, Jul 24, 2009 at 12:08:32AM -0400, Miloslav Trmac wrote:
----- "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jul 21, 2009 at 01:11:57PM +0200, Miloslav Trma?? wrote:
+#include <stdbool.h> +#include <libxml/tree.h> + +enum virStorageEncryptionFormat { + VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED = 0, + VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, /* Both qcow and qcow2 */ + + VIR_STORAGE_ENCRYPTION_FORMAT_LAST, +}; +VIR_ENUM_DECL(virStorageEncryptionFormat) + +typedef struct _virStorageEncryption virStorageEncryption; +typedef virStorageEncryption *virStorageEncryptionPtr; +struct _virStorageEncryption { + int format; /* enum virStorageEncryptionFormat */ + + union { /* Format-specific data */ + struct { + char *passphrase; + } qcow; + } v; +};
As with the XML format, I'd like to avoid encoding qcow as a structural element here. Instead go for a generic storage of secrets.
enum virStorageEncryptionSecret { VIR_STORAGE_ENCRYPTION_SECRET_PASSPHRASE, };
struct virStorageSecret{ int type; /* enum virStorageSecret */
union { char *passphrase; } data; };
struct _virStorageEncryption { unsigned encrypted : 1;
int nsecrets; virStorageSecret *secrets; }
This allows for > 1 secret should we need that (eg, for LUKS/cryptsetup volume)
As argued in the 0/9 e-mail, the "encryption format" is an independent piece of information that needs to be stored, and the set of other information that needs to be stored can differ among formats. Should we ever need support for more than one secret in LUKS, that a separate struct { ... } luks should be added to the union: it seems to me that support for storing more than one passphrase is not necessary for libvirt's use of LUKS (although LUKS supports it) - on the other hand, if such support would be necessary, it would most likely be necessary to store the slot used by each passphrase as well.
For the domain XML I agree that libirt would not need to worry about multiple LUKS keys, but for the storage volume XML we would need to expose the fact that there are multiple keys,or allow creation of volumes with multiple keys.
We know that the type and amount of information that will need to be stored will vary depending on the "encryption format"; on the other hand, expecting that the information consists of independent "secrets", each with a simple and format-independent definition, is probably too optimistic. (As mentioned above, we might need a key slot ID associated with a LUKS passphrase; we might also need a password hash algorithm associated with a dm-crypt passphrase. The encryption formats used in practice are often complex enough that a "simple passphrase" can not be used.) Once we have to assume that each secret can have an "encryption format"-dependent format, I think it is much clearer to use something like
The idea of a 'key slot' and 'password hash algorithm' are not neccessarily unique to LUKS though. If we get 2 different encryption formats both requiring the concept of a 'key slot' we need to represent them in the same way in the XML, not hve a different XML for each format.
enum { FORMAT_1, FORMAT_2, FORMAT_3 } format; union { struct { char *secret_1; } format_1; struct { struct { char *secret, id; } secrets[N]; } format_2; struct { int param_1, param_2, param_3; char *secret_1, *secret_2 }; format_2; }
which explicitly defines what information is requested for each format, and how it is related, (and which allows showing the data related to a single format in a debugger by simply choosing a single member of the union) than something like
struct { enum { SECRET_FMT_1, SECRET_FMT_2, PARAM_FMT_1, PARAM_FMT_2 } type; union { char *secret_fmt_1; struct { char *secret, id} secret_fmt_2; int param_fmt_1; char *param_fmt_2; } } [...] with implicit assumptions about the secret and parameter formats used by various "encryption formats".
This makes life very hard for libvirt clients, because it implies that every single new disk encryption format we add, results in new XML elements being added, which pushes a significant burden onto clients which want to use encryption, because they now have to be aware of multiple different ways of representing the same fundamental concepts.
Even if the XML uses one or more generic-looking <secret> elements, the information will have to be categorized depending on its meaning and relations before being used; it seems to me cleaner to do it once, when parsing the XML, than to parse the XML without assigning much semantic value to it, and then re-parse the internal representation to categorize the information.
Categorizing the information already when parsing the XML will also make it easy to detect invalid or missing data already when parsing the XML, before any "real" operations that might be costly or difficult to undo are started, or before the information is used or stored by other components of libvirt.
The libvirt approach to XML representation is to try and define XML format that are indenpedant of specific implementations. This does imply that the XML parser cannot really do anything beyond basic syntactic validation, near zero semantic validation. The task of semantic validation of the parsed data, is thus passed off to the internal drivers which provide the specific implementations. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
The XML allows <encryption format='unencrypted'/>, this implementation canonicalizes the internal representation so that "vol->encryption" is non-NULL iff the volume is encrypted. Note that partial encryption information (e.g. specifying an encryption format, but not the key/passphrase) is valid: * virStorageVolGetXMLDesc() will never reveal the key/passphrase, even if known by libvirt. * Future mechanisms could be set up to allow a libvirt user to specify during volume creation that a volume should be encrypted, leaving libvirt to choose suitable parameters and key and return them: this would allow the libvirt user to automatically support any encryption parameters (and perhaps encryption formats) supported in libvirt, as long as the user can send the same information back when using the volume in the future. --- src/storage_conf.c | 19 +++++++++++++++++++ src/storage_conf.h | 2 ++ 2 files changed, 21 insertions(+), 0 deletions(-) diff --git a/src/storage_conf.c b/src/storage_conf.c index 075279c..4a77e87 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -267,6 +267,7 @@ virStorageVolDefFree(virStorageVolDefPtr def) { VIR_FREE(def->target.perms.label); VIR_FREE(def->backingStore.path); VIR_FREE(def->backingStore.perms.label); + virStorageEncryptionFree(def->encryption); VIR_FREE(def); } @@ -960,6 +961,7 @@ virStorageVolDefParseXML(virConnectPtr conn, char *allocation = NULL; char *capacity = NULL; char *unit = NULL; + xmlNodePtr node; options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) @@ -1047,6 +1049,19 @@ virStorageVolDefParseXML(virConnectPtr conn, "./backingStore/permissions", 0600) < 0) goto cleanup; + node = virXPathNode(conn, "./encryption", ctxt); + if (node != NULL) { + virStorageEncryptionPtr enc; + + enc = virStorageEncryptionParseNode(conn, ctxt->doc, node); + if (enc == NULL) + goto cleanup; + if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED) + ret->encryption = enc; + else + virStorageEncryptionFree(enc); + } + return ret; cleanup: @@ -1254,6 +1269,10 @@ virStorageVolDefFormat(virConnectPtr conn, &def->backingStore, "backingStore") < 0) goto cleanup; + if (def->encryption != NULL && + virStorageEncryptionFormat(conn, &buf, def->encryption, false) < 0) + goto cleanup; + virBufferAddLit(&buf,"</volume>\n"); if (virBufferError(&buf)) diff --git a/src/storage_conf.h b/src/storage_conf.h index a6c3650..cd6944f 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -26,6 +26,7 @@ #include "internal.h" #include "util.h" +#include "storage_encryption.h" #include "threads.h" #include <libxml/tree.h> @@ -95,6 +96,7 @@ struct _virStorageVolDef { virStorageVolSource source; virStorageVolTarget target; virStorageVolTarget backingStore; + virStorageEncryptionPtr encryption; /* only used if not "unencrypted" */ }; typedef struct _virStorageVolDefList virStorageVolDefList; -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:11:58PM +0200, Miloslav Trma?? wrote:
The XML allows <encryption format='unencrypted'/>, this implementation canonicalizes the internal representation so that "vol->encryption" is non-NULL iff the volume is encrypted.
Note that partial encryption information (e.g. specifying an encryption format, but not the key/passphrase) is valid: * virStorageVolGetXMLDesc() will never reveal the key/passphrase, even if known by libvirt.
I don't think that restriction really adds anything in the scenario that we're using domain XML files for persistent storage of keys. eg, if domain XML lets you see passphrases, then vol XML should too (given a suitable VIR_STORAGE_VOL_SECURE flag). if we use a keystore, then forbid display of passphrases for domain XML and volume XML, and reference everything via UUID.
* Future mechanisms could be set up to allow a libvirt user to specify during volume creation that a volume should be encrypted, leaving libvirt to choose suitable parameters and key and return them: this would allow the libvirt user to automatically support any encryption parameters (and perhaps encryption formats) supported in libvirt, as long as the user can send the same information back when using the volume in the future.
We could allow this now without extra APIs, if we let virStorageVolGetXML show the ke/passphrase given a new VIR_STORAGE_VOL_SECURE flag.
@@ -960,6 +961,7 @@ virStorageVolDefParseXML(virConnectPtr conn, char *allocation = NULL; char *capacity = NULL; char *unit = NULL; + xmlNodePtr node;
options = virStorageVolOptionsForPoolType(pool->type); if (options == NULL) @@ -1047,6 +1049,19 @@ virStorageVolDefParseXML(virConnectPtr conn, "./backingStore/permissions", 0600) < 0) goto cleanup;
+ node = virXPathNode(conn, "./encryption", ctxt); + if (node != NULL) { + virStorageEncryptionPtr enc; + + enc = virStorageEncryptionParseNode(conn, ctxt->doc, node); + if (enc == NULL) + goto cleanup; + if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED) + ret->encryption = enc; + else + virStorageEncryptionFree(enc); + }
The <encryption> element should probably be inside the <target> element, since we might need to have separate <encryption> element for the <backingstore> too in the future. eg have it alongside the <permissions> element
diff --git a/src/storage_conf.h b/src/storage_conf.h index a6c3650..cd6944f 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -26,6 +26,7 @@
#include "internal.h" #include "util.h" +#include "storage_encryption.h" #include "threads.h"
#include <libxml/tree.h> @@ -95,6 +96,7 @@ struct _virStorageVolDef { virStorageVolSource source; virStorageVolTarget target; virStorageVolTarget backingStore; + virStorageEncryptionPtr encryption; /* only used if not "unencrypted" */ };
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
(The implementation is not very generic, but that can be very easily rectified if/when new encryption formats appear.) --- src/storage_backend_fs.c | 61 +++++++++++++++++++++++++++++++++++---------- 1 files changed, 47 insertions(+), 14 deletions(-) diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 8cfc462..0219eb6 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -26,6 +26,7 @@ #include <sys/statvfs.h> #include <sys/types.h> #include <sys/stat.h> +#include <stdbool.h> #include <stdio.h> #include <dirent.h> #include <errno.h> @@ -81,6 +82,9 @@ struct FileTypeInfo { /* Store a COW base image path (possibly relative), * or NULL if there is no COW base image, to RES; * return BACKING_STORE_* */ + int qcowCryptOffset; /* Byte offset from start of file + * where to find encryption mode, + * -1 if encryption is not used */ int (*getBackingStore)(virConnectPtr conn, char **res, const unsigned char *buf, size_t buf_size); }; @@ -89,54 +93,54 @@ struct FileTypeInfo const fileTypeInfo[] = { /* XXX Untested { VIR_STORAGE_VOL_FILE_BOCHS, "Bochs Virtual HD Image", NULL, LV_LITTLE_ENDIAN, 64, 0x20000, - 32+16+16+4+4+4+4+4, 8, 1, NULL },*/ + 32+16+16+4+4+4+4+4, 8, 1, -1, NULL },*/ /* CLoop */ /* XXX Untested { VIR_STORAGE_VOL_CLOOP, "#!/bin/sh\n#V2.0 Format\nmodprobe cloop file=$0 && mount -r -t iso9660 /dev/cloop $1\n", NULL, LV_LITTLE_ENDIAN, -1, 0, - -1, 0, 0, NULL }, */ + -1, 0, 0, -1, NULL }, */ /* Cow */ { VIR_STORAGE_VOL_FILE_COW, "OOOM", NULL, LV_BIG_ENDIAN, 4, 2, - 4+4+1024+4, 8, 1, cowGetBackingStore }, + 4+4+1024+4, 8, 1, -1, cowGetBackingStore }, /* DMG */ /* XXX QEMU says there's no magic for dmg, but we should check... */ { VIR_STORAGE_VOL_FILE_DMG, NULL, ".dmg", 0, -1, 0, - -1, 0, 0, NULL }, + -1, 0, 0, -1, NULL }, /* XXX there's probably some magic for iso we can validate too... */ { VIR_STORAGE_VOL_FILE_ISO, NULL, ".iso", 0, -1, 0, - -1, 0, 0, NULL }, + -1, 0, 0, -1, NULL }, /* Parallels */ /* XXX Untested { VIR_STORAGE_VOL_FILE_PARALLELS, "WithoutFreeSpace", NULL, LV_LITTLE_ENDIAN, 16, 2, - 16+4+4+4+4, 4, 512, NULL }, + 16+4+4+4+4, 4, 512, -1, NULL }, */ /* QCow */ { VIR_STORAGE_VOL_FILE_QCOW, "QFI", NULL, LV_BIG_ENDIAN, 4, 1, - 4+4+8+4+4, 8, 1, qcowXGetBackingStore }, + 4+4+8+4+4, 8, 1, 4+4+8+4+4+8+1+1+2, qcowXGetBackingStore }, /* QCow 2 */ { VIR_STORAGE_VOL_FILE_QCOW2, "QFI", NULL, LV_BIG_ENDIAN, 4, 2, - 4+4+8+4+4, 8, 1, qcowXGetBackingStore }, + 4+4+8+4+4, 8, 1, 4+4+8+4+4+8, qcowXGetBackingStore }, /* VMDK 3 */ /* XXX Untested { VIR_STORAGE_VOL_FILE_VMDK, "COWD", NULL, LV_LITTLE_ENDIAN, 4, 1, - 4+4+4, 4, 512, NULL }, + 4+4+4, 4, 512, -1, NULL }, */ /* VMDK 4 */ { VIR_STORAGE_VOL_FILE_VMDK, "KDMV", NULL, LV_LITTLE_ENDIAN, 4, 1, - 4+4+4, 8, 512, vmdk4GetBackingStore }, + 4+4+4, 8, 512, -1, vmdk4GetBackingStore }, /* Connectix / VirtualPC */ /* XXX Untested { VIR_STORAGE_VOL_FILE_VPC, "conectix", NULL, LV_BIG_ENDIAN, -1, 0, - -1, 0, 0, NULL}, + -1, 0, 0, -1, NULL}, */ }; @@ -282,13 +286,16 @@ static int virStorageBackendProbeTarget(virConnectPtr conn, virStorageVolTargetPtr target, char **backingStore, unsigned long long *allocation, - unsigned long long *capacity) { + unsigned long long *capacity, + virStorageEncryptionPtr *encryption) { int fd; unsigned char head[20*512]; /* vmdk4GetBackingStore needs this much. */ int len, i, ret; if (backingStore) *backingStore = NULL; + if (encryption) + *encryption = NULL; if ((fd = open(target->path, O_RDONLY)) < 0) { virReportSystemError(conn, errno, @@ -317,6 +324,8 @@ static int virStorageBackendProbeTarget(virConnectPtr conn, /* First check file magic */ for (i = 0 ; i < ARRAY_CARDINALITY(fileTypeInfo) ; i++) { int mlen; + bool encrypted_qcow = false; + if (fileTypeInfo[i].magic == NULL) continue; @@ -375,6 +384,16 @@ static int virStorageBackendProbeTarget(virConnectPtr conn, *capacity *= fileTypeInfo[i].sizeMultiplier; } + if (fileTypeInfo[i].qcowCryptOffset != -1) { + int crypt_format; + + crypt_format = (head[fileTypeInfo[i].qcowCryptOffset] << 24) | + (head[fileTypeInfo[i].qcowCryptOffset+1] << 16) | + (head[fileTypeInfo[i].qcowCryptOffset+2] << 8) | + head[fileTypeInfo[i].qcowCryptOffset+3]; + encrypted_qcow = crypt_format != 0; + } + /* Validation passed, we know the file format now */ target->format = fileTypeInfo[i].type; if (fileTypeInfo[i].getBackingStore != NULL && backingStore) { @@ -400,6 +419,18 @@ static int virStorageBackendProbeTarget(virConnectPtr conn, } } } + if (encryption != NULL && encrypted_qcow) { + virStorageEncryptionPtr enc; + + if (VIR_ALLOC(enc) < 0) { + virReportOOMError(conn); + if (backingStore) + VIR_FREE(*backingStore); + return -1; + } + enc->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW; + *encryption = enc; + } return 0; } @@ -836,7 +867,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, &vol->target, &backingStore, &vol->allocation, - &vol->capacity) < 0)) { + &vol->capacity, + &vol->encryption) < 0)) { if (ret == -1) goto cleanup; else { @@ -876,7 +908,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, if ((ret = virStorageBackendProbeTarget(conn, &vol->backingStore, - NULL, NULL, NULL)) < 0) { + NULL, NULL, NULL, + NULL)) < 0) { if (ret == -1) goto cleanup; else { -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:11:59PM +0200, Miloslav Trma?? wrote:
(The implementation is not very generic, but that can be very easily rectified if/when new encryption formats appear.) --- src/storage_backend_fs.c | 61 +++++++++++++++++++++++++++++++++++---------- 1 files changed, 47 insertions(+), 14 deletions(-)
Yep, this is fine as an initial approach until we need to deal with other formats. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Supports only virStorageVolCreateXML, not virStorageVolCreateXMLFrom. Curiously, qemu-img does not need the passphrase for anything to create an encrypted volume. This implementation is sufficient for the qcow2 format, and for other formats when all encryption parameters are pre-specified. The qcow2 passphrase is stored in libvirt only during the volume creation operation, and automatically deleted afterwards. Other users might be able to query the volume properties while the volume is being created, but virStorageVolGetXMLDesc() won't reveal the passphrase to them. An earlier patch description has mentioned the possibility of a libvirt user specifying only the encryption format (or not even the format), letting libvirt to choose the parameters and return them. This would require libvirt interface extensions, and it is not supported by this patch. --- src/storage_backend.c | 41 +++++++++++++++++++++++++++++++++++++++-- src/storage_backend_disk.c | 7 +++++++ src/storage_backend_fs.c | 7 +++++++ src/storage_backend_logical.c | 7 +++++++ src/storage_driver.c | 4 ++++ 5 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/storage_backend.c b/src/storage_backend.c index 1664804..42c59ad 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -246,6 +246,13 @@ virStorageBackendCreateRaw(virConnectPtr conn, unsigned long long remain; char *buf = NULL; + if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support encrypted " + "volumes")); + return -1; + } + if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, vol->target.perms.mode)) < 0) { virReportSystemError(conn, errno, @@ -346,15 +353,17 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, NULL; const char **imgargv; + /* The extra NULL field is for indicating encryption (-e). */ const char *imgargvnormal[] = { NULL, "create", "-f", type, vol->target.path, size, NULL, + NULL }; /* Extra NULL fields are for including "backingType" when using - * kvm-img. It's -F backingType + * kvm-img (-F backingType), and for indicating encryption (-e). */ const char *imgargvbacking[] = { NULL, "create", @@ -364,6 +373,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, size, NULL, NULL, + NULL, NULL }; const char *convargv[] = { @@ -417,6 +427,22 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, } } + if (vol->encryption != NULL) { + if (vol->encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + _("unsupported volume encryption format %d"), + vol->encryption->format); + return -1; + } + if (vol->target.format != VIR_STORAGE_VOL_FILE_QCOW && + vol->target.format != VIR_STORAGE_VOL_FILE_QCOW2) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + _("qcow volume encryption unsupported with " + "volume format %s"), type); + return -1; + } + } + if ((create_tool = virFindFileInPath("kvm-img")) != NULL) use_kvmimg = 1; else if ((create_tool = virFindFileInPath("qemu-img")) != NULL) @@ -437,11 +463,16 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, imgargvbacking[7] = backingType; imgargvbacking[8] = vol->target.path; imgargvbacking[9] = size; - } + if (vol->encryption != NULL) + imgargvbacking[10] = "-e"; + } else if (vol->encryption != NULL) + imgargvbacking[8] = "-e"; imgargv = imgargvbacking; } else { imgargvnormal[0] = create_tool; imgargv = imgargvnormal; + if (vol->encryption != NULL) + imgargv[6] = "-e"; } @@ -490,6 +521,12 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn, "qcow-create")); return -1; } + if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("encrypted volumes not supported with " + "qcow-create")); + return -1; + } /* Size in MB - yes different units to qemu-img :-( */ snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024); diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index ae2acae..8f4f0ed 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -557,6 +557,13 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, NULL }; + if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support encrypted " + "volumes")); + return -1; + } + if (virStorageBackendDiskPartFormat(conn, pool, vol, partFormat) != 0) { return -1; } diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 0219eb6..b43daef 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -1070,6 +1070,13 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, int tool_type; if (inputvol) { + if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support " + "building encrypted volumes from " + "other volumes")); + return -1; + } create_func = virStorageBackendGetBuildVolFromFunction(conn, vol, inputvol); if (!create_func) diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c index 6c123ae..822d321 100644 --- a/src/storage_backend_logical.c +++ b/src/storage_backend_logical.c @@ -581,6 +581,13 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, }; const char **cmdargv = cmdargvnew; + if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support encrypted " + "volumes")); + return -1; + } + if (vol->backingStore.path) { cmdargv = cmdargvsnap; } diff --git a/src/storage_driver.c b/src/storage_driver.c index e9ecb20..98db9a0 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -1305,6 +1305,8 @@ storageVolumeCreateXML(virStoragePoolPtr obj, cleanup: if (volobj) virUnrefStorageVol(volobj); + if (voldef) + virStorageEncryptionDropSecrets(voldef->encryption); virStorageVolDefFree(voldef); if (pool) virStoragePoolObjUnlock(pool); @@ -1466,6 +1468,8 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, cleanup: if (volobj) virUnrefStorageVol(volobj); + if (newvol) + virStorageEncryptionDropSecrets(newvol->encryption); virStorageVolDefFree(newvol); if (pool) virStoragePoolObjUnlock(pool); -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:12:00PM +0200, Miloslav Trma?? wrote:
Supports only virStorageVolCreateXML, not virStorageVolCreateXMLFrom.
Curiously, qemu-img does not need the passphrase for anything to create an encrypted volume. This implementation is sufficient for the qcow2 format, and for other formats when all encryption parameters are pre-specified.
I believe that qemu-img only requires the passphrase if it is going to actually write data blocks to the file. Thus creating an empty file doesn't need it, but in the code path which clones an existing file it would need it. eg, virStorageVolCreateXMLFrom() would need it if you had implemented that - indeed that would potentially need both the source and destination keys. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
----- "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jul 21, 2009 at 01:12:00PM +0200, Miloslav Trma?? wrote:
Supports only virStorageVolCreateXML, not virStorageVolCreateXMLFrom.
Curiously, qemu-img does not need the passphrase for anything to create an encrypted volume. This implementation is sufficient for the qcow2 format, and for other formats when all encryption parameters are pre-specified.
I believe that qemu-img only requires the passphrase if it is going to actually write data blocks to the file. Thus creating an empty file doesn't need it, but in the code path which clones an existing file it would need it. eg, virStorageVolCreateXMLFrom() would need it if you had implemented that - indeed that would potentially need both the source and destination keys. Yes, that's correct. Mirek
On Tue, Jul 21, 2009 at 01:12:00PM +0200, Miloslav Trma?? wrote:
Supports only virStorageVolCreateXML, not virStorageVolCreateXMLFrom.
Curiously, qemu-img does not need the passphrase for anything to create an encrypted volume. This implementation is sufficient for the qcow2 format, and for other formats when all encryption parameters are pre-specified.
The qcow2 passphrase is stored in libvirt only during the volume creation operation, and automatically deleted afterwards. Other users might be able to query the volume properties while the volume is being created, but virStorageVolGetXMLDesc() won't reveal the passphrase to them.
An earlier patch description has mentioned the possibility of a libvirt user specifying only the encryption format (or not even the format), letting libvirt to choose the parameters and return them. This would require libvirt interface extensions, and it is not supported by this patch. --- src/storage_backend.c | 41 +++++++++++++++++++++++++++++++++++++++-- src/storage_backend_disk.c | 7 +++++++ src/storage_backend_fs.c | 7 +++++++ src/storage_backend_logical.c | 7 +++++++ src/storage_driver.c | 4 ++++ 5 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/src/storage_backend.c b/src/storage_backend.c index 1664804..42c59ad 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -246,6 +246,13 @@ virStorageBackendCreateRaw(virConnectPtr conn, unsigned long long remain; char *buf = NULL;
+ if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support encrypted " + "volumes")); + return -1; + } + if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, vol->target.perms.mode)) < 0) { virReportSystemError(conn, errno, @@ -346,15 +353,17 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, NULL;
const char **imgargv; + /* The extra NULL field is for indicating encryption (-e). */ const char *imgargvnormal[] = { NULL, "create", "-f", type, vol->target.path, size, NULL, + NULL }; /* Extra NULL fields are for including "backingType" when using - * kvm-img. It's -F backingType + * kvm-img (-F backingType), and for indicating encryption (-e). */ const char *imgargvbacking[] = { NULL, "create", @@ -364,6 +373,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, size, NULL, NULL, + NULL, NULL }; const char *convargv[] = { @@ -417,6 +427,22 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, } }
+ if (vol->encryption != NULL) { + if (vol->encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_QCOW) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + _("unsupported volume encryption format %d"), + vol->encryption->format); + return -1; + } + if (vol->target.format != VIR_STORAGE_VOL_FILE_QCOW && + vol->target.format != VIR_STORAGE_VOL_FILE_QCOW2) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + _("qcow volume encryption unsupported with " + "volume format %s"), type); + return -1; + } + } + if ((create_tool = virFindFileInPath("kvm-img")) != NULL) use_kvmimg = 1; else if ((create_tool = virFindFileInPath("qemu-img")) != NULL) @@ -437,11 +463,16 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, imgargvbacking[7] = backingType; imgargvbacking[8] = vol->target.path; imgargvbacking[9] = size; - } + if (vol->encryption != NULL) + imgargvbacking[10] = "-e"; + } else if (vol->encryption != NULL) + imgargvbacking[8] = "-e"; imgargv = imgargvbacking; } else { imgargvnormal[0] = create_tool; imgargv = imgargvnormal; + if (vol->encryption != NULL) + imgargv[6] = "-e"; }
@@ -490,6 +521,12 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn, "qcow-create")); return -1; } + if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("encrypted volumes not supported with " + "qcow-create")); + return -1; + }
/* Size in MB - yes different units to qemu-img :-( */ snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024); diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index ae2acae..8f4f0ed 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -557,6 +557,13 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, NULL };
+ if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support encrypted " + "volumes")); + return -1; + } + if (virStorageBackendDiskPartFormat(conn, pool, vol, partFormat) != 0) { return -1; } diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 0219eb6..b43daef 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -1070,6 +1070,13 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, int tool_type;
if (inputvol) { + if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support " + "building encrypted volumes from " + "other volumes")); + return -1; + } create_func = virStorageBackendGetBuildVolFromFunction(conn, vol, inputvol); if (!create_func) diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c index 6c123ae..822d321 100644 --- a/src/storage_backend_logical.c +++ b/src/storage_backend_logical.c @@ -581,6 +581,13 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, }; const char **cmdargv = cmdargvnew;
+ if (vol->encryption != NULL) { + virStorageReportError(conn, VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support encrypted " + "volumes")); + return -1; + } + if (vol->backingStore.path) { cmdargv = cmdargvsnap; } diff --git a/src/storage_driver.c b/src/storage_driver.c index e9ecb20..98db9a0 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -1305,6 +1305,8 @@ storageVolumeCreateXML(virStoragePoolPtr obj, cleanup: if (volobj) virUnrefStorageVol(volobj); + if (voldef) + virStorageEncryptionDropSecrets(voldef->encryption); virStorageVolDefFree(voldef); if (pool) virStoragePoolObjUnlock(pool); @@ -1466,6 +1468,8 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, cleanup: if (volobj) virUnrefStorageVol(volobj); + if (newvol) + virStorageEncryptionDropSecrets(newvol->encryption); virStorageVolDefFree(newvol); if (pool) virStoragePoolObjUnlock(pool);
As per other mails, I don't think its neccessary to drop the secrets from volumes here. At least not until we introduce keystores as an explicit public API Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
----- "Daniel P. Berrange" <berrange@redhat.com> wrote:
As per other mails, I don't think its neccessary to drop the secrets from volumes here. At least not until we introduce keystores as an explicit public API As per other mails, it is necessary not to tell the secrets to clients. Given that, libvirt does not need the secrets for anything, so it should not keep them in memory in case a bug allowed an attacker to read the memory. Mirek
The XML allows <encryption format='unencrypted'/>, this implementation canonicalizes the internal representation so that "disk->encryption" is non-NULL iff encryption information is available. Note that partial encryption information (e.g. specifying an encryption format, but not the key/passphrase) is valid: * virDomainGetXMLDesc() will only reveal the key/passphrase if VIR_DOMAIN_XML_SECURE is used. * A domain with partial encryption information can be defined, completenes of the information is not verified. The domain won't start until the remaining information is added, of course. --- src/domain_conf.c | 25 +++++++++++++++++++++++-- src/domain_conf.h | 2 ++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index f3e4c6c..507abd8 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -287,6 +287,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->dst); VIR_FREE(def->driverName); VIR_FREE(def->driverType); + virStorageEncryptionFree(def->encryption); VIR_FREE(def); } @@ -654,6 +655,7 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *target = NULL; char *bus = NULL; char *cachetag = NULL; + virStorageEncryptionPtr encryption = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -708,6 +710,17 @@ virDomainDiskDefParseXML(virConnectPtr conn, def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { def->shared = 1; + } else if (encryption == NULL && + xmlStrEqual(cur->name, BAD_CAST "encryption")) { + encryption = virStorageEncryptionParseNode(conn, node->doc, + cur); + if (encryption == NULL) + goto error; + if (encryption->format == + VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED) { + virStorageEncryptionFree(encryption); + encryption = NULL; + } } } cur = cur->next; @@ -815,6 +828,8 @@ virDomainDiskDefParseXML(virConnectPtr conn, driverName = NULL; def->driverType = driverType; driverType = NULL; + def->encryption = encryption; + encryption = NULL; cleanup: VIR_FREE(bus); @@ -825,6 +840,7 @@ cleanup: VIR_FREE(driverType); VIR_FREE(driverName); VIR_FREE(cachetag); + virStorageEncryptionFree(encryption); return def; @@ -3387,7 +3403,8 @@ virDomainLifecycleDefFormat(virConnectPtr conn, static int virDomainDiskDefFormat(virConnectPtr conn, virBufferPtr buf, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + int flags) { const char *type = virDomainDiskTypeToString(def->type); const char *device = virDomainDiskDeviceTypeToString(def->device); @@ -3444,6 +3461,10 @@ virDomainDiskDefFormat(virConnectPtr conn, virBufferAddLit(buf, " <readonly/>\n"); if (def->shared) virBufferAddLit(buf, " <shareable/>\n"); + if (def->encryption != NULL && + virStorageEncryptionFormat(conn, buf, def->encryption, + (flags & VIR_DOMAIN_XML_SECURE)) < 0) + return -1; virBufferAddLit(buf, " </disk>\n"); @@ -4047,7 +4068,7 @@ char *virDomainDefFormat(virConnectPtr conn, def->emulator); for (n = 0 ; n < def->ndisks ; n++) - if (virDomainDiskDefFormat(conn, &buf, def->disks[n]) < 0) + if (virDomainDiskDefFormat(conn, &buf, def->disks[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nfss ; n++) diff --git a/src/domain_conf.h b/src/domain_conf.h index 6e111fa..32d03ac 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -30,6 +30,7 @@ #include "internal.h" #include "capabilities.h" +#include "storage_encryption.h" #include "util.h" #include "threads.h" @@ -107,6 +108,7 @@ struct _virDomainDiskDef { unsigned int readonly : 1; unsigned int shared : 1; int slotnum; /* pci slot number for unattach */ + virStorageEncryptionPtr encryption; }; -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:12:01PM +0200, Miloslav Trma?? wrote:
The XML allows <encryption format='unencrypted'/>, this implementation canonicalizes the internal representation so that "disk->encryption" is non-NULL iff encryption information is available.
Note that partial encryption information (e.g. specifying an encryption format, but not the key/passphrase) is valid: * virDomainGetXMLDesc() will only reveal the key/passphrase if VIR_DOMAIN_XML_SECURE is used. * A domain with partial encryption information can be defined, completenes of the information is not verified. The domain won't start until the remaining information is added, of course. --- src/domain_conf.c | 25 +++++++++++++++++++++++-- src/domain_conf.h | 2 ++ 2 files changed, 25 insertions(+), 2 deletions(-)
This looks fine as a way to integrate. BTW, the XML schemas should be updated under docs/schemas/domain.rng and docs/schemas/storagevol.rng Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
----- "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jul 21, 2009 at 01:12:01PM +0200, Miloslav Trma?? wrote: This looks fine as a way to integrate. BTW, the XML schemas should be updated under docs/schemas/domain.rng and docs/schemas/storagevol.rng Will do. Mirek
The if ((nlptr...)) implicitly assumes commptr != NULL. Make the assumption explicit, it will be broken in a future patch. --- src/qemu_driver.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d2db1a2..f2be27f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2081,10 +2081,11 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, * occurence, and inbetween the command and the newline starting * the response */ - if ((commptr = strstr(buf, cmd))) + if ((commptr = strstr(buf, cmd))) { memmove(buf, commptr, strlen(commptr)+1); - if ((nlptr = strchr(buf, '\n'))) - memmove(buf+strlen(cmd), nlptr, strlen(nlptr)+1); + if ((nlptr = strchr(buf, '\n'))) + memmove(buf+strlen(cmd), nlptr, strlen(nlptr)+1); + } break; } -- 1.6.2.5
--- src/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 73 insertions(+), 14 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f2be27f..12079f8 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -87,6 +87,12 @@ static void qemuDriverUnlock(struct qemud_driver *driver) virMutexUnlock(&driver->lock); } +/* Return -1 for error, 0 for success */ +typedef int qemudMonitorExtraPromptHandler(const virDomainObjPtr vm, + const char *buf, + const char *prompt, + void *data); + static void qemuDomainEventFlush(int timer, void *opaque); static void qemuDomainEventQueue(struct qemud_driver *driver, virDomainEventPtr event); @@ -111,6 +117,12 @@ static int qemudDomainGetMaxVcpus(virDomainPtr dom); static int qemudMonitorCommand(const virDomainObjPtr vm, const char *cmd, char **reply); +static int qemudMonitorCommandWithHandler(const virDomainObjPtr vm, + const char *cmd, + const char *extraPrompt, + qemudMonitorExtraPromptHandler extraHandler, + void *handlerData, + char **reply); static int qemudMonitorCommandExtra(const virDomainObjPtr vm, const char *cmd, const char *extra, @@ -2014,15 +2026,15 @@ qemuMonitorDiscardPendingData(virDomainObjPtr vm) { static int -qemudMonitorCommandExtra(const virDomainObjPtr vm, - const char *cmd, - const char *extra, - const char *extraPrompt, - char **reply) { +qemudMonitorCommandWithHandler(const virDomainObjPtr vm, + const char *cmd, + const char *extraPrompt, + qemudMonitorExtraPromptHandler extraHandler, + void *handlerData, + char **reply) { int size = 0; char *buf = NULL; size_t cmdlen = strlen(cmd); - size_t extralen = extra ? strlen(extra) : 0; qemuMonitorDiscardPendingData(vm); @@ -2061,14 +2073,20 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, /* Look for QEMU prompt to indicate completion */ if (buf) { - if (extra) { - if (strstr(buf, extraPrompt) != NULL) { - if (safewrite(vm->monitor, extra, extralen) != extralen) - return -1; - if (safewrite(vm->monitor, "\r", 1) != 1) - return -1; - extra = NULL; - } + char *foundPrompt; + + if (extraPrompt && + (foundPrompt = strstr(buf, extraPrompt)) != NULL) { + char *promptEnd; + + if (extraHandler(vm, buf, foundPrompt, handlerData) < 0) + return -1; + /* Discard output so far, necessary to detect whether + extraPrompt appears again. We don't need the output between + original command and this prompt anyway. */ + promptEnd = foundPrompt + strlen(extraPrompt); + memmove(buf, promptEnd, strlen(promptEnd)+1); + size -= promptEnd - buf; } else if ((tmp = strstr(buf, QEMU_CMD_PROMPT)) != NULL) { char *commptr = NULL, *nlptr = NULL; /* Preserve the newline */ @@ -2106,6 +2124,47 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, return -1; } +struct extraHandlerData +{ + const char *reply; + bool first; +}; + +static int +qemudMonitorCommandSimpleExtraHandler(const virDomainObjPtr vm, + const char *buf ATTRIBUTE_UNUSED, + const char *prompt ATTRIBUTE_UNUSED, + void *data_) +{ + struct extraHandlerData *data = data_; + size_t len; + + if (!data->first) + return 0; + len = strlen(data->reply); + if (safewrite(vm->monitor, data->reply, len) != len) + return -1; + if (safewrite(vm->monitor, "\r", 1) != 1) + return -1; + data->first = false; + return 0; +} + +static int +qemudMonitorCommandExtra(const virDomainObjPtr vm, + const char *cmd, + const char *extra, + const char *extraPrompt, + char **reply) { + struct extraHandlerData data; + + data.reply = extra; + data.first = true; + return qemudMonitorCommandWithHandler(vm, cmd, extraPrompt, + qemudMonitorCommandSimpleExtraHandler, + &data, reply); +} + static int qemudMonitorCommand(const virDomainObjPtr vm, const char *cmd, -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:12:03PM +0200, Miloslav Trma?? wrote:
--- src/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 73 insertions(+), 14 deletions(-)
I have a feeling this patch is going to clash horribly with the refactoring Mark just did of the monitor code for FD passing. Your goal seems sane here - we'll just need to resolve this against Mark's code Daniel
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f2be27f..12079f8 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -87,6 +87,12 @@ static void qemuDriverUnlock(struct qemud_driver *driver) virMutexUnlock(&driver->lock); }
+/* Return -1 for error, 0 for success */ +typedef int qemudMonitorExtraPromptHandler(const virDomainObjPtr vm, + const char *buf, + const char *prompt, + void *data); + static void qemuDomainEventFlush(int timer, void *opaque); static void qemuDomainEventQueue(struct qemud_driver *driver, virDomainEventPtr event); @@ -111,6 +117,12 @@ static int qemudDomainGetMaxVcpus(virDomainPtr dom); static int qemudMonitorCommand(const virDomainObjPtr vm, const char *cmd, char **reply); +static int qemudMonitorCommandWithHandler(const virDomainObjPtr vm, + const char *cmd, + const char *extraPrompt, + qemudMonitorExtraPromptHandler extraHandler, + void *handlerData, + char **reply); static int qemudMonitorCommandExtra(const virDomainObjPtr vm, const char *cmd, const char *extra, @@ -2014,15 +2026,15 @@ qemuMonitorDiscardPendingData(virDomainObjPtr vm) {
static int -qemudMonitorCommandExtra(const virDomainObjPtr vm, - const char *cmd, - const char *extra, - const char *extraPrompt, - char **reply) { +qemudMonitorCommandWithHandler(const virDomainObjPtr vm, + const char *cmd, + const char *extraPrompt, + qemudMonitorExtraPromptHandler extraHandler, + void *handlerData, + char **reply) { int size = 0; char *buf = NULL; size_t cmdlen = strlen(cmd); - size_t extralen = extra ? strlen(extra) : 0;
qemuMonitorDiscardPendingData(vm);
@@ -2061,14 +2073,20 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm,
/* Look for QEMU prompt to indicate completion */ if (buf) { - if (extra) { - if (strstr(buf, extraPrompt) != NULL) { - if (safewrite(vm->monitor, extra, extralen) != extralen) - return -1; - if (safewrite(vm->monitor, "\r", 1) != 1) - return -1; - extra = NULL; - } + char *foundPrompt; + + if (extraPrompt && + (foundPrompt = strstr(buf, extraPrompt)) != NULL) { + char *promptEnd; + + if (extraHandler(vm, buf, foundPrompt, handlerData) < 0) + return -1; + /* Discard output so far, necessary to detect whether + extraPrompt appears again. We don't need the output between + original command and this prompt anyway. */ + promptEnd = foundPrompt + strlen(extraPrompt); + memmove(buf, promptEnd, strlen(promptEnd)+1); + size -= promptEnd - buf; } else if ((tmp = strstr(buf, QEMU_CMD_PROMPT)) != NULL) { char *commptr = NULL, *nlptr = NULL; /* Preserve the newline */ @@ -2106,6 +2124,47 @@ qemudMonitorCommandExtra(const virDomainObjPtr vm, return -1; }
+struct extraHandlerData +{ + const char *reply; + bool first; +}; + +static int +qemudMonitorCommandSimpleExtraHandler(const virDomainObjPtr vm, + const char *buf ATTRIBUTE_UNUSED, + const char *prompt ATTRIBUTE_UNUSED, + void *data_) +{ + struct extraHandlerData *data = data_; + size_t len; + + if (!data->first) + return 0; + len = strlen(data->reply); + if (safewrite(vm->monitor, data->reply, len) != len) + return -1; + if (safewrite(vm->monitor, "\r", 1) != 1) + return -1; + data->first = false; + return 0; +} + +static int +qemudMonitorCommandExtra(const virDomainObjPtr vm, + const char *cmd, + const char *extra, + const char *extraPrompt, + char **reply) { + struct extraHandlerData data; + + data.reply = extra; + data.first = true; + return qemudMonitorCommandWithHandler(vm, cmd, extraPrompt, + qemudMonitorCommandSimpleExtraHandler, + &data, reply); +} + static int qemudMonitorCommand(const virDomainObjPtr vm, const char *cmd, -- 1.6.2.5
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
----- "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jul 21, 2009 at 01:12:03PM +0200, Miloslav Trma?? wrote:
--- src/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 73 insertions(+), 14 deletions(-)
I have a feeling this patch is going to clash horribly with the refactoring Mark just did of the monitor code for FD passing. Your goal seems sane here - we'll just need to resolve this against Mark's code I'll help as much as I can - ping me on IRC if necessary. Mirek
Also fix a potential infinite loop in qemuDomainCoreDump() if sending cont repeatedly fails. --- src/qemu_driver.c | 43 ++++++++++++++++++------------------------- 1 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 12079f8..9ead5fd 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -128,6 +128,7 @@ static int qemudMonitorCommandExtra(const virDomainObjPtr vm, const char *extra, const char *extraPrompt, char **reply); +static int qemudMonitorSendCont(const virDomainObjPtr vm); static int qemudDomainSetMemoryBalloon(virConnectPtr conn, virDomainObjPtr vm, unsigned long newmem); @@ -1199,7 +1200,6 @@ static int qemudInitCpus(virConnectPtr conn, virDomainObjPtr vm, const char *migrateFrom) { - char *info = NULL; #if HAVE_SCHED_GETAFFINITY cpu_set_t mask; int i, maxcpu = QEMUD_CPUMASK_LEN; @@ -1235,12 +1235,11 @@ qemudInitCpus(virConnectPtr conn, if (migrateFrom == NULL) { /* Allow the CPUS to start executing */ - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); return -1; } - VIR_FREE(info); } return 0; @@ -2172,6 +2171,17 @@ qemudMonitorCommand(const virDomainObjPtr vm, return qemudMonitorCommandExtra(vm, cmd, NULL, NULL, reply); } +static int +qemudMonitorSendCont(const virDomainObjPtr vm) { + char *reply; + + if (qemudMonitorCommand(vm, "cont", &reply) < 0) + return -1; + qemudDebug ("%s: cont reply: %s", vm->def->name, info); + VIR_FREE(reply); + return 0; +} + static virDrvOpenStatus qemudOpen(virConnectPtr conn, @@ -2633,7 +2643,6 @@ cleanup: static int qemudDomainResume(virDomainPtr dom) { struct qemud_driver *driver = dom->conn->privateData; - char *info; virDomainObjPtr vm; int ret = -1; virDomainEventPtr event = NULL; @@ -2654,17 +2663,15 @@ static int qemudDomainResume(virDomainPtr dom) { goto cleanup; } if (vm->state == VIR_DOMAIN_PAUSED) { - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("resume operation failed")); goto cleanup; } vm->state = VIR_DOMAIN_RUNNING; - qemudDebug("Reply %s", info); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_UNPAUSED); - VIR_FREE(info); } if (virDomainSaveStatus(dom->conn, driver->stateDir, vm) < 0) goto cleanup; @@ -3349,13 +3356,9 @@ cleanup: will support synchronous operations so we always get here after the migration is complete. */ if (resume && paused) { - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("resuming after dump failed")); - goto cleanup; - } - DEBUG ("%s: cont reply: %s", vm->def->name, info); - VIR_FREE(info); } if (vm) virDomainObjUnlock(vm); @@ -3824,13 +3827,11 @@ static int qemudDomainRestore(virConnectPtr conn, /* If it was running before, resume it now. */ if (header.was_running) { - char *info; - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("failed to resume domain")); goto cleanup; } - VIR_FREE(info); vm->state = VIR_DOMAIN_RUNNING; } ret = 0; @@ -5645,14 +5646,9 @@ qemudDomainMigratePerform (virDomainPtr dom, ret = 0; cleanup: - /* Note that we have to free info *first*, since we are re-using the - * variable below (and otherwise might cause a memory leak) - */ - VIR_FREE(info); - if (paused) { /* we got here through some sort of failure; start the domain again */ - if (qemudMonitorCommand (vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { /* Hm, we already know we are in error here. We don't want to * overwrite the previous error, though, so we just throw something * to the logs and hope for the best @@ -5660,16 +5656,13 @@ cleanup: VIR_ERROR(_("Failed to resume guest %s after failure\n"), vm->def->name); } - else { - DEBUG ("%s: cont reply: %s", vm->def->name, info); - VIR_FREE(info); - } event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_MIGRATED); } + VIR_FREE(info); if (vm) virDomainObjUnlock(vm); if (event) -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:12:04PM +0200, Miloslav Trma?? wrote:
Also fix a potential infinite loop in qemuDomainCoreDump() if sending cont repeatedly fails. --- src/qemu_driver.c | 43 ++++++++++++++++++------------------------- 1 files changed, 18 insertions(+), 25 deletions(-)
Looks good to me. Daniel
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 12079f8..9ead5fd 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -128,6 +128,7 @@ static int qemudMonitorCommandExtra(const virDomainObjPtr vm, const char *extra, const char *extraPrompt, char **reply); +static int qemudMonitorSendCont(const virDomainObjPtr vm); static int qemudDomainSetMemoryBalloon(virConnectPtr conn, virDomainObjPtr vm, unsigned long newmem); @@ -1199,7 +1200,6 @@ static int qemudInitCpus(virConnectPtr conn, virDomainObjPtr vm, const char *migrateFrom) { - char *info = NULL; #if HAVE_SCHED_GETAFFINITY cpu_set_t mask; int i, maxcpu = QEMUD_CPUMASK_LEN; @@ -1235,12 +1235,11 @@ qemudInitCpus(virConnectPtr conn,
if (migrateFrom == NULL) { /* Allow the CPUS to start executing */ - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); return -1; } - VIR_FREE(info); }
return 0; @@ -2172,6 +2171,17 @@ qemudMonitorCommand(const virDomainObjPtr vm, return qemudMonitorCommandExtra(vm, cmd, NULL, NULL, reply); }
+static int +qemudMonitorSendCont(const virDomainObjPtr vm) { + char *reply; + + if (qemudMonitorCommand(vm, "cont", &reply) < 0) + return -1; + qemudDebug ("%s: cont reply: %s", vm->def->name, info); + VIR_FREE(reply); + return 0; +} +
static virDrvOpenStatus qemudOpen(virConnectPtr conn, @@ -2633,7 +2643,6 @@ cleanup:
static int qemudDomainResume(virDomainPtr dom) { struct qemud_driver *driver = dom->conn->privateData; - char *info; virDomainObjPtr vm; int ret = -1; virDomainEventPtr event = NULL; @@ -2654,17 +2663,15 @@ static int qemudDomainResume(virDomainPtr dom) { goto cleanup; } if (vm->state == VIR_DOMAIN_PAUSED) { - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("resume operation failed")); goto cleanup; } vm->state = VIR_DOMAIN_RUNNING; - qemudDebug("Reply %s", info); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_UNPAUSED); - VIR_FREE(info); } if (virDomainSaveStatus(dom->conn, driver->stateDir, vm) < 0) goto cleanup; @@ -3349,13 +3356,9 @@ cleanup: will support synchronous operations so we always get here after the migration is complete. */ if (resume && paused) { - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("resuming after dump failed")); - goto cleanup; - } - DEBUG ("%s: cont reply: %s", vm->def->name, info); - VIR_FREE(info); } if (vm) virDomainObjUnlock(vm); @@ -3824,13 +3827,11 @@ static int qemudDomainRestore(virConnectPtr conn,
/* If it was running before, resume it now. */ if (header.was_running) { - char *info; - if (qemudMonitorCommand(vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("failed to resume domain")); goto cleanup; } - VIR_FREE(info); vm->state = VIR_DOMAIN_RUNNING; } ret = 0; @@ -5645,14 +5646,9 @@ qemudDomainMigratePerform (virDomainPtr dom, ret = 0;
cleanup: - /* Note that we have to free info *first*, since we are re-using the - * variable below (and otherwise might cause a memory leak) - */ - VIR_FREE(info); - if (paused) { /* we got here through some sort of failure; start the domain again */ - if (qemudMonitorCommand (vm, "cont", &info) < 0) { + if (qemudMonitorSendCont(vm) < 0) { /* Hm, we already know we are in error here. We don't want to * overwrite the previous error, though, so we just throw something * to the logs and hope for the best @@ -5660,16 +5656,13 @@ cleanup: VIR_ERROR(_("Failed to resume guest %s after failure\n"), vm->def->name); } - else { - DEBUG ("%s: cont reply: %s", vm->def->name, info); - VIR_FREE(info); - }
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_MIGRATED); }
+ VIR_FREE(info); if (vm) virDomainObjUnlock(vm); if (event) -- 1.6.2.5
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--- src/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 50 insertions(+), 1 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 9ead5fd..b57db31 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2172,10 +2172,59 @@ qemudMonitorCommand(const virDomainObjPtr vm, } static int +qemudMonitorSendVolumePassphrase(const virDomainObjPtr vm, + const char *buf, + const char *prompt, + void *data ATTRIBUTE_UNUSED) +{ + const char *path; + size_t path_len; + int i; + + /* The complete prompt looks like this: + ide0-hd0 (/path/to/volume) is encrypted. + Password: + prompt starts with ") is encrypted". Extract /path/to/volume. */ + for (path = prompt; path > buf && path[-1] != '('; path-- ) + ; + if (path == buf) + return -1; + path_len = prompt - path; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk; + + disk = vm->def->disks[i]; + if (disk->src != NULL && memcmp(disk->src, path, path_len) == 0 && + disk->src[path_len] == '\0' && + disk->encryption != NULL && + disk->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_QCOW) { + const char *passphrase; + + passphrase = disk->encryption->v.qcow.passphrase; + if (passphrase != NULL) { + size_t passphrase_len; + + passphrase_len = strlen(passphrase); + if (safewrite(vm->monitor, passphrase, passphrase_len) != + passphrase_len) + return -1; + if (safewrite(vm->monitor, "\r", 1) != 1) + return -1; + return 0; + } + } + } + return -1; +} + +static int qemudMonitorSendCont(const virDomainObjPtr vm) { char *reply; - if (qemudMonitorCommand(vm, "cont", &reply) < 0) + if (qemudMonitorCommandWithHandler(vm, "cont", ") is encrypted.", + qemudMonitorSendVolumePassphrase, + NULL, &reply) < 0) return -1; qemudDebug ("%s: cont reply: %s", vm->def->name, info); VIR_FREE(reply); -- 1.6.2.5
On Tue, Jul 21, 2009 at 01:12:05PM +0200, Miloslav Trma?? wrote:
--- src/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 50 insertions(+), 1 deletions(-)
IIUC this should only be needed when starting a QEMU guest. The 'cont' method is called from quite a few different contexts. So it might be nice to pass a flag down from qemudStartVMDaemon such that the passphrase handling is guarenteed to only be run during startup (qemudStartVMDaemon is also used for migrate and restore, so covers those scenarios OK) Daniel
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 9ead5fd..b57db31 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -2172,10 +2172,59 @@ qemudMonitorCommand(const virDomainObjPtr vm, }
static int +qemudMonitorSendVolumePassphrase(const virDomainObjPtr vm, + const char *buf, + const char *prompt, + void *data ATTRIBUTE_UNUSED) +{ + const char *path; + size_t path_len; + int i; + + /* The complete prompt looks like this: + ide0-hd0 (/path/to/volume) is encrypted. + Password: + prompt starts with ") is encrypted". Extract /path/to/volume. */ + for (path = prompt; path > buf && path[-1] != '('; path-- ) + ; + if (path == buf) + return -1; + path_len = prompt - path; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk; + + disk = vm->def->disks[i]; + if (disk->src != NULL && memcmp(disk->src, path, path_len) == 0 && + disk->src[path_len] == '\0' && + disk->encryption != NULL && + disk->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_QCOW) { + const char *passphrase; + + passphrase = disk->encryption->v.qcow.passphrase; + if (passphrase != NULL) { + size_t passphrase_len; + + passphrase_len = strlen(passphrase); + if (safewrite(vm->monitor, passphrase, passphrase_len) != + passphrase_len) + return -1; + if (safewrite(vm->monitor, "\r", 1) != 1) + return -1; + return 0; + } + } + } + return -1; +} + +static int qemudMonitorSendCont(const virDomainObjPtr vm) { char *reply;
- if (qemudMonitorCommand(vm, "cont", &reply) < 0) + if (qemudMonitorCommandWithHandler(vm, "cont", ") is encrypted.", + qemudMonitorSendVolumePassphrase, + NULL, &reply) < 0) return -1; qemudDebug ("%s: cont reply: %s", vm->def->name, info); VIR_FREE(reply); -- 1.6.2.5
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Jul 21, 2009 at 01:11:56PM +0200, Miloslav Trma?? wrote:
Hello, the following patches add full support for qcow/qcow2 volume encryption, assuming a client that supports it.
New XML tags are defined to represent encryption parameters (currently format and passphrase, more can be added in the future), e.g. <encryption format='qcow'> <passphrase>c2lsbHk=</passphrase> </encryption> (passphrase content uses base64)
I don't think we need 'format=qcow' in there - the guest XML already has ability to have a <driver name='qemu' type='qcow2'/> element, and the storage vol XML also has a <format type='qcow2'/> element. I think it would be good to change the naming of the inner element a little too. I think having it called 'secret' and then a 'type' attribute would be a little nicer. eg <encryption> <secret type='passphrase'/>c2lsbHk=</secret> </encryption> It might be desirable to add encryption algorithm, but that can probably wait since qcow doesn't support multiple algorithms at this time.
The <encryption> tag can be added to a <volume> node passed to virStorageVolCreateXML() to create an encrypted volume, or to a <disk> node inside a <domain> to specify what encryption parameters to use for a domain. If the domain is persistent, the parameters (including the passphrase) will be saved unencrypted in /etc/libvirtd; the primary use case is to store the parameters outside of libvirtd, (perhaps by virt-manager in a GNOME keyring).
Possible enhancements: - Documentation and test cases. I'll write both if the code is acceptable, I wanted to make the code available for review first. - Support for "dumb" clients that don't know anything about encryption formats and the required parameters: adding an encryption format to libvirt would automatically make it supported in all clients.
Such a client would only request that a volume should be created when creating it, and libvirt would choose an appropriate format, parameters and passphrase/key and return it to the client, who could later pass it unmodified inside a <domain>.
This requires public API additions to let libvirt return the encryption information as one of the results of a volume creation operation.
- Support for storing the passphrases/keys used by persistent domains outside of the main XML files, e.g. in a separate passphrase-encrypted file that must be entered on libvirtd startup.
I think these two points overlap quite alot. I'll use 'secret' to refer to a passphrase or key here As you say there are two initial approaches to persistence of secrets - Keep the keys in the domain XML files - Use a separate keystore For the separate keystore, there are probably 3 interesting targets to consider 1. A simple text (or pkcs11) file managed by libvirtd on the host This would be useful for the privileged libvirtd to use in ad-hoc, small scale deployments. Perhaps allowing it to be shared between a small number of hosts on NFS, or GFS etc 2. A desktop key agent (eg gnome-keyring) This would be useful for the unprivileged libvirtd instances that run in the context of the desktop login session. Users already have SSH, GPG, website keys stored here, so having keys for their VM disks is obviously desirable 3. A off-node key management server This would be useful for a large scale data center / cluster cloud deployment. This is good to allow management scalability and better separation of responsiblities of adminstration. If no keystore is in use, then I clearly all keys must go in & out of libvirt using the XML, which is pretty much what you're doing in this series. I would say though, there is no point in clearing the secret from the virStorageVolDefPtr instance/XML after volume creation, since the secret is going to be kept in memory forever for any guest using the volume. By not clearing the secret, an app could create a volume, requesting automatic key generation, then just do virStorageVolDumpXML(vol, VIR_STORAGE_VOL_XML_SECURE) to extract it and pass to onto the XML for the guest that's created If a keystore is in use, then I'd suggest we should have explicit APIs for secret management, and forbid the use of the actual secrets in the XML everywhere. Instead, either pass in a pre-created key UUID when creating a volume or defining a guest eg, the XML fragment would look like this <encryption> <secret type='keyuuid'>123456-1234-1245-1234-124512345</secret> </encryption> Internally libvirt would then get the real passphrase from the keystore by looking up the associated UUID. Or request auto-creation of a key by leaving out the data <encryption> <secret type='keyuuid'/> </encryption> Internally libvirt would add a key to its database, and fillin the UUID which you could then see via DumpXML. With a keystore we'd likely need a handful of APIs - create a secret providing a passphrase - list all known secret UUIDs - get the passphrase assoicated with a secret - delete a secret based on UUID - lookup a secret UUID based on the a disk path The nice thing about separating the passphrases out of the XML completely is that we would then have the ability to do fine grained access control. eg, you could give people ability to create new guests / volumes, using encryption without them ever specifying, or having access to the secrets. A seperate role could be given ability to create/list/delete secrets. This would also let us associate one secret with many disks, which might be a useful scenario for some people. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Miloslav Trmac -
Miloslav Trmač