2 p.m.
v2 here:
http://www.redhat.com/archives/libvir-list/2016-May/msg00067.html
First 6 patches from v2 were pushed since they were ACK'd
I modified the 7th patch to add the ATTRIBUTE_UNUSED to the static
functions qemuDomainSecretHaveEncypt and qemuDomainSecretIVSetup and
merged in the HAVE_GNUTLS_CIPHER_ENCRYPT.
This should address Michal's point in patch 7 review.
Patch 2 thus becomes the "remainder" of the work necessary in order
to get IV Secrets available for RBD volumes. It's most of the former
patch 8 with removal if iSCSI oddities.
Patch 8 will then remove the ATTRIBUTE_UNUSED from the 2 static functions
now that qemuDomainSecretSetup exists to call it. The change here was
to add the check for protocol == VIR_STORAGE_NET_PROTOCOL_RBD before
"defaulting" to using the IV secret (e.g., the magic decision point).
Since the command code didn't need the secret setup for iSCSI, it's been
extracted to another safe place just in case it needs to be resurrected.
John Ferlan (2):
qemu: Introduce new Secret IV API's
qemu: Utilize qemu secret objects for RBD auth/secret
configure.ac | 1 +
src/qemu/qemu_alias.c | 23 +++
src/qemu/qemu_alias.h | 2 +
src/qemu/qemu_command.c | 130 +++++++++++++-
src/qemu/qemu_domain.c | 189 ++++++++++++++++++++-
...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 ++++
...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 +++++
tests/qemuxml2argvmock.c | 31 +++-
tests/qemuxml2argvtest.c | 11 ++
9 files changed, 451 insertions(+), 9 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml
--
2.5.5
2 p.m.
New subject: [libvirt] [PATCH v3 1/2] qemu: Introduce new Secret IV API's
New APIs:
qemuDomainGetIVKeyAlias:
Generate/return the secret object alias for an initialization
vector (IV) secret info type. This will be saved in the secret
info block. This will be called from qemuDomainSecretIVSetup.
qemuDomainSecretHaveEncrypt: (private)
Boolean function to determine whether the underly encryption
API is available. This function will utilize a similar mechanism
as the 'gnutls_rnd' did in configure.ac.
qemuDomainSecretIVSetup: (private)
This API handles the details of the generation of the IV secret
and saves the pieces that need to be passed to qemu in order for
the secret to be decrypted. The encrypted secret based upon the
domain master key, an initialization vector (16 byte random value),
and the stored secret. Finally, the requirement from qemu is the IV
and encrypted secret are to be base64 encoded. They can be passed
either directly or within a file. This implementation chooses
to pass directly rather than a file.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
configure.ac | 1 +
src/qemu/qemu_alias.c | 23 +++++++++
src/qemu/qemu_alias.h | 2 +
src/qemu/qemu_domain.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 163 insertions(+)
diff --git a/configure.ac b/configure.ac
index 88e2e20..3cabd5e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1264,6 +1264,7 @@ if test "x$with_gnutls" != "xno"; then
]])
AC_CHECK_FUNCS([gnutls_rnd])
+ AC_CHECK_FUNCS([gnutls_cipher_encrypt])
CFLAGS="$old_CFLAGS"
LIBS="$old_LIBS"
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index ade2033..de9a74f 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -556,3 +556,26 @@ qemuDomainGetMasterKeyAlias(void)
return alias;
}
+
+
+/* qemuDomainGetIVKeyAlias:
+ *
+ * Generate and return an initialization vector alias
+ *
+ * Returns NULL or a string containing the IV key alias
+ */
+char *
+qemuDomainGetIVKeyAlias(const char *srcalias)
+{
+ char *alias;
+
+ if (!srcalias) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("secret iv alias requires valid source alias"));
+ return NULL;
+ }
+
+ ignore_value(virAsprintf(&alias, "%s-ivKey0", srcalias));
+
+ return alias;
+}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 2d7bc9b..435747e 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -69,4 +69,6 @@ char *qemuAliasFromDisk(const virDomainDiskDef *disk);
char *qemuDomainGetMasterKeyAlias(void);
+char *qemuDomainGetIVKeyAlias(const char *srcalias);
+
#endif /* __QEMU_ALIAS_H__*/
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cae356c..b174caa 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -893,6 +893,143 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
}
+/* qemuDomainSecretHaveEncypt:
+ *
+ * Returns true if we can support the encryption code, false otherwise
+ */
+static bool ATTRIBUTE_UNUSED
+qemuDomainSecretHaveEncrypt(void)
+{
+#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
+ return true;
+#else
+ return false;
+#endif
+}
+
+
+/* qemuDomainSecretIVSetup:
+ * @conn: Pointer to connection
+ * @priv: pointer to domain private object
+ * @secinfo: Pointer to secret info
+ * @srcalias: Alias of the disk/hostdev used to generate the secret alias
+ * @protocol: Protocol for secret
+ * @authdef: Pointer to auth data
+ *
+ * Taking a secinfo, fill in the initialization vector information
+ *
+ * Returns 0 on success, -1 on failure with error message
+ */
+static int ATTRIBUTE_UNUSED
+qemuDomainSecretIVSetup(virConnectPtr conn,
+ qemuDomainObjPrivatePtr priv,
+ qemuDomainSecretInfoPtr secinfo,
+ const char *srcalias,
+ virStorageNetProtocol protocol,
+ virStorageAuthDefPtr authdef)
+{
+ int ret = -1;
+ int rc;
+ uint8_t *raw_iv = NULL;
+ char *secret = NULL;
+ uint8_t *ciphertext = NULL;
+ size_t secretlen = 0, ciphertextlen = 0, paddinglen;
+ int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
+ const char *protocolstr = virStorageNetProtocolTypeToString(protocol);
+ gnutls_cipher_hd_t handle = NULL;
+ gnutls_datum_t enc_key;
+ gnutls_datum_t iv_key;
+
+ secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV;
+ if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0)
+ return -1;
+
+ if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
+ secretType = VIR_SECRET_USAGE_TYPE_CEPH;
+
+ if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias)))
+ return -1;
+
+ /* Create a random initialization vector */
+ if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN)))
+ return -1;
+
+ /* Encode the IV and save that since qemu will need it */
+ base64_encode_alloc((const char *)raw_iv, QEMU_DOMAIN_IV_KEY_LEN,
+ &secinfo->s.iv.iv);
+ if (!secinfo->s.iv.iv) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to encode initialization vector"));
+ goto cleanup;
+ }
+
+ /* Grab the unencoded secret */
+ if (!(secret = virSecretGetSecretString(conn, protocolstr, false,
+ authdef, secretType)))
+ goto cleanup;
+
+ /* Allocate a padded buffer */
+ secretlen = strlen(secret);
+ paddinglen = 16 - (secretlen % 16);
+ ciphertextlen = secretlen + paddinglen;
+ if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
+ goto cleanup;
+ memcpy(ciphertext, secret, secretlen);
+ memset(ciphertext + secretlen, paddinglen, paddinglen);
+
+ /* clear secret so that it doesn't hang around */
+ memset(secret, 0, strlen(secret));
+
+ /* Initialize the gnutls cipher */
+ enc_key.size = QEMU_DOMAIN_MASTER_KEY_LEN;
+ enc_key.data = priv->masterKey;
+ iv_key.size = QEMU_DOMAIN_IV_KEY_LEN;
+ iv_key.data = raw_iv;
+ if ((rc = gnutls_cipher_init(&handle, GNUTLS_CIPHER_AES_256_CBC,
+ &enc_key, &iv_key)) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to initialize cipher: '%s'"),
+ gnutls_strerror(rc));
+ goto cleanup;
+ }
+
+ /* Encrypt the secret and free the memory for cipher operations */
+ rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen);
+ gnutls_cipher_deinit(handle);
+ if (rc < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to encrypt the secret: '%s'"),
+ gnutls_strerror(rc));
+ goto cleanup;
+ }
+
+ /* Now base64 encode the ciphertext and store to be passed to qemu */
+ base64_encode_alloc((const char *)ciphertext, ciphertextlen,
+ &secinfo->s.iv.ciphertext);
+ if (!secinfo->s.iv.ciphertext) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to encode ciphertext"));
+ goto cleanup;
+ }
+
+ ret = 0;
+
+ cleanup:
+
+ /* Clear and free the raw_iv, plaintext secret, and ciphertext */
+ memset(raw_iv, 0, QEMU_DOMAIN_IV_KEY_LEN);
+ VIR_FREE(raw_iv);
+
+ memset(secret, 0, secretlen);
+ VIR_FREE(secret);
+
+ memset(ciphertext, 0, ciphertextlen);
+ VIR_FREE(ciphertext);
+
+ return ret;
+}
+
+
/* qemuDomainSecretDiskDestroy:
* @disk: Pointer to a disk definition
*
--
2.5.5
7:47 a.m.
New subject: [libvirt] [PATCH v3 1/2] qemu: Introduce new Secret IV API's
On Thu, May 05, 2016 at 15:00:40 -0400, John Ferlan wrote:
New APIs:
qemuDomainGetIVKeyAlias:
Generate/return the secret object alias for an initialization
vector (IV) secret info type. This will be saved in the secret
info block. This will be called from qemuDomainSecretIVSetup.
qemuDomainSecretHaveEncrypt: (private)
Boolean function to determine whether the underly encryption
API is available. This function will utilize a similar mechanism
as the 'gnutls_rnd' did in configure.ac.
qemuDomainSecretIVSetup: (private)
This API handles the details of the generation of the IV secret
and saves the pieces that need to be passed to qemu in order for
the secret to be decrypted. The encrypted secret based upon the
domain master key, an initialization vector (16 byte random value),
and the stored secret. Finally, the requirement from qemu is the IV
and encrypted secret are to be base64 encoded. They can be passed
either directly or within a file. This implementation chooses
to pass directly rather than a file.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
configure.ac | 1 +
src/qemu/qemu_alias.c | 23 +++++++++
src/qemu/qemu_alias.h | 2 +
src/qemu/qemu_domain.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 163 insertions(+)
diff --git a/configure.ac b/configure.ac
index 88e2e20..3cabd5e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1264,6 +1264,7 @@ if test "x$with_gnutls" != "xno"; then
]])
AC_CHECK_FUNCS([gnutls_rnd])
+ AC_CHECK_FUNCS([gnutls_cipher_encrypt])
CFLAGS="$old_CFLAGS"
LIBS="$old_LIBS"
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index ade2033..de9a74f 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -556,3 +556,26 @@ qemuDomainGetMasterKeyAlias(void)
return alias;
}
+
+
+/* qemuDomainGetIVKeyAlias:
+ *
+ * Generate and return an initialization vector alias
+ *
+ * Returns NULL or a string containing the IV key alias
+ */
+char *
+qemuDomainGetIVKeyAlias(const char *srcalias)
+{
+ char *alias;
+
+ if (!srcalias) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("secret iv alias requires valid source alias"));
+ return NULL;
+ }
+
+ ignore_value(virAsprintf(&alias, "%s-ivKey0", srcalias));
+
+ return alias;
+}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 2d7bc9b..435747e 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -69,4 +69,6 @@ char *qemuAliasFromDisk(const virDomainDiskDef *disk);
char *qemuDomainGetMasterKeyAlias(void);
+char *qemuDomainGetIVKeyAlias(const char *srcalias);
+
#endif /* __QEMU_ALIAS_H__*/
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cae356c..b174caa 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -893,6 +893,143 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
}
+/* qemuDomainSecretHaveEncypt:
+ *
+ * Returns true if we can support the encryption code, false otherwise
+ */
+static bool ATTRIBUTE_UNUSED
+qemuDomainSecretHaveEncrypt(void)
+{
+#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
+ return true;
+#else
+ return false;
+#endif
+}
+
+
+/* qemuDomainSecretIVSetup:
+ * @conn: Pointer to connection
+ * @priv: pointer to domain private object
+ * @secinfo: Pointer to secret info
+ * @srcalias: Alias of the disk/hostdev used to generate the secret alias
+ * @protocol: Protocol for secret
+ * @authdef: Pointer to auth data
+ *
+ * Taking a secinfo, fill in the initialization vector information
+ *
+ * Returns 0 on success, -1 on failure with error message
+ */
+static int ATTRIBUTE_UNUSED
+qemuDomainSecretIVSetup(virConnectPtr conn,
+ qemuDomainObjPrivatePtr priv,
+ qemuDomainSecretInfoPtr secinfo,
+ const char *srcalias,
+ virStorageNetProtocol protocol,
+ virStorageAuthDefPtr authdef)
+{
+ int ret = -1;
+ int rc;
+ uint8_t *raw_iv = NULL;
+ char *secret = NULL;
+ uint8_t *ciphertext = NULL;
+ size_t secretlen = 0, ciphertextlen = 0, paddinglen;
Please one variable per line.
+ int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
Rather than changing to RBD below I think the decision should be done in
a single swithch.
+ const char *protocolstr =
virStorageNetProtocolTypeToString(protocol);
+ gnutls_cipher_hd_t handle = NULL;
+ gnutls_datum_t enc_key;
+ gnutls_datum_t iv_key;
This function is compiled even without gnutls.
+
+ secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV;
+ if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0)
+ return -1;
+
+ if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
+ secretType = VIR_SECRET_USAGE_TYPE_CEPH;
+
+ if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias)))
+ return -1;
+
+ /* Create a random initialization vector */
+ if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN)))
+ return -1;
+
+ /* Encode the IV and save that since qemu will need it */
So the initialization vector or IV is a specific of the used cipher. I
don't think we should use it in the names for the secret type.
Either the name of the cipher should be used or just "secret" if it's
unlikey that qemu would add new ciphers.
+ base64_encode_alloc((const char *)raw_iv,
QEMU_DOMAIN_IV_KEY_LEN,
+ &secinfo->s.iv.iv);
+ if (!secinfo->s.iv.iv) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to encode initialization vector"));
+ goto cleanup;
+ }
+
+ /* Grab the unencoded secret */
+ if (!(secret = virSecretGetSecretString(conn, protocolstr, false,
In qemuDomainSecretPlainSetup the secret is requested encoded for RBD,
thus it was possible to use non ascii keys ...
+ authdef, secretType)))
+ goto cleanup;
+
+ /* Allocate a padded buffer */
+ secretlen = strlen(secret);
... which won't work with this.
+ paddinglen = 16 - (secretlen % 16);
Magic constants?
+ ciphertextlen = secretlen + paddinglen;
VIR_ROUND_UP
+ if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
+ goto cleanup;
+ memcpy(ciphertext, secret, secretlen);
+ memset(ciphertext + secretlen, paddinglen, paddinglen);
You are padding it by the last byte of the padding size? Is this really
desired?
+
+ /* clear secret so that it doesn't hang around */
+ memset(secret, 0, strlen(secret));
You do it again in cleanup. Is it really necessary?
+
+ /* Initialize the gnutls cipher */
+ enc_key.size = QEMU_DOMAIN_MASTER_KEY_LEN;
+ enc_key.data = priv->masterKey;
+ iv_key.size = QEMU_DOMAIN_IV_KEY_LEN;
+ iv_key.data = raw_iv;
+ if ((rc = gnutls_cipher_init(&handle, GNUTLS_CIPHER_AES_256_CBC,
+ &enc_key, &iv_key)) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to initialize cipher: '%s'"),
+ gnutls_strerror(rc));
+ goto cleanup;
+ }
+
+ /* Encrypt the secret and free the memory for cipher operations */
+ rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen);
+ gnutls_cipher_deinit(handle);
+ if (rc < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to encrypt the secret: '%s'"),
+ gnutls_strerror(rc));
+ goto cleanup;
+ }
+
+ /* Now base64 encode the ciphertext and store to be passed to qemu */
+ base64_encode_alloc((const char *)ciphertext, ciphertextlen,
+ &secinfo->s.iv.ciphertext);
+ if (!secinfo->s.iv.ciphertext) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to encode ciphertext"));
+ goto cleanup;
+ }
+
+ ret = 0;
+
+ cleanup:
+
+ /* Clear and free the raw_iv, plaintext secret, and ciphertext */
+ memset(raw_iv, 0, QEMU_DOMAIN_IV_KEY_LEN);
+ VIR_FREE(raw_iv);
+
+ memset(secret, 0, secretlen);
+ VIR_FREE(secret);
+
+ memset(ciphertext, 0, ciphertextlen);
+ VIR_FREE(ciphertext);
+
+ return ret;
Peter
7:51 a.m.
New subject: [libvirt] [PATCH v3 1/2] qemu: Introduce new Secret IV API's
On Wed, May 11, 2016 at 02:47:21PM +0200, Peter Krempa wrote:
On Thu, May 05, 2016 at 15:00:40 -0400, John Ferlan wrote:
> New APIs:
>
> qemuDomainGetIVKeyAlias:
> Generate/return the secret object alias for an initialization
> vector (IV) secret info type. This will be saved in the secret
> info block. This will be called from qemuDomainSecretIVSetup.
>
> qemuDomainSecretHaveEncrypt: (private)
> Boolean function to determine whether the underly encryption
> API is available. This function will utilize a similar mechanism
> as the 'gnutls_rnd' did in configure.ac.
>
> qemuDomainSecretIVSetup: (private)
> This API handles the details of the generation of the IV secret
> and saves the pieces that need to be passed to qemu in order for
> the secret to be decrypted. The encrypted secret based upon the
> domain master key, an initialization vector (16 byte random value),
> and the stored secret. Finally, the requirement from qemu is the IV
> and encrypted secret are to be base64 encoded. They can be passed
> either directly or within a file. This implementation chooses
> to pass directly rather than a file.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> configure.ac | 1 +
> src/qemu/qemu_alias.c | 23 +++++++++
> src/qemu/qemu_alias.h | 2 +
> src/qemu/qemu_domain.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 163 insertions(+)
>
> +static int ATTRIBUTE_UNUSED
> +qemuDomainSecretIVSetup(virConnectPtr conn,
> + qemuDomainObjPrivatePtr priv,
> + qemuDomainSecretInfoPtr secinfo,
> + const char *srcalias,
> + virStorageNetProtocol protocol,
> + virStorageAuthDefPtr authdef)
> +{
> + int ret = -1;
> + int rc;
> + uint8_t *raw_iv = NULL;
> + char *secret = NULL;
> + uint8_t *ciphertext = NULL;
> + size_t secretlen = 0, ciphertextlen = 0, paddinglen;
Please one variable per line.
> + int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
Rather than changing to RBD below I think the decision should be done in
a single swithch.
> + const char *protocolstr = virStorageNetProtocolTypeToString(protocol);
> + gnutls_cipher_hd_t handle = NULL;
> + gnutls_datum_t enc_key;
> + gnutls_datum_t iv_key;
This function is compiled even without gnutls.
> +
> + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV;
> + if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0)
> + return -1;
> +
> + if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
> + secretType = VIR_SECRET_USAGE_TYPE_CEPH;
> +
> + if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias)))
> + return -1;
> +
> + /* Create a random initialization vector */
> + if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN)))
> + return -1;
> +
> + /* Encode the IV and save that since qemu will need it */
So the initialization vector or IV is a specific of the used cipher. I
don't think we should use it in the names for the secret type.
Either the name of the cipher should be used or just "secret" if it's
unlikey that qemu would add new ciphers.
> + base64_encode_alloc((const char *)raw_iv, QEMU_DOMAIN_IV_KEY_LEN,
> + &secinfo->s.iv.iv);
> + if (!secinfo->s.iv.iv) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to encode initialization vector"));
> + goto cleanup;
> + }
> +
> + /* Grab the unencoded secret */
> + if (!(secret = virSecretGetSecretString(conn, protocolstr, false,
In qemuDomainSecretPlainSetup the secret is requested encoded for RBD,
thus it was possible to use non ascii keys ...
> + authdef, secretType)))
> + goto cleanup;
> +
> + /* Allocate a padded buffer */
> + secretlen = strlen(secret);
... which won't work with this.
Yep, since we're treating the secret as raw bytes, we should really
use 'uint8_t *' for 'secret' instead of 'char*' to make it
obvious
you can't strlen() it. This means virSecretGetSecretString should
probably return uint8_t too, and have an size_t return for the length
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:09 p.m.
New subject: [libvirt] [PATCH v3 1/2] qemu: Introduce new Secret IV API's
On 05/11/2016 08:47 AM, Peter Krempa wrote:
On Thu, May 05, 2016 at 15:00:40 -0400, John Ferlan wrote:
> New APIs:
>
> qemuDomainGetIVKeyAlias:
> Generate/return the secret object alias for an initialization
> vector (IV) secret info type. This will be saved in the secret
> info block. This will be called from qemuDomainSecretIVSetup.
>
> qemuDomainSecretHaveEncrypt: (private)
> Boolean function to determine whether the underly encryption
> API is available. This function will utilize a similar mechanism
> as the 'gnutls_rnd' did in configure.ac.
>
> qemuDomainSecretIVSetup: (private)
> This API handles the details of the generation of the IV secret
> and saves the pieces that need to be passed to qemu in order for
> the secret to be decrypted. The encrypted secret based upon the
> domain master key, an initialization vector (16 byte random value),
> and the stored secret. Finally, the requirement from qemu is the IV
> and encrypted secret are to be base64 encoded. They can be passed
> either directly or within a file. This implementation chooses
> to pass directly rather than a file.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> configure.ac | 1 +
> src/qemu/qemu_alias.c | 23 +++++++++
> src/qemu/qemu_alias.h | 2 +
> src/qemu/qemu_domain.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 163 insertions(+)
>
> diff --git a/configure.ac b/configure.ac
> index 88e2e20..3cabd5e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1264,6 +1264,7 @@ if test "x$with_gnutls" != "xno"; then
> ]])
>
> AC_CHECK_FUNCS([gnutls_rnd])
> + AC_CHECK_FUNCS([gnutls_cipher_encrypt])
>
> CFLAGS="$old_CFLAGS"
> LIBS="$old_LIBS"
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index ade2033..de9a74f 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -556,3 +556,26 @@ qemuDomainGetMasterKeyAlias(void)
>
> return alias;
> }
> +
> +
> +/* qemuDomainGetIVKeyAlias:
> + *
> + * Generate and return an initialization vector alias
> + *
> + * Returns NULL or a string containing the IV key alias
> + */
> +char *
> +qemuDomainGetIVKeyAlias(const char *srcalias)
> +{
> + char *alias;
> +
> + if (!srcalias) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("secret iv alias requires valid source alias"));
> + return NULL;
> + }
> +
> + ignore_value(virAsprintf(&alias, "%s-ivKey0", srcalias));
> +
> + return alias;
> +}
> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
> index 2d7bc9b..435747e 100644
> --- a/src/qemu/qemu_alias.h
> +++ b/src/qemu/qemu_alias.h
> @@ -69,4 +69,6 @@ char *qemuAliasFromDisk(const virDomainDiskDef *disk);
>
> char *qemuDomainGetMasterKeyAlias(void);
>
> +char *qemuDomainGetIVKeyAlias(const char *srcalias);
> +
> #endif /* __QEMU_ALIAS_H__*/
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cae356c..b174caa 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -893,6 +893,143 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
> }
>
>
> +/* qemuDomainSecretHaveEncypt:
> + *
> + * Returns true if we can support the encryption code, false otherwise
> + */
> +static bool ATTRIBUTE_UNUSED
> +qemuDomainSecretHaveEncrypt(void)
> +{
> +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
> + return true;
> +#else
> + return false;
> +#endif
> +}
> +
> +
> +/* qemuDomainSecretIVSetup:
> + * @conn: Pointer to connection
> + * @priv: pointer to domain private object
> + * @secinfo: Pointer to secret info
> + * @srcalias: Alias of the disk/hostdev used to generate the secret alias
> + * @protocol: Protocol for secret
> + * @authdef: Pointer to auth data
> + *
> + * Taking a secinfo, fill in the initialization vector information
> + *
> + * Returns 0 on success, -1 on failure with error message
> + */
> +static int ATTRIBUTE_UNUSED
> +qemuDomainSecretIVSetup(virConnectPtr conn,
> + qemuDomainObjPrivatePtr priv,
> + qemuDomainSecretInfoPtr secinfo,
> + const char *srcalias,
> + virStorageNetProtocol protocol,
> + virStorageAuthDefPtr authdef)
> +{
> + int ret = -1;
> + int rc;
> + uint8_t *raw_iv = NULL;
> + char *secret = NULL;
> + uint8_t *ciphertext = NULL;
> + size_t secretlen = 0, ciphertextlen = 0, paddinglen;
Please one variable per line.
OK
> + int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
Rather than changing to RBD below I think the decision should be done in
a single swithch.
OK - a switch it is.
> + const char *protocolstr =
virStorageNetProtocolTypeToString(protocol);
> + gnutls_cipher_hd_t handle = NULL;
> + gnutls_datum_t enc_key;
> + gnutls_datum_t iv_key;
This function is compiled even without gnutls.
True, but it's not called unless qemuDomainSecretHaveEncrypt returns
true, which of course is the "next" patch only because I was trying to
keep the changes to a minimum. I left it this way since "theoretically
speaking" something could be added.
Shall I assume you'd prefer a function for each? That means a function
with a bunch of ATTRIBUTE_UNUSED for each argument, a virReportError,
and a -1 return.
> +
> + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV;
> + if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0)
> + return -1;
> +
> + if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
> + secretType = VIR_SECRET_USAGE_TYPE_CEPH;
> +
> + if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias)))
> + return -1;
> +
> + /* Create a random initialization vector */
> + if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN)))
> + return -1;
> +
> + /* Encode the IV and save that since qemu will need it */
So the initialization vector or IV is a specific of the used cipher. I
don't think we should use it in the names for the secret type.
Either the name of the cipher should be used or just "secret" if it's
unlikey that qemu would add new ciphers.
The IV is just a 16 byte random key and yes, currently generated by
either the gnutls_rnd if available; otherwise, by virRandomBytes if not.
I'm not sure it matters "how" it was generated since we pass it in on
the command line in an argument "iv=" for the secret. of course for
this implementation, we're pretty much guaranteed that gnutls_rnd
generated the iv.
Which "iv" name do you want changed? The union or one in the struct? I
think you want the union name changed, but I'm not 100% sure - so better
to ask. Based on your comment that means you'd rather see soemthing like
"_qemuDomainSecretAES_256_CBC" instead of "_qemuDomainSecretIV"? And
then use "secret" instead of "iv" in the union?
> + base64_encode_alloc((const char *)raw_iv,
QEMU_DOMAIN_IV_KEY_LEN,
> + &secinfo->s.iv.iv);
> + if (!secinfo->s.iv.iv) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to encode initialization vector"));
> + goto cleanup;
> + }
> +
> + /* Grab the unencoded secret */
> + if (!(secret = virSecretGetSecretString(conn, protocolstr, false,
In qemuDomainSecretPlainSetup the secret is requested encoded for RBD,
thus it was possible to use non ascii keys ...
Ug... Looks like virSecretGetSecretString needs another parameter to
return the secretlen and as Dan points out the uint8_t * instead of char
*. That'll add more patches to the pile...
> + authdef,
secretType)))
> + goto cleanup;
> +
> + /* Allocate a padded buffer */
> + secretlen = strlen(secret);
... which won't work with this.
> + paddinglen = 16 - (secretlen % 16);
Magic constants?
Black magic constants!
> + ciphertextlen = secretlen + paddinglen;
VIR_ROUND_UP
Ok sure...
> + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
> + goto cleanup;
> + memcpy(ciphertext, secret, secretlen);
> + memset(ciphertext + secretlen, paddinglen, paddinglen);
You are padding it by the last byte of the padding size? Is this really
desired?
What I read on this for this encryption algorithm is that the size needs
to be 16 byte aligned and that the "filler" bytes need to be the size of
the padding. Look at the qemu source for qcrypto_secret_decrypt.
> +
> + /* clear secret so that it doesn't hang around */
> + memset(secret, 0, strlen(secret));
You do it again in cleanup. Is it really necessary?
I can remove this one.
John
> +
> + /* Initialize the gnutls cipher */
> + enc_key.size = QEMU_DOMAIN_MASTER_KEY_LEN;
> + enc_key.data = priv->masterKey;
> + iv_key.size = QEMU_DOMAIN_IV_KEY_LEN;
> + iv_key.data = raw_iv;
> + if ((rc = gnutls_cipher_init(&handle, GNUTLS_CIPHER_AES_256_CBC,
> + &enc_key, &iv_key)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to initialize cipher: '%s'"),
> + gnutls_strerror(rc));
> + goto cleanup;
> + }
> +
> + /* Encrypt the secret and free the memory for cipher operations */
> + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen);
> + gnutls_cipher_deinit(handle);
> + if (rc < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to encrypt the secret: '%s'"),
> + gnutls_strerror(rc));
> + goto cleanup;
> + }
> +
> + /* Now base64 encode the ciphertext and store to be passed to qemu */
> + base64_encode_alloc((const char *)ciphertext, ciphertextlen,
> + &secinfo->s.iv.ciphertext);
> + if (!secinfo->s.iv.ciphertext) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to encode ciphertext"));
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> + cleanup:
> +
> + /* Clear and free the raw_iv, plaintext secret, and ciphertext */
> + memset(raw_iv, 0, QEMU_DOMAIN_IV_KEY_LEN);
> + VIR_FREE(raw_iv);
> +
> + memset(secret, 0, secretlen);
> + VIR_FREE(secret);
> +
> + memset(ciphertext, 0, ciphertextlen);
> + VIR_FREE(ciphertext);
> +
> + return ret;
Peter
3:07 p.m.
New subject: [libvirt] [PATCH v3 1/2] qemu: Introduce new Secret IV API's
On Wed, May 11, 2016 at 13:09:27 -0400, John Ferlan wrote:
On 05/11/2016 08:47 AM, Peter Krempa wrote:
> On Thu, May 05, 2016 at 15:00:40 -0400, John Ferlan wrote:
>> New APIs:
[...]
>> diff --git a/src/qemu/qemu_domain.c
b/src/qemu/qemu_domain.c
>> index cae356c..b174caa 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -893,6 +893,143 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
[...]
>> + gnutls_datum_t enc_key;
>> + gnutls_datum_t iv_key;
>
> This function is compiled even without gnutls.
>
True, but it's not called unless qemuDomainSecretHaveEncrypt returns
true, which of course is the "next" patch only because I was trying to
keep the changes to a minimum. I left it this way since "theoretically
speaking" something could be added.
No. The point is that it will fail to compile when configured without
gnutls.
Shall I assume you'd prefer a function for each? That means a function
with a bunch of ATTRIBUTE_UNUSED for each argument, a virReportError,
and a -1 return.
>> +
>> + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV;
>> + if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0)
>> + return -1;
>> +
>> + if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
>> + secretType = VIR_SECRET_USAGE_TYPE_CEPH;
>> +
>> + if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias)))
>> + return -1;
>> +
>> + /* Create a random initialization vector */
>> + if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN)))
>> + return -1;
>> +
>> + /* Encode the IV and save that since qemu will need it */
>
> So the initialization vector or IV is a specific of the used cipher. I
> don't think we should use it in the names for the secret type.
>
> Either the name of the cipher should be used or just "secret" if it's
> unlikey that qemu would add new ciphers.
>
The IV is just a 16 byte random key and yes, currently generated by
either the gnutls_rnd if available; otherwise, by virRandomBytes if not.
The IV is used by the CBC cipher mode to do the first block of
encryption since you don't have anything besides the master key and the
plaintext to do the encryption. The CBC mode uses output of the cipher
function as a IV for the next step (thus Cipher Block Chaining).
The IV is necessary so that the same plaintext does not encrypt to the
same ciphertext. This is also probably the reason why qemu chose to use
the CBC mode of AES instead of the ECB mode which is not suitable for
encrypting more than 1 block.
...
I'm not sure it matters "how" it was generated since we
pass it in on
the command line in an argument "iv=" for the secret. of course for
this implementation, we're pretty much guaranteed that gnutls_rnd
generated the iv.
Since the IV is just an ingredient for the AES cipher itself I think
it's really not a good name for describing the internal scructs and the
corresponding enums.
Which "iv" name do you want changed? The union or one in
the struct? I
think you want the union name changed, but I'm not 100% sure - so better
to ask. Based on your comment that means you'd rather see soemthing like
"_qemuDomainSecretAES_256_CBC" instead of "_qemuDomainSecretIV"? And
More like qemuDomainSecretAES. Of course the name of the variable in the
struct is correct. The union name and the corresponding enum value name
are odd. And perhaps VIR_DOMAIN_SECRET_INFO_TYPE_AES for the enum name.
then use "secret" instead of "iv" in the union?
Either secret or aes or something like that.
[...]
>> + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
>> + goto cleanup;
>> + memcpy(ciphertext, secret, secretlen);
>> + memset(ciphertext + secretlen, paddinglen, paddinglen);
>
> You are padding it by the last byte of the padding size? Is this really
> desired?
>
What I read on this for this encryption algorithm is that the size needs
to be 16 byte aligned and that the "filler" bytes need to be the size of
the padding. Look at the qemu source for qcrypto_secret_decrypt.
Whoah. Even more black magic. So then it's desired. In that case this
line most of all of the rest desires a comment.
Peter
2 p.m.
New subject: [libvirt] [PATCH v3 2/2] qemu: Utilize qemu secret objects for RBD auth/secret
Partial fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=1182074
If they're available and we need to pass secrets to qemu, then use the
qemu domain secret object in order to pass the secrets for RBD volumes
instead of passing the base64 encoded secret on the command line.
The goal is to make IV secrets the default and have no user interaction
required in order to allow using the IV mechanism. If the mechanism
is not available, then fall back to the current plain mechanism using
a base64 encoded secret.
New API's:
qemu_command.c:
qemuBuildSecretInfoProps: (private)
Generate/return a JSON properties object for the IV secret to
be used by both the command building and eventually the hotplug
code in order to add the secret object. Code was designed so that
in the future perhaps hotplug could use it if it made sense.
qemuBuildSecretIVCommandLine (private)
Generate and add to the command line the -object secret for the
IV secret. This will be required for the subsequent RBD reference
to the object.
qemuBuildDiskSecinfoCommandLine (private)
Handle adding the IV secret object.
qemu_domain.c
qemuDomainSecretSetup: (private)
Shim to call either the IV or Plain Setup functions based upon
whether IV secrets are possible (we have the encryption API) or not,
we have secrets, and of course if the protocol source is RBD.
Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and
qemuDomainSecretHostdevPrepare to add the secret rather than assuming
plain. In the future when iSCSI secrets can use this mechanism for
either a disk or hostdev there will be less change.
Command Building:
Adjust the qemuBuildRBDSecinfoURI API's in order to generate the
specific command options for an IV secret, such as:
-object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted,
format=base64
-drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
mon_host=mon1.example.org\:6321,password-secret=$alias,...
where the 'id=' value is the secret object alias generated by
concatenating the disk alias and "-ivKey0". The 'keyid= $masterKey'
is the master key shared with qemu, and the -drive syntax will
reference that alias as the 'password-secret'. For the -drive
syntax, the 'id=myname' is kept to define the username, while the
'key=$base64 encoded secret' is removed.
While according to the syntax described for qemu commit '60390a21'
or as seen in the email archive:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html
it is possible to pass a plaintext password via a file, the qemu
commit 'ac1d8878' describes the more feature rich 'keyid=' option
based upon the shared masterKey.
Tests:
Add mock's for virRandomBytes and gnutls_rnd in order to return a
constant stream of '0xff' in the bytes for a non random key in order
to generate "constant" values for the secrets so that the tests can
use those results to compare results.
Hotplug:
Since the hotplug code doesn't add command line arguments, passing
the encoded secret directly to the monitor will suffice.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_command.c | 130 ++++++++++++++++++++-
src/qemu/qemu_domain.c | 56 +++++++--
...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 +++++
...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 +++++++
tests/qemuxml2argvmock.c | 31 ++++-
tests/qemuxml2argvtest.c | 11 ++
6 files changed, 290 insertions(+), 11 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 93aaf2a..9b0bd7a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -607,6 +607,119 @@ qemuNetworkDriveGetPort(int protocol,
}
+/**
+ * qemuBuildSecretInfoProps:
+ * @secinfo: pointer to the secret info object
+ * @type: returns a pointer to a character string for object name
+ * @props: json properties to return
+ *
+ * Build the JSON properties for the secret info type.
+ *
+ * Returns 0 on success with the filled in JSON property; otherwise,
+ * returns -1 on failure error message set.
+ */
+static int
+qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
+ const char **type,
+ virJSONValuePtr *propsret)
+{
+ int ret = -1;
+ char *keyid = NULL;
+ virJSONValuePtr props = NULL;
+
+ *type = "secret";
+
+ if (!(keyid = qemuDomainGetMasterKeyAlias()))
+ return -1;
+
+ if (!(props = virJSONValueNewObject()))
+ goto cleanup;
+
+ if (virJSONValueObjectAdd(props,
+ "s:data", secinfo->s.iv.ciphertext,
+ "s:keyid", keyid,
+ "s:iv", secinfo->s.iv.iv,
+ "s:format", "base64", NULL) < 0)
+ goto cleanup;
+
+ *propsret = props;
+ props = NULL;
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(keyid);
+ virJSONValueFree(props);
+
+ return ret;
+}
+
+
+/**
+ * qemuBuildSecretIVCommandLine:
+ * @cmd: the command to modify
+ * @secinfo: pointer to the secret info object
+ *
+ * If the secinfo is available and associated with an IV secret,
+ * then format the command line for the secret object. This object
+ * will be referenced by the device that needs/uses it, so it needs
+ * to be in place first.
+ *
+ * Returns 0 on success, -1 w/ error message on failure
+ */
+static int
+qemuBuildSecretIVCommandLine(virCommandPtr cmd,
+ qemuDomainSecretInfoPtr secinfo)
+{
+ int ret = -1;
+ virJSONValuePtr props = NULL;
+ const char *type;
+ char *tmp = NULL;
+
+ if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0)
+ return -1;
+
+ if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.iv.alias,
+ props)))
+ goto cleanup;
+
+ virCommandAddArgList(cmd, "-object", tmp, NULL);
+ ret = 0;
+
+ cleanup:
+ virJSONValueFree(props);
+ VIR_FREE(tmp);
+
+ return ret;
+}
+
+
+/* qemuBuildDiskSecinfoCommandLine:
+ * @cmd: Pointer to the command string
+ * @secinfo: Pointer to a possible secinfo
+ *
+ * Adding an IV secret to the command line for an iSCSI disk currently
+ * does not work. As of qemu 2.6, in order to add the options "user=" and
+ * "password-secret=", one would have to do so using an "-iscsi"
command
+ * line option. However, that requires supplying an "id=" that can be used
+ * by some "-drive" command in order to "find" the correct IQN.
Unfortunately,
+ * an IQN consists of characters ':' and '/' that cannot be used for an
"id=".
+ *
+ * Returns 0 on success, -1 w/ error on some sort of failure.
+ */
+static int
+qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd,
+ qemuDomainSecretInfoPtr secinfo)
+{
+ /* Not necessary for non IV secrets */
+ if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_IV)
+ return 0;
+
+ /* For now we'll just build the IV. In the future more may be required
+ * in order to support iSCSI */
+ return qemuBuildSecretIVCommandLine(cmd, secinfo);
+}
+
+
/* qemuBuildGeneralSecinfoURI:
* @uri: Pointer to the URI structure to add to
* @secinfo: Pointer to the secret info data (if present)
@@ -677,6 +790,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf,
break;
case VIR_DOMAIN_SECRET_INFO_TYPE_IV:
+ virBufferEscape(buf, '\\', ":",
":id=%s:auth_supported=cephx\\;none",
+ secinfo->s.iv.username);
+ break;
+
case VIR_DOMAIN_SECRET_INFO_TYPE_LAST:
return -1;
}
@@ -1083,6 +1200,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
char *source = NULL;
int actualType = virStorageSourceGetActualType(disk->src);
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+ qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
if (idx < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1164,7 +1282,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
break;
}
- if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) <
0)
+ if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0)
goto error;
if (source &&
@@ -1215,6 +1333,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
virBufferEscape(&opt, ',', ",", "%s,", source);
+ if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
+ secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_IV) {
+ virBufferAsprintf(&opt, "password-secret=%s,",
secinfo->s.iv.alias);
+ }
+
if (disk->src->format > 0 &&
disk->src->type != VIR_STORAGE_TYPE_DIR)
virBufferAsprintf(&opt, "format=%s,",
@@ -1905,6 +2028,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
virDomainDiskDefPtr disk = def->disks[i];
bool withDeviceArg = false;
bool deviceFlagMasked = false;
+ qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+ qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
/* Unless we have -device, then USB disks need special
handling */
@@ -1947,6 +2072,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
break;
}
+ if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)
+ return -1;
+
virCommandAddArg(cmd, "-drive");
/* Unfortunately it is not possible to use
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b174caa..3e627a5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -897,7 +897,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
*
* Returns true if we can support the encryption code, false otherwise
*/
-static bool ATTRIBUTE_UNUSED
+static bool
qemuDomainSecretHaveEncrypt(void)
{
#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
@@ -920,7 +920,7 @@ qemuDomainSecretHaveEncrypt(void)
*
* Returns 0 on success, -1 on failure with error message
*/
-static int ATTRIBUTE_UNUSED
+static int
qemuDomainSecretIVSetup(virConnectPtr conn,
qemuDomainObjPrivatePtr priv,
qemuDomainSecretInfoPtr secinfo,
@@ -1030,6 +1030,44 @@ qemuDomainSecretIVSetup(virConnectPtr conn,
}
+/* qemuDomainSecretSetup:
+ * @conn: Pointer to connection
+ * @priv: pointer to domain private object
+ * @secinfo: Pointer to secret info
+ * @srcalias: Alias of the disk/hostdev used to generate the secret alias
+ * @protocol: Protocol for secret
+ * @authdef: Pointer to auth data
+ *
+ * If we have the encryption API present and can support a secret object, then
+ * build the IV secret; otherwise, build the Plain secret. This is the magic
+ * decision point for utilizing the IV secrets for an RBD disk. For now iSCSI
+ * disks and hostdevs will not be able to utilize this mechanism. For details,
+ * see qemuBuildDiskSecinfoCommandLine in qemu_command.c
+ *
+ * Returns 0 on success, -1 on failure
+ */
+static int
+qemuDomainSecretSetup(virConnectPtr conn,
+ qemuDomainObjPrivatePtr priv,
+ qemuDomainSecretInfoPtr secinfo,
+ const char *srcalias,
+ virStorageNetProtocol protocol,
+ virStorageAuthDefPtr authdef)
+{
+ if (qemuDomainSecretHaveEncrypt() &&
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) &&
+ protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
+ if (qemuDomainSecretIVSetup(conn, priv, secinfo,
+ srcalias, protocol, authdef) < 0)
+ return -1;
+ } else {
+ if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0)
+ return -1;
+ }
+ return 0;
+}
+
+
/* qemuDomainSecretDiskDestroy:
* @disk: Pointer to a disk definition
*
@@ -1058,7 +1096,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
*/
int
qemuDomainSecretDiskPrepare(virConnectPtr conn,
- qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
+ qemuDomainObjPrivatePtr priv,
virDomainDiskDefPtr disk)
{
virStorageSourcePtr src = disk->src;
@@ -1075,8 +1113,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
if (VIR_ALLOC(secinfo) < 0)
return -1;
- if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
- src->auth) < 0)
+ if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias,
+ src->protocol, src->auth) < 0)
goto error;
diskPriv->secinfo = secinfo;
@@ -1119,7 +1157,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev)
*/
int
qemuDomainSecretHostdevPrepare(virConnectPtr conn,
- qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
+ qemuDomainObjPrivatePtr priv,
virDomainHostdevDefPtr hostdev)
{
virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys;
@@ -1140,9 +1178,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn,
if (VIR_ALLOC(secinfo) < 0)
return -1;
- if (qemuDomainSecretPlainSetup(conn, secinfo,
- VIR_STORAGE_NET_PROTOCOL_ISCSI,
- iscsisrc->auth) < 0)
+ if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias,
+ VIR_STORAGE_NET_PROTOCOL_ISCSI,
+ iscsisrc->auth) < 0)
goto error;
hostdevPriv->secinfo = secinfo;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
new file mode 100644
index 0000000..f6c0906
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-object secret,id=masterKey0,format=raw,\
+file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
+-M pc \
+-m 214 \
+-smp 1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-object secret,id=virtio-disk0-ivKey0,\
+data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\
+keyid=masterKey0,iv=/////////////////////w==,format=base64 \
+-drive 'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
+mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
+password-secret=virtio-disk0-ivKey0,format=raw,if=none,id=drive-virtio-disk0' \
+-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
+id=virtio-disk0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml
new file mode 100644
index 0000000..ac2e942
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml
@@ -0,0 +1,42 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <disk type='network' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <auth username='myname'>
+ <secret type='ceph' usage='mycluster_myname'/>
+ </auth>
+ <source protocol='rbd' name='pool/image'>
+ <host name='mon1.example.org' port='6321'/>
+ <host name='mon2.example.org' port='6322'/>
+ <host name='mon3.example.org' port='6322'/>
+ </source>
+ <target dev='vda' bus='virtio'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 1616eed..2bfbf6b 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014 Red Hat, Inc.
+ * Copyright (C) 2014-2016 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
@@ -30,6 +30,13 @@
#include "virstring.h"
#include "virtpm.h"
#include "virutil.h"
+#include "virrandom.h"
+#ifdef WITH_GNUTLS
+# include <gnutls/gnutls.h>
+# if HAVE_GNUTLS_CRYPTO_H
+# include <gnutls/crypto.h>
+# endif
+#endif
#include <time.h>
#include <unistd.h>
@@ -145,3 +152,25 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
{
/* nada */
}
+
+int
+virRandomBytes(unsigned char *buf,
+ size_t buflen)
+{
+ size_t i;
+
+ for (i = 0; i < buflen; i++)
+ buf[i] = 0xff;
+
+ return 0;
+}
+
+#ifdef WITH_GNUTLS
+int
+gnutls_rnd(gnutls_rnd_level_t level ATTRIBUTE_UNUSED,
+ void *data,
+ size_t len)
+{
+ return virRandomBytes(data, len);
+#endif
+}
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e41444d..1f2577a 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -26,6 +26,7 @@
# include "virstring.h"
# include "storage/storage_driver.h"
# include "virmock.h"
+# include "virrandom.h"
# include "testutilsqemu.h"
@@ -330,6 +331,14 @@ static int testCompareXMLToArgvFiles(const char *xml,
}
}
+ /* Create a fake master key */
+ if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
+ goto out;
+
+ if (virRandomBytes(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
+ goto out;
+ priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
+
if (!(cmd = qemuProcessCreatePretendCmd(conn, &driver, vm, migrateURI,
(flags & FLAG_FIPS), false,
VIR_QEMU_PROCESS_START_COLD))) {
@@ -776,6 +785,8 @@ mymain(void)
DO_TEST("disk-drive-network-rbd", NONE);
DO_TEST("disk-drive-network-sheepdog", NONE);
DO_TEST("disk-drive-network-rbd-auth", NONE);
+ DO_TEST("disk-drive-network-rbd-auth-IV",
+ QEMU_CAPS_OBJECT_SECRET);
DO_TEST("disk-drive-network-rbd-ipv6", NONE);
DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE);
DO_TEST("disk-drive-no-boot",
--
2.5.5
7:21 a.m.
New subject: [libvirt] [PATCH v3 2/2] qemu: Utilize qemu secret objects for RBD auth/secret
On Thu, May 05, 2016 at 15:00:41 -0400, John Ferlan wrote:
Partial fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=1182074
If they're available and we need to pass secrets to qemu, then use the
qemu domain secret object in order to pass the secrets for RBD volumes
instead of passing the base64 encoded secret on the command line.
The goal is to make IV secrets the default and have no user interaction
required in order to allow using the IV mechanism. If the mechanism
is not available, then fall back to the current plain mechanism using
a base64 encoded secret.
New API's:
qemu_command.c:
qemuBuildSecretInfoProps: (private)
Generate/return a JSON properties object for the IV secret to
be used by both the command building and eventually the hotplug
code in order to add the secret object. Code was designed so that
in the future perhaps hotplug could use it if it made sense.
qemuBuildSecretIVCommandLine (private)
Generate and add to the command line the -object secret for the
IV secret. This will be required for the subsequent RBD reference
to the object.
qemuBuildDiskSecinfoCommandLine (private)
Handle adding the IV secret object.
qemu_domain.c
qemuDomainSecretSetup: (private)
Shim to call either the IV or Plain Setup functions based upon
whether IV secrets are possible (we have the encryption API) or not,
we have secrets, and of course if the protocol source is RBD.
Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and
qemuDomainSecretHostdevPrepare to add the secret rather than assuming
plain. In the future when iSCSI secrets can use this mechanism for
either a disk or hostdev there will be less change.
Command Building:
Adjust the qemuBuildRBDSecinfoURI API's in order to generate the
specific command options for an IV secret, such as:
-object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted,
format=base64
-drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
mon_host=mon1.example.org\:6321,password-secret=$alias,...
where the 'id=' value is the secret object alias generated by
concatenating the disk alias and "-ivKey0". The 'keyid= $masterKey'
is the master key shared with qemu, and the -drive syntax will
reference that alias as the 'password-secret'. For the -drive
syntax, the 'id=myname' is kept to define the username, while the
'key=$base64 encoded secret' is removed.
While according to the syntax described for qemu commit '60390a21'
or as seen in the email archive:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html
it is possible to pass a plaintext password via a file, the qemu
commit 'ac1d8878' describes the more feature rich 'keyid=' option
based upon the shared masterKey.
Tests:
Add mock's for virRandomBytes and gnutls_rnd in order to return a
constant stream of '0xff' in the bytes for a non random key in order
to generate "constant" values for the secrets so that the tests can
use those results to compare results.
Hotplug:
Since the hotplug code doesn't add command line arguments, passing
the encoded secret directly to the monitor will suffice.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_command.c | 130 ++++++++++++++++++++-
src/qemu/qemu_domain.c | 56 +++++++--
...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 +++++
...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 +++++++
tests/qemuxml2argvmock.c | 31 ++++-
tests/qemuxml2argvtest.c | 11 ++
6 files changed, 290 insertions(+), 11 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml
I think this patch is doing too much in one place. You can definitely
split out the mocking code.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 93aaf2a..9b0bd7a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -607,6 +607,119 @@ qemuNetworkDriveGetPort(int protocol,
}
+/**
+ * qemuBuildSecretInfoProps:
+ * @secinfo: pointer to the secret info object
+ * @type: returns a pointer to a character string for object name
+ * @props: json properties to return
+ *
+ * Build the JSON properties for the secret info type.
+ *
+ * Returns 0 on success with the filled in JSON property; otherwise,
+ * returns -1 on failure error message set.
+ */
+static int
+qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
+ const char **type,
+ virJSONValuePtr *propsret)
+{
+ int ret = -1;
+ char *keyid = NULL;
+ virJSONValuePtr props = NULL;
+
+ *type = "secret";
+
+ if (!(keyid = qemuDomainGetMasterKeyAlias()))
+ return -1;
+
+ if (!(props = virJSONValueNewObject()))
+ goto cleanup;
+
+ if (virJSONValueObjectAdd(props,
+ "s:data", secinfo->s.iv.ciphertext,
+ "s:keyid", keyid,
+ "s:iv", secinfo->s.iv.iv,
+ "s:format", "base64", NULL) < 0)
+ goto cleanup;
You can use virJSONValueObjectCreate instead of the two calls above.
+
+ *propsret = props;
+ props = NULL;
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(keyid);
+ virJSONValueFree(props);
+
+ return ret;
+}
+
+
+/**
+ * qemuBuildSecretIVCommandLine:
+ * @cmd: the command to modify
+ * @secinfo: pointer to the secret info object
+ *
+ * If the secinfo is available and associated with an IV secret,
+ * then format the command line for the secret object. This object
+ * will be referenced by the device that needs/uses it, so it needs
+ * to be in place first.
+ *
+ * Returns 0 on success, -1 w/ error message on failure
+ */
+static int
+qemuBuildSecretIVCommandLine(virCommandPtr cmd,
Why does this contain "IV" you are creating a secret object.
+ qemuDomainSecretInfoPtr secinfo)
+{
+ int ret = -1;
+ virJSONValuePtr props = NULL;
+ const char *type;
+ char *tmp = NULL;
+
+ if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0)
+ return -1;
+
+ if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.iv.alias,
+ props)))
+ goto cleanup;
+
+ virCommandAddArgList(cmd, "-object", tmp, NULL);
+ ret = 0;
+
+ cleanup:
+ virJSONValueFree(props);
+ VIR_FREE(tmp);
+
+ return ret;
+}
+
+
+/* qemuBuildDiskSecinfoCommandLine:
+ * @cmd: Pointer to the command string
+ * @secinfo: Pointer to a possible secinfo
+ *
+ * Adding an IV secret to the command line for an iSCSI disk currently
+ * does not work. As of qemu 2.6, in order to add the options "user=" and
+ * "password-secret=", one would have to do so using an "-iscsi"
command
+ * line option. However, that requires supplying an "id=" that can be used
+ * by some "-drive" command in order to "find" the correct IQN.
Unfortunately,
+ * an IQN consists of characters ':' and '/' that cannot be used for an
"id=".
I don't think this kind of information belongs into a function comment.
It also doesn't really describe what is going on.
+ *
+ * Returns 0 on success, -1 w/ error on some sort of failure.
+ */
+static int
+qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd,
+ qemuDomainSecretInfoPtr secinfo)
+{
+ /* Not necessary for non IV secrets */
+ if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_IV)
Again, what is IV supposed to mean? The declaration of the enum is not
commented.
+ return 0;
+
+ /* For now we'll just build the IV. In the future more may be required
+ * in order to support iSCSI */
This comment is weird. This patch is dealing with RBD.
+ return qemuBuildSecretIVCommandLine(cmd, secinfo);
+}
+
+
/* qemuBuildGeneralSecinfoURI:
* @uri: Pointer to the URI structure to add to
* @secinfo: Pointer to the secret info data (if present)
@@ -677,6 +790,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf,
break;
case VIR_DOMAIN_SECRET_INFO_TYPE_IV:
+ virBufferEscape(buf, '\\', ":",
":id=%s:auth_supported=cephx\\;none",
+ secinfo->s.iv.username);
+ break;
+
case VIR_DOMAIN_SECRET_INFO_TYPE_LAST:
return -1;
}
@@ -1083,6 +1200,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
char *source = NULL;
int actualType = virStorageSourceGetActualType(disk->src);
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+ qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
if (idx < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1164,7 +1282,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
break;
}
- if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) <
0)
+ if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0)
goto error;
if (source &&
@@ -1215,6 +1333,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
virBufferEscape(&opt, ',', ",", "%s,", source);
+ if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
+ secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_IV) {
+ virBufferAsprintf(&opt, "password-secret=%s,",
secinfo->s.iv.alias);
This could be added to the switch above the place where you are doing
this.
+ }
+
if (disk->src->format > 0 &&
disk->src->type != VIR_STORAGE_TYPE_DIR)
virBufferAsprintf(&opt, "format=%s,",
@@ -1905,6 +2028,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
virDomainDiskDefPtr disk = def->disks[i];
bool withDeviceArg = false;
bool deviceFlagMasked = false;
+ qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
+ qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
/* Unless we have -device, then USB disks need special
handling */
@@ -1947,6 +2072,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
break;
}
+ if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)
qemuBuildDiskSecretCommanLine?
+ return -1;
+
virCommandAddArg(cmd, "-drive");
/* Unfortunately it is not possible to use
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b174caa..3e627a5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -897,7 +897,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
*
* Returns true if we can support the encryption code, false otherwise
*/
-static bool ATTRIBUTE_UNUSED
+static bool
qemuDomainSecretHaveEncrypt(void)
{
#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
@@ -920,7 +920,7 @@ qemuDomainSecretHaveEncrypt(void)
*
* Returns 0 on success, -1 on failure with error message
*/
-static int ATTRIBUTE_UNUSED
+static int
qemuDomainSecretIVSetup(virConnectPtr conn,
qemuDomainObjPrivatePtr priv,
qemuDomainSecretInfoPtr secinfo,
@@ -1030,6 +1030,44 @@ qemuDomainSecretIVSetup(virConnectPtr conn,
}
+/* qemuDomainSecretSetup:
+ * @conn: Pointer to connection
+ * @priv: pointer to domain private object
+ * @secinfo: Pointer to secret info
+ * @srcalias: Alias of the disk/hostdev used to generate the secret alias
+ * @protocol: Protocol for secret
+ * @authdef: Pointer to auth data
+ *
+ * If we have the encryption API present and can support a secret object, then
+ * build the IV secret; otherwise, build the Plain secret. This is the magic
+ * decision point for utilizing the IV secrets for an RBD disk. For now iSCSI
+ * disks and hostdevs will not be able to utilize this mechanism. For details,
+ * see qemuBuildDiskSecinfoCommandLine in qemu_command.c
+ *
+ * Returns 0 on success, -1 on failure
+ */
+static int
+qemuDomainSecretSetup(virConnectPtr conn,
+ qemuDomainObjPrivatePtr priv,
+ qemuDomainSecretInfoPtr secinfo,
+ const char *srcalias,
+ virStorageNetProtocol protocol,
+ virStorageAuthDefPtr authdef)
+{
+ if (qemuDomainSecretHaveEncrypt() &&
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) &&
+ protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
+ if (qemuDomainSecretIVSetup(conn, priv, secinfo,
+ srcalias, protocol, authdef) < 0)
+ return -1;
+ } else {
+ if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0)
+ return -1;
+ }
+ return 0;
Looks like this should be in the previous patch so that the function
doesn't need to be unused.
+}
+
+
/* qemuDomainSecretDiskDestroy:
* @disk: Pointer to a disk definition
*
@@ -1058,7 +1096,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
*/
int
qemuDomainSecretDiskPrepare(virConnectPtr conn,
- qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
+ qemuDomainObjPrivatePtr priv,
virDomainDiskDefPtr disk)
{
virStorageSourcePtr src = disk->src;
@@ -1075,8 +1113,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
if (VIR_ALLOC(secinfo) < 0)
return -1;
- if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
- src->auth) < 0)
+ if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias,
+ src->protocol, src->auth) < 0)
goto error;
diskPriv->secinfo = secinfo;
@@ -1119,7 +1157,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev)
*/
int
qemuDomainSecretHostdevPrepare(virConnectPtr conn,
- qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
+ qemuDomainObjPrivatePtr priv,
virDomainHostdevDefPtr hostdev)
{
virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys;
@@ -1140,9 +1178,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn,
if (VIR_ALLOC(secinfo) < 0)
return -1;
- if (qemuDomainSecretPlainSetup(conn, secinfo,
- VIR_STORAGE_NET_PROTOCOL_ISCSI,
- iscsisrc->auth) < 0)
+ if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias,
+ VIR_STORAGE_NET_PROTOCOL_ISCSI,
+ iscsisrc->auth) < 0)
goto error;
hostdevPriv->secinfo = secinfo;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
new file mode 100644
index 0000000..f6c0906
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu \
+-name QEMUGuest1 \
+-S \
+-object secret,id=masterKey0,format=raw,\
+file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
+-M pc \
+-m 214 \
+-smp 1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-object secret,id=virtio-disk0-ivKey0,\
+data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\
+keyid=masterKey0,iv=/////////////////////w==,format=base64 \
+-drive 'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
+mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
+password-secret=virtio-disk0-ivKey0,format=raw,if=none,id=drive-virtio-disk0' \
+-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
+id=virtio-disk0
[...]
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 1616eed..2bfbf6b 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014 Red Hat, Inc.
+ * Copyright (C) 2014-2016 Red Hat, Inc.
Doesn't look like we've touched it in 2015 ...
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -30,6 +30,13 @@
#include "virstring.h"
#include "virtpm.h"
#include "virutil.h"
+#include "virrandom.h"
+#ifdef WITH_GNUTLS
+# include <gnutls/gnutls.h>
+# if HAVE_GNUTLS_CRYPTO_H
+# include <gnutls/crypto.h>
+# endif
+#endif
#include <time.h>
#include <unistd.h>
If you use the approach I've chosen you won't need to include gnutls
headers.
@@ -145,3 +152,25 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
{
/* nada */
}
+
+int
+virRandomBytes(unsigned char *buf,
+ size_t buflen)
+{
+ size_t i;
+
+ for (i = 0; i < buflen; i++)
+ buf[i] = 0xff;
+
+ return 0;
+}
+
+#ifdef WITH_GNUTLS
+int
+gnutls_rnd(gnutls_rnd_level_t level ATTRIBUTE_UNUSED,
+ void *data,
+ size_t len)
+{
+ return virRandomBytes(data, len);
+#endif
+}
This won't compile without gnutls.
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e41444d..1f2577a 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
...
@@ -330,6 +331,14 @@ static int testCompareXMLToArgvFiles(const char
*xml,
}
}
+ /* Create a fake master key */
+ if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
+ goto out;
+
+ if (virRandomBytes(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
+ goto out;
+ priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
+
if (!(cmd = qemuProcessCreatePretendCmd(conn, &driver, vm, migrateURI,
(flags & FLAG_FIPS), false,
VIR_QEMU_PROCESS_START_COLD))) {
This calls qemuProcessPrepareDomain which calls
qemuDomainMasterKeyCreate so the above call to generating the master key
is not necessary.
3:42 p.m.
New subject: [libvirt] [PATCH v3 2/2] qemu: Utilize qemu secret objects for RBD auth/secret
On 05/11/2016 08:21 AM, Peter Krempa wrote:
On Thu, May 05, 2016 at 15:00:41 -0400, John Ferlan wrote:
> Partial fix for:
> https://bugzilla.redhat.com/show_bug.cgi?id=1182074
>
> If they're available and we need to pass secrets to qemu, then use the
> qemu domain secret object in order to pass the secrets for RBD volumes
> instead of passing the base64 encoded secret on the command line.
>
> The goal is to make IV secrets the default and have no user interaction
> required in order to allow using the IV mechanism. If the mechanism
> is not available, then fall back to the current plain mechanism using
> a base64 encoded secret.
>
> New API's:
>
> qemu_command.c:
> qemuBuildSecretInfoProps: (private)
> Generate/return a JSON properties object for the IV secret to
> be used by both the command building and eventually the hotplug
> code in order to add the secret object. Code was designed so that
> in the future perhaps hotplug could use it if it made sense.
>
> qemuBuildSecretIVCommandLine (private)
> Generate and add to the command line the -object secret for the
> IV secret. This will be required for the subsequent RBD reference
> to the object.
>
> qemuBuildDiskSecinfoCommandLine (private)
> Handle adding the IV secret object.
>
> qemu_domain.c
> qemuDomainSecretSetup: (private)
> Shim to call either the IV or Plain Setup functions based upon
> whether IV secrets are possible (we have the encryption API) or not,
> we have secrets, and of course if the protocol source is RBD.
>
> Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and
> qemuDomainSecretHostdevPrepare to add the secret rather than assuming
> plain. In the future when iSCSI secrets can use this mechanism for
> either a disk or hostdev there will be less change.
>
> Command Building:
>
> Adjust the qemuBuildRBDSecinfoURI API's in order to generate the
> specific command options for an IV secret, such as:
>
> -object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted,
> format=base64
> -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
> mon_host=mon1.example.org\:6321,password-secret=$alias,...
>
> where the 'id=' value is the secret object alias generated by
> concatenating the disk alias and "-ivKey0". The 'keyid=
$masterKey'
> is the master key shared with qemu, and the -drive syntax will
> reference that alias as the 'password-secret'. For the -drive
> syntax, the 'id=myname' is kept to define the username, while the
> 'key=$base64 encoded secret' is removed.
>
> While according to the syntax described for qemu commit '60390a21'
> or as seen in the email archive:
>
> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html
>
> it is possible to pass a plaintext password via a file, the qemu
> commit 'ac1d8878' describes the more feature rich 'keyid=' option
> based upon the shared masterKey.
>
> Tests:
>
> Add mock's for virRandomBytes and gnutls_rnd in order to return a
> constant stream of '0xff' in the bytes for a non random key in order
> to generate "constant" values for the secrets so that the tests can
> use those results to compare results.
>
> Hotplug:
>
> Since the hotplug code doesn't add command line arguments, passing
> the encoded secret directly to the monitor will suffice.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 130 ++++++++++++++++++++-
> src/qemu/qemu_domain.c | 56 +++++++--
> ...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 +++++
> ...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 +++++++
> tests/qemuxml2argvmock.c | 31 ++++-
> tests/qemuxml2argvtest.c | 11 ++
> 6 files changed, 290 insertions(+), 11 deletions(-)
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml
I think this patch is doing too much in one place. You can definitely
split out the mocking code.
Or if your mocking code is the better solution, that's fine as well.
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 93aaf2a..9b0bd7a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -607,6 +607,119 @@ qemuNetworkDriveGetPort(int protocol,
> }
>
>
> +/**
> + * qemuBuildSecretInfoProps:
> + * @secinfo: pointer to the secret info object
> + * @type: returns a pointer to a character string for object name
> + * @props: json properties to return
> + *
> + * Build the JSON properties for the secret info type.
> + *
> + * Returns 0 on success with the filled in JSON property; otherwise,
> + * returns -1 on failure error message set.
> + */
> +static int
> +qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
> + const char **type,
> + virJSONValuePtr *propsret)
> +{
> + int ret = -1;
> + char *keyid = NULL;
> + virJSONValuePtr props = NULL;
> +
> + *type = "secret";
> +
> + if (!(keyid = qemuDomainGetMasterKeyAlias()))
> + return -1;
> +
> + if (!(props = virJSONValueNewObject()))
> + goto cleanup;
> +
> + if (virJSONValueObjectAdd(props,
> + "s:data", secinfo->s.iv.ciphertext,
> + "s:keyid", keyid,
> + "s:iv", secinfo->s.iv.iv,
> + "s:format", "base64", NULL) <
0)
> + goto cleanup;
You can use virJSONValueObjectCreate instead of the two calls above.
OK
> +
> + *propsret = props;
> + props = NULL;
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(keyid);
> + virJSONValueFree(props);
> +
> + return ret;
> +}
> +
> +
> +/**
> + * qemuBuildSecretIVCommandLine:
> + * @cmd: the command to modify
> + * @secinfo: pointer to the secret info object
> + *
> + * If the secinfo is available and associated with an IV secret,
> + * then format the command line for the secret object. This object
> + * will be referenced by the device that needs/uses it, so it needs
> + * to be in place first.
> + *
> + * Returns 0 on success, -1 w/ error message on failure
> + */
> +static int
> +qemuBuildSecretIVCommandLine(virCommandPtr cmd,
Why does this contain "IV" you are creating a secret object.
It's just a name... I'll change to AES based on the previous patch.
> + qemuDomainSecretInfoPtr secinfo)
> +{
> + int ret = -1;
> + virJSONValuePtr props = NULL;
> + const char *type;
> + char *tmp = NULL;
> +
> + if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0)
> + return -1;
> +
> + if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.iv.alias,
> + props)))
> + goto cleanup;
> +
> + virCommandAddArgList(cmd, "-object", tmp, NULL);
> + ret = 0;
> +
> + cleanup:
> + virJSONValueFree(props);
> + VIR_FREE(tmp);
> +
> + return ret;
> +}
> +
> +
> +/* qemuBuildDiskSecinfoCommandLine:
> + * @cmd: Pointer to the command string
> + * @secinfo: Pointer to a possible secinfo
> + *
> + * Adding an IV secret to the command line for an iSCSI disk currently
> + * does not work. As of qemu 2.6, in order to add the options "user=" and
> + * "password-secret=", one would have to do so using an "-iscsi"
command
> + * line option. However, that requires supplying an "id=" that can be
used
> + * by some "-drive" command in order to "find" the correct IQN.
Unfortunately,
> + * an IQN consists of characters ':' and '/' that cannot be used for
an "id=".
I don't think this kind of information belongs into a function comment.
It also doesn't really describe what is going on.
OK...
> + *
> + * Returns 0 on success, -1 w/ error on some sort of failure.
> + */
> +static int
> +qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd,
> + qemuDomainSecretInfoPtr secinfo)
> +{
> + /* Not necessary for non IV secrets */
> + if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_IV)
Again, what is IV supposed to mean? The declaration of the enum is not
commented.
OK - it's changing to AES prior patch response.
> + return 0;
> +
> + /* For now we'll just build the IV. In the future more may be required
> + * in order to support iSCSI */
This comment is weird. This patch is dealing with RBD.
If you look at the previous version w/ iSCSI code included it may make
sense, but I'll dump it..
> + return qemuBuildSecretIVCommandLine(cmd, secinfo);
> +}
> +
> +
> /* qemuBuildGeneralSecinfoURI:
> * @uri: Pointer to the URI structure to add to
> * @secinfo: Pointer to the secret info data (if present)
> @@ -677,6 +790,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf,
> break;
>
> case VIR_DOMAIN_SECRET_INFO_TYPE_IV:
> + virBufferEscape(buf, '\\', ":",
":id=%s:auth_supported=cephx\\;none",
> + secinfo->s.iv.username);
> + break;
> +
> case VIR_DOMAIN_SECRET_INFO_TYPE_LAST:
> return -1;
> }
> @@ -1083,6 +1200,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
> char *source = NULL;
> int actualType = virStorageSourceGetActualType(disk->src);
> qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>
> if (idx < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1164,7 +1282,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
> break;
> }
>
> - if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source)
< 0)
> + if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0)
> goto error;
>
> if (source &&
> @@ -1215,6 +1333,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>
> virBufferEscape(&opt, ',', ",", "%s,",
source);
>
> + if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
> + secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_IV)
{
> + virBufferAsprintf(&opt, "password-secret=%s,",
secinfo->s.iv.alias);
This could be added to the switch above the place where you are doing
this.
Not clear where you mean...
We add "file=" first inside the if (source && ... That goes
immediately
after the "-drive "
That switch would add "fat:", "floppy:", then do some escape magic on
source while adding it to the line including the trailing comma.
Then add the "password-secret=%s,"
Result is:
"-drive file=$source,password-secret..."
> + }
> +
> if (disk->src->format > 0 &&
> disk->src->type != VIR_STORAGE_TYPE_DIR)
> virBufferAsprintf(&opt, "format=%s,",
> @@ -1905,6 +2028,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> virDomainDiskDefPtr disk = def->disks[i];
> bool withDeviceArg = false;
> bool deviceFlagMasked = false;
> + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>
> /* Unless we have -device, then USB disks need special
> handling */
> @@ -1947,6 +2072,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> break;
> }
>
> + if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)
qemuBuildDiskSecretCommanLine?
Not sure I follow. The "-object secret" for the IV (or AES) secret must
be in place before the "-drive" option for a disk that needs it.
Eventually there will be a HostdevSecinfo.
> + return -1;
> +
> virCommandAddArg(cmd, "-drive");
>
> /* Unfortunately it is not possible to use
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b174caa..3e627a5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -897,7 +897,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
> *
> * Returns true if we can support the encryption code, false otherwise
> */
> -static bool ATTRIBUTE_UNUSED
> +static bool
> qemuDomainSecretHaveEncrypt(void)
> {
> #ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
> @@ -920,7 +920,7 @@ qemuDomainSecretHaveEncrypt(void)
> *
> * Returns 0 on success, -1 on failure with error message
> */
> -static int ATTRIBUTE_UNUSED
> +static int
> qemuDomainSecretIVSetup(virConnectPtr conn,
> qemuDomainObjPrivatePtr priv,
> qemuDomainSecretInfoPtr secinfo,
> @@ -1030,6 +1030,44 @@ qemuDomainSecretIVSetup(virConnectPtr conn,
> }
>
>
> +/* qemuDomainSecretSetup:
> + * @conn: Pointer to connection
> + * @priv: pointer to domain private object
> + * @secinfo: Pointer to secret info
> + * @srcalias: Alias of the disk/hostdev used to generate the secret alias
> + * @protocol: Protocol for secret
> + * @authdef: Pointer to auth data
> + *
> + * If we have the encryption API present and can support a secret object, then
> + * build the IV secret; otherwise, build the Plain secret. This is the magic
> + * decision point for utilizing the IV secrets for an RBD disk. For now iSCSI
> + * disks and hostdevs will not be able to utilize this mechanism. For details,
> + * see qemuBuildDiskSecinfoCommandLine in qemu_command.c
I'll adjust this comment too...
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +static int
> +qemuDomainSecretSetup(virConnectPtr conn,
> + qemuDomainObjPrivatePtr priv,
> + qemuDomainSecretInfoPtr secinfo,
> + const char *srcalias,
> + virStorageNetProtocol protocol,
> + virStorageAuthDefPtr authdef)
> +{
> + if (qemuDomainSecretHaveEncrypt() &&
> + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) &&
> + protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> + if (qemuDomainSecretIVSetup(conn, priv, secinfo,
> + srcalias, protocol, authdef) < 0)
> + return -1;
> + } else {
> + if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0)
> + return -1;
> + }
> + return 0;
Looks like this should be in the previous patch so that the function
doesn't need to be unused.
AH and that's the rub... If this goes in the previous patch, then we
create and expect to use an IV (AES) secret; however, without the rest
of the code in this patch we don't know how to build one. So we
"assume" it's a Plain secret, which quite obviously it isn't.
Perhaps it'll just be easier to combine the two patches - more to review
at one time though.
> +}
> +
> +
> /* qemuDomainSecretDiskDestroy:
> * @disk: Pointer to a disk definition
> *
> @@ -1058,7 +1096,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
> */
> int
> qemuDomainSecretDiskPrepare(virConnectPtr conn,
> - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
> + qemuDomainObjPrivatePtr priv,
> virDomainDiskDefPtr disk)
> {
> virStorageSourcePtr src = disk->src;
> @@ -1075,8 +1113,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
> if (VIR_ALLOC(secinfo) < 0)
> return -1;
>
> - if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
> - src->auth) < 0)
> + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias,
> + src->protocol, src->auth) < 0)
> goto error;
>
> diskPriv->secinfo = secinfo;
> @@ -1119,7 +1157,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev)
> */
> int
> qemuDomainSecretHostdevPrepare(virConnectPtr conn,
> - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
> + qemuDomainObjPrivatePtr priv,
> virDomainHostdevDefPtr hostdev)
> {
> virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys;
> @@ -1140,9 +1178,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn,
> if (VIR_ALLOC(secinfo) < 0)
> return -1;
>
> - if (qemuDomainSecretPlainSetup(conn, secinfo,
> - VIR_STORAGE_NET_PROTOCOL_ISCSI,
> - iscsisrc->auth) < 0)
> + if (qemuDomainSecretSetup(conn, priv, secinfo,
hostdev->info->alias,
> + VIR_STORAGE_NET_PROTOCOL_ISCSI,
> + iscsisrc->auth) < 0)
> goto error;
>
> hostdevPriv->secinfo = secinfo;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
> new file mode 100644
> index 0000000..f6c0906
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
> @@ -0,0 +1,31 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu \
> +-name QEMUGuest1 \
> +-S \
> +-object secret,id=masterKey0,format=raw,\
> +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
> +-M pc \
> +-m 214 \
> +-smp 1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
> +-object secret,id=virtio-disk0-ivKey0,\
> +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\
> +keyid=masterKey0,iv=/////////////////////w==,format=base64 \
> +-drive 'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
> +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
> +password-secret=virtio-disk0-ivKey0,format=raw,if=none,id=drive-virtio-disk0' \
> +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
> +id=virtio-disk0
[...]
> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index 1616eed..2bfbf6b 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2014 Red Hat, Inc.
> + * Copyright (C) 2014-2016 Red Hat, Inc.
Doesn't look like we've touched it in 2015 ...
git log -p tests/qemuxml2argvmock.c shows me...
...
commit ace1ee225f5cd87fb095054a6a19bdcd0fa57518
Author: Peter Krempa <pkrempa(a)redhat.com>
Date: Thu Dec 10 14:36:51 2015 +0100
,,,
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -30,6 +30,13 @@
> #include "virstring.h"
> #include "virtpm.h"
> #include "virutil.h"
> +#include "virrandom.h"
> +#ifdef WITH_GNUTLS
> +# include <gnutls/gnutls.h>
> +# if HAVE_GNUTLS_CRYPTO_H
> +# include <gnutls/crypto.h>
> +# endif
> +#endif
> #include <time.h>
> #include <unistd.h>
If you use the approach I've chosen you won't need to include gnutls
headers.
I'm OK with going with your approach... I think I explained why I went
with virRandomBytes in response to your patch.
John
>
> @@ -145,3 +152,25 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
> {
> /* nada */
> }
> +
> +int
> +virRandomBytes(unsigned char *buf,
> + size_t buflen)
> +{
> + size_t i;
> +
> + for (i = 0; i < buflen; i++)
> + buf[i] = 0xff;
> +
> + return 0;
> +}
> +
> +#ifdef WITH_GNUTLS
> +int
> +gnutls_rnd(gnutls_rnd_level_t level ATTRIBUTE_UNUSED,
> + void *data,
> + size_t len)
> +{
> + return virRandomBytes(data, len);
> +#endif
> +}
This won't compile without gnutls.
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e41444d..1f2577a 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
...
> @@ -330,6 +331,14 @@ static int testCompareXMLToArgvFiles(const char *xml,
> }
> }
>
> + /* Create a fake master key */
> + if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
> + goto out;
> +
> + if (virRandomBytes(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
> + goto out;
> + priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
> +
> if (!(cmd = qemuProcessCreatePretendCmd(conn, &driver, vm, migrateURI,
> (flags & FLAG_FIPS), false,
> VIR_QEMU_PROCESS_START_COLD))) {
This calls qemuProcessPrepareDomain which calls
qemuDomainMasterKeyCreate so the above call to generating the master key
is not necessary.
3056
days inactive
3062
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Peter Krempa