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@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

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@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

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@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@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.