Re: [libvirt] [PATCH v2 09/15] encryption: Add <cipher> and <ivgen> to encryption

On Thu, Jun 23, 2016 at 13:29:05 -0400, John Ferlan wrote: > For a luks device, allow the configuration of a specific cipher to be > used for encrypting the volume. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > docs/formatstorageencryption.html.in | 81 +++++++++++++- > docs/schemas/storagecommon.rng | 44 +++++++- > src/conf/domain_conf.c | 11 ++ > src/util/virstorageencryption.c | 124 +++++++++++++++++++++ > src/util/virstorageencryption.h | 13 +++ > .../qemuxml2argv-luks-disk-cipher.xml | 41 +++++++ > .../qemuxml2xmlout-luks-disk-cipher.xml | 45 ++++++++ > tests/qemuxml2xmltest.c | 1 + > tests/storagevolxml2xmlin/vol-luks-cipher.xml | 23 ++++ > tests/storagevolxml2xmlout/vol-luks-cipher.xml | 23 ++++ > tests/storagevolxml2xmltest.c | 1 + > 11 files changed, 401 insertions(+), 6 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml > create mode 100644 tests/storagevolxml2xmlin/vol-luks-cipher.xml > create mode 100644 tests/storagevolxml2xmlout/vol-luks-cipher.xml > > diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in > index 3a08192..80111e3 100644 > --- a/docs/formatstorageencryption.html.in > +++ b/docs/formatstorageencryption.html.in > @@ -71,6 +71,58 @@ > be used as the passphrase to decrypt the volume. > <span class="since">Since 2.0.0</span>. > </p> > + <p> > + For volume creation, it is possible to specify the encryption > + algorithm used to encrypt the luks volume. The following two > + optional elements may be provided for that purpose. It is hypervisor > + dependent as to which algorithms are supported. The default algorithm > + for QEMU is 'aes-256-cbc' using 'essiv' for initialization vector At this point this is the default not for qemu, but for the 'DIR' backend for the storage driver that is using qemu-img.

> + generation and 'sha256' hash algorithm for both the cipher and the > + initialization vector generation. > + </p> > + > + <dl> > + <dt><code>cipher</code></dt> > + <dd>This element describes the cipher algorithm to be used to either > + encrypt or decrypt the luks volume. This element has the following > + attributes: > + <dl> > + <dt><code>name</code></dt> > + <dd>The name of the cipher algorithm used for data encryption, > + such as 'aes', 'des', 'cast5', 'serpent', 'twofish', etc. > + Support of the specific algorithm is hypervisor dependent.</dd> This is storage driver implementation dependent.

> + <dt><code>size</code></dt> > + <dd>The size of the cipher in bits, such as '256', '192', '128', > + etc. Support of the specific size for a specific cipher is > + hypervisor dependent.</dd> > + <dt><code>mode</code></dt> > + <dd>An optional cipher algorithm mode such as 'cbc', 'xts', > + 'ecb', etc. Support of the specific cipher mode is > + hypervisor dependent.</dd> > + <dt><code>hash</code></dt> > + <dd>An optional master key hash algorithm such as 'md5', 'sha1', > + 'sha256', etc. Support of the specific hash algorithm is > + hypervisor dependent.</dd> > + </dl> > + </dd> > + <dt><code>ivgen</code></dt> > + <dd>This optional element describes the initialization vector > + generation algorithm used in conjunction with the > + <code>cipher</code>. If the <code>cipher</code> is not provided, > + then an error will be generated by the parser. > + <dl> > + <dt><code>name</code></dt> > + <dd>The name of the algorithm, such as 'plain', 'plain64', > + 'essiv', etc. Support of the specific algorithm is hypervisor > + dependent.</dd> > + <dt><code>hash</code></dt> > + <dd>An optional hash algorithm such as 'md5', 'sha1', 'sha256', > + etc. Support of the specific ivgen hash algorithm is hypervisor > + dependent.</dd> > + </dl> > + </dd> > + </dl> > + > > <h2><a name="example">Examples</a></h2> > > @@ -84,9 +136,12 @@ > &lt;/encryption&gt;</pre> > > <p> > - Here is a simple example, specifying use of the <code>luks</code> format > - where it's assumed that a <code>secret</code> has been defined using a > - <code>usage</code> element with a <code>id</code> of "luks_example": > + Assuming a <a href="formatsecret.html#luksUsageType"> > + <code>luks secret</code></a> is already defined using a > + <code>usage</code> element with an <code>id</code> of "luks_example", s/id/name/

> + a simple example specifying use of the <code>luks</code> format > + for either volume creation without a specific cipher being defined or > + as part of a domain volume definition: > </p> > <pre> > &lt;encryption format='luks'&gt; > @@ -94,5 +149,25 @@ > &lt;/encryption&gt; > </pre> > > + <p> > + Here is an example, specifying use of the <code>luks</code> format for > + a specific cipher algorihm for volume creation: > + </p> > + <pre> > + &lt;volume&gt; > + &lt;name&gt;twofish.luks&lt;/name&gt; > + &lt;capacity unit='G'&gt;5&lt;/capacity&gt; > + &lt;target&gt; > + &lt;path&gt;/var/lib/libvirt/images/demo.luks&lt;/path&gt; > + &lt;format type='luks'/&gt; > + &lt;encryption format='luks'&gt; > + &lt;secret type='passphrase' usage='luks_example'/&gt; > + &lt;cipher name='twofish' size='256' mode='cbc' hash='sha256'/&gt; > + &lt;ivgen name='plain64' hash='sha256'/&gt; > + &lt;/encryption&gt; > + &lt;/target&gt; > + &lt;/volume&gt; > + </pre> > + > </body> > </html> [...] > diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h > index 5e1be3b..34bba03 100644 > --- a/src/util/virstorageencryption.h > +++ b/src/util/virstorageencryption.h > @@ -44,6 +44,16 @@ struct _virStorageEncryptionSecret { > virSecretLookupTypeDef seclookupdef; > }; > > +/* For a key type it's possible to dictate the cipher and if necessary iv */ > +typedef struct _virStorageEncryptionInfoDef virStorageEncryptionInfoDef; > +typedef virStorageEncryptionInfoDef *virStorageEncryptionInfoDefPtr; > +struct _virStorageEncryptionInfoDef { > + unsigned int size; > + char *name; > + char *mode; > + char *hash; I'd either store the IV generator info here or add a different structure. It seems weird to reuse it just because it has two fields with same name.

> +}; > + > typedef enum { > /* "default" is only valid for volume creation */ > VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT = 0, [...] > virStorageEncryptionPtr virStorageEncryptionCopy(const virStorageEncryption *src) > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml > new file mode 100644 > index 0000000..00399cf > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml [...] > diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml > new file mode 100644 > index 0000000..9ce15c0 > --- /dev/null > +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml Again the output file is not really needed: $ diff -u qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml --- qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml 2016-06-24 15:55:42.059454563 +0200 +++ qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml 2016-06-24 15:55:42.059454563 +0200 @@ -32,10 +32,14 @@ </encryption> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </disk> - <controller type='usb' index='0'/> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> <controller type='pci' index='0' model='pci-root'/> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> - <memballoon model='virtio'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> </devices> </domain> Difference to the test files added in previous patch makes this test even less useful: $ diff -u qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml qemuxml2argvdata/qemuxml2argv-luks-disks.xml $