[libvirt] [PATCH v4 2/3] qemu: Introduce new Secret AES API's

[Peter Krempa]

On Wed, May 18, 2016 at 18:45:21 -0400, John Ferlan wrote:
> On 05/18/2016 03:16 AM, Peter Krempa wrote:
> > On Tue, May 17, 2016 at 12:36:08 -0400, John Ferlan wrote:

[...]

> >> +
> >> +    /* Grab the unencoded secret */
> > 
> > [1]
> > 
> >> +    if (virSecretGetSecretString(conn, authdef, secretType,
> >> +                                 &secret, &secretlen) < 0)
> >> +        goto cleanup;
> >> +
> >> +    /* Allocate a padded buffer */
> >> +    ciphertextlen = VIR_ROUND_UP(secretlen, 16);
> >> +    if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
> >> +        goto cleanup;
> >> +    memcpy(ciphertext, secret, secretlen);
> >> +    for (i = secretlen; i < ciphertextlen; i++)
> >> +        ciphertext[i] = ciphertextlen - secretlen;
> > 
> > The memset here was okay along with this. What I've asked for is to add
> > a comment for one of the few non-obvious parts of the code once you
> > didn't spare comments for the obvious stuff [1].
> > 
> 
> So the comment "/* this is black magic */" is sufficient, right? ;-)

Almost. It'd at least switch me from "this looks wrong" to "oh,
something interresting is going on (also it's expected)".

> 
> I can be less verbose on the [1] comments though. They are derived when
> I'm putting together the code so I don't forget a step...

I just tried to point out that there are certain places where comments
are actually needed and that is really one of them. The rest doesn't
hurt but is obvious compared to this magic part.

> >> +    /* Initialize the gnutls cipher */
> >> +    enc_key.size = QEMU_DOMAIN_MASTER_KEY_LEN;
> >> +    enc_key.data = priv->masterKey;
> >> +    iv_key.size = QEMU_DOMAIN_AES_IV_LEN;
> >> +    iv_key.data = raw_iv;

Also while changing this. s/iv_key/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 encode the ciphertext and store to be passed to qemu */
> >> +    if (!(secinfo->s.aes.ciphertext = virStringEncodeBase64(ciphertext,
> >> +                                                            ciphertextlen)))
> >> +        goto cleanup;
> >> +
> >> +    ret = 0;
> >> +
> >> + cleanup:
> >> +    VIR_FREE(raw_iv);
> > 
> > Since you are wiping the ciphertext you can wipe the IV too.
> > 
> 
> I think I tried that, but VIR_DISPOSE_N didn't seem to be happy with
> receiving QEMU_DOMAIN_AES_IV_LEN

Oh right. I chose to clear the second argument too in VIR_DISPOSE_N so
it has to be an L-value. Don't bother changing this then. IV is still
going to be put on the command line.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160519/4369fdf3/attachment-0001.sig>