Re: [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
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)