[libvirt] [PATCH v4 0/7] Add support for LUKS encrypted devices

According to Dan's post commit response: http://www.redhat.com/archives/libvir-list/2016-July/msg00088.html to the v3 series: http://www.redhat.com/archives/libvir-list/2016-June/msg01935.html using a 'passphrase' usage is not desired, rather a 'volume' usage model should be used for LUKS. So patches 1 & 2 make those alterations to already pushed docs and tests Patch 3 then repurposes the 'passphrase' usage to a 'tls' usage type. I posted with this series since it removed the 'passphrase' usage and thus flushed out any errors in subsequent patches. I could hold off and repost it with the TLS changes that will also need to be made... Patches 4-7 were reviewed previously and had been given what I took as provisional ACK's; however, I reposted the changes after the most recent review "just in case". Fortunately (I guess) I didn't push them along with the other changes. In any case, there are once again posted here - the primary difference between what's posted in this series vs. what was posted previously is the change to use a "volume" secret plus a tweak to the qemuxml2argvtest to fix some issues found while making the change. John Ferlan (7): tests: Adjust LUKS tests to use 'volume' secret type docs: Update docs to reflect LUKS secret changes Repurpose the 'passphrase' secret to 'tls' storage: Add support to create a luks volume qemu: Add secinfo for hotplug virtio disk qemu: Alter the qemuDomainGetSecretAESAlias to add new arg qemu: Add luks support for domain disk docs/aclpolkit.html.in | 2 +- docs/formatsecret.html.in | 81 +++++--- docs/formatstorage.html.in | 16 ++ docs/formatstorageencryption.html.in | 29 ++- docs/schemas/secret.rng | 6 +- include/libvirt/libvirt-secret.h | 2 +- src/access/viraccessdriverpolkit.c | 2 +- src/conf/secret_conf.c | 12 +- src/conf/virsecretobj.c | 2 +- src/libvirt_private.syms | 1 + src/qemu/qemu_alias.c | 10 +- src/qemu/qemu_alias.h | 3 +- src/qemu/qemu_command.c | 9 + src/qemu/qemu_domain.c | 40 +++- src/qemu/qemu_hotplug.c | 126 +++++++++++- src/storage/storage_backend.c | 218 +++++++++++++++++++-- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 +++ src/util/virqemu.h | 6 + .../qemuxml2argv-luks-disk-cipher.xml | 45 ----- .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 ++++ tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 2 +- tests/qemuxml2argvtest.c | 24 ++- .../qemuxml2xmlout-luks-disk-cipher.xml | 1 - tests/qemuxml2xmltest.c | 1 - tests/secretxml2xmlin/usage-passphrase.xml | 7 - tests/secretxml2xmlin/usage-tls.xml | 7 + tests/secretxml2xmltest.c | 2 +- tests/storagevolxml2argvtest.c | 3 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 33 files changed, 577 insertions(+), 150 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args delete mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml delete mode 100644 tests/secretxml2xmlin/usage-passphrase.xml create mode 100644 tests/secretxml2xmlin/usage-tls.xml -- 2.5.5

Commit id's '9bbf0d7e6' and '2552fec24' added some XML parsing tests for a LUKS volume to use a 'passphrase' secret format. After commit, this was deemed to be incorrect, so covert the various tests to use the volume usage format where the 'usage' is the path to the volume rather than a user defined name string. Also, removed the qemuxml2argv-luks-disk-cipher.xml since it was just a duplicate of qemuxml2argv-luks-disks.xml. Signed-off-by: John Ferlan <jferlan@redhat.com> --- .../qemuxml2argv-luks-disk-cipher.xml | 45 ---------------------- tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 2 +- .../qemuxml2xmlout-luks-disk-cipher.xml | 1 - tests/qemuxml2xmltest.c | 1 - tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 8 files changed, 5 insertions(+), 52 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml delete mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml deleted file mode 100644 index 9ce15c0..0000000 --- a/tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml +++ /dev/null @@ -1,45 +0,0 @@ -<domain type='qemu'> - <name>encryptdisk</name> - <uuid>496898a6-e6ff-f7c8-5dc2-3cf410945ee9</uuid> - <memory unit='KiB'>1048576</memory> - <currentMemory unit='KiB'>524288</currentMemory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='x86_64' machine='pc-i440fx-2.1'>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='file' device='disk'> - <driver name='qemu' type='luks'/> - <source file='/storage/guest_disks/encryptdisk'/> - <target dev='vda' bus='virtio'/> - <encryption format='luks'> - <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> - </encryption> - <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> - </disk> - <disk type='file' device='disk'> - <driver name='qemu' type='luks'/> - <source file='/storage/guest_disks/encryptdisk2'/> - <target dev='vdb' bus='virtio'/> - <encryption format='luks'> - <secret type='passphrase' usage='mycluster_myname'/> - </encryption> - <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> - </disk> - <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'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> - </memballoon> - </devices> -</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml index 9ce15c0..4c9c4c7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml @@ -28,7 +28,7 @@ <source file='/storage/guest_disks/encryptdisk2'/> <target dev='vdb' bus='virtio'/> <encryption format='luks'> - <secret type='passphrase' usage='mycluster_myname'/> + <secret type='passphrase' usage='/storage/guest_disks/encryptdisk2'/> </encryption> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </disk> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml deleted file mode 120000 index fa55233..0000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index eb810e3..d62e536 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -557,7 +557,6 @@ mymain(void) DO_TEST("encrypted-disk"); DO_TEST("encrypted-disk-usage"); DO_TEST("luks-disks"); - DO_TEST("luks-disk-cipher"); DO_TEST("memtune"); DO_TEST("memtune-unlimited"); DO_TEST("blkiotune"); diff --git a/tests/storagevolxml2xmlin/vol-luks-cipher.xml b/tests/storagevolxml2xmlin/vol-luks-cipher.xml index 009246f..da28a27 100644 --- a/tests/storagevolxml2xmlin/vol-luks-cipher.xml +++ b/tests/storagevolxml2xmlin/vol-luks-cipher.xml @@ -15,7 +15,7 @@ <label>unconfined_u:object_r:virt_image_t:s0</label> </permissions> <encryption format='luks'> - <secret type='passphrase' usage='mumblyfratz'/> + <secret type='passphrase' uuid='f52a81b2-424e-490c-823d-6bd4235bc572'/> <cipher name='serpent' size='256' mode='cbc' hash='sha256'/> <ivgen name='plain64' hash='sha256'/> </encryption> diff --git a/tests/storagevolxml2xmlin/vol-luks.xml b/tests/storagevolxml2xmlin/vol-luks.xml index eb4dc41..bf3c519 100644 --- a/tests/storagevolxml2xmlin/vol-luks.xml +++ b/tests/storagevolxml2xmlin/vol-luks.xml @@ -15,7 +15,7 @@ <label>unconfined_u:object_r:virt_image_t:s0</label> </permissions> <encryption format='luks'> - <secret type='passphrase' usage='mumblyfratz'/> + <secret type='passphrase' uuid='f52a81b2-424e-490c-823d-6bd4235bc572'/> </encryption> </target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-luks-cipher.xml b/tests/storagevolxml2xmlout/vol-luks-cipher.xml index 9014849..1ac7424 100644 --- a/tests/storagevolxml2xmlout/vol-luks-cipher.xml +++ b/tests/storagevolxml2xmlout/vol-luks-cipher.xml @@ -15,7 +15,7 @@ <label>unconfined_u:object_r:virt_image_t:s0</label> </permissions> <encryption format='luks'> - <secret type='passphrase' usage='mumblyfratz'/> + <secret type='passphrase' uuid='f52a81b2-424e-490c-823d-6bd4235bc572'/> <cipher name='serpent' size='256' mode='cbc' hash='sha256'/> <ivgen name='plain64' hash='sha256'/> </encryption> diff --git a/tests/storagevolxml2xmlout/vol-luks.xml b/tests/storagevolxml2xmlout/vol-luks.xml index 5b764b7..7b82866 100644 --- a/tests/storagevolxml2xmlout/vol-luks.xml +++ b/tests/storagevolxml2xmlout/vol-luks.xml @@ -15,7 +15,7 @@ <label>unconfined_u:object_r:virt_image_t:s0</label> </permissions> <encryption format='luks'> - <secret type='passphrase' usage='mumblyfratz'/> + <secret type='passphrase' uuid='f52a81b2-424e-490c-823d-6bd4235bc572'/> </encryption> </target> </volume> -- 2.5.5

On Mon, Jul 11, 2016 at 02:07:52PM -0400, John Ferlan wrote:
Commit id's '9bbf0d7e6' and '2552fec24' added some XML parsing tests for a LUKS volume to use a 'passphrase' secret format. After commit, this was deemed to be incorrect, so covert the various tests to use the volume usage format where the 'usage' is the path to the volume rather than a user defined name string.
Also, removed the qemuxml2argv-luks-disk-cipher.xml since it was just a duplicate of qemuxml2argv-luks-disks.xml.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- .../qemuxml2argv-luks-disk-cipher.xml | 45 ---------------------- tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 2 +- .../qemuxml2xmlout-luks-disk-cipher.xml | 1 - tests/qemuxml2xmltest.c | 1 - tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 8 files changed, 5 insertions(+), 52 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml delete mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml
ACK Jan

Commit id's 'c8438010', '9bbf0d7e', and '2552fec24' altered the documentation to describe adding a 'passphrase' type secret usage model in order to reference the secret for a luks volume. After commit, it was deemed that a 'volume' usage model should be used, so adjust the various documents in order rephrase descriptions in order to follow the correct usage model. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatsecret.html.in | 104 +++++++++++++---------------------- docs/formatstorage.html.in | 16 ++++++ docs/formatstorageencryption.html.in | 29 ++++------ 3 files changed, 66 insertions(+), 83 deletions(-) diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index ffaa7dc..216a83c 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -41,19 +41,20 @@ <dd> Specifies what this secret is used for. A mandatory <code>type</code> attribute specifies the usage category, currently - only <code>volume</code>, <code>ceph</code>, <code>iscsi</code>, - and <code>passphrase</code> are defined. Specific usage categories - are described below. + only <code>volume</code>, <code>ceph</code>, and <code>iscsi</code> + are defined. Specific usage categories are described below. </dd> </dl> <h3><a name="VolumeUsageType">Usage type "volume"</a></h3> <p> - This secret is associated with a volume, and it is safe to delete the - secret after the volume is deleted. The <code><usage - type='volume'></code> element must contain a - single <code>volume</code> element that specifies the key of the volume + This secret is associated with a volume, whether the format is either + for a "qcow" or a "luks" encrypted volume. Each volume will have a + unique secret associated with it and it is safe to delete the + secret after the volume is deleted. The + <code><usage type='volume'></code> element must contain a + single <code>volume</code> element that specifies the path of the volume this secret is associated with. For example, create a volume-secret.xml file as follows: </p> @@ -69,7 +70,7 @@ </pre> <p> - Define the secret and set the pass phrase as follows: + Define the secret and set the passphrase as follows: </p> <pre> # virsh secret-define volume-secret.xml @@ -82,8 +83,8 @@ </pre> <p> - The volume type secret can then be used in the XML for a storage volume - <a href="formatstorageencryption.html">encryption</a> as follows: + The volume type secret can be supplied in domain XML for a qcow storage + volume <a href="formatstorageencryption.html">encryption</a> as follows: </p> <pre> <encryption format='qcow'> @@ -91,6 +92,33 @@ </encryption> </pre> + <p> + The volume type secret can be supplied either in volume XML during + creation of a <a href="formatstorage.html#StorageVol">storage volume</a> + in order to provide the passphrase to encrypt the volume or in + domain XML <a href="formatdomain.html#elementsDisks">disk device</a> + in order to provide the passphrase to decrypt the volume, + <span class="since">since 2.1.0</span>. An example follows: + </p> + <pre> + # cat luks-secret.xml + <secret ephemeral='no' private='yes'> + <description>LUKS Sample Secret</description> + <uuid>f52a81b2-424e-490c-823d-6bd4235bc57</uuid> + <usage type='volume'> + <volume>/var/lib/libvirt/images/luks-sample.img</volume> + </usage> + </secret> + + # virsh secret-define luks-secret.xml + Secret f52a81b2-424e-490c-823d-6bd4235bc57 created + # + # MYSECRET=`printf %s "letmein" | base64` + # virsh secret-set-value f52a81b2-424e-490c-823d-6bd4235bc57 $MYSECRET + Secret value set + # + </pre> + <h3><a name="CephUsageType">Usage type "ceph"</a></h3> <p> This secret is associated with a Ceph RBD (rados block device). @@ -243,61 +271,5 @@ </auth> </pre> - <h3><a name="passphraseUsageType">Usage type "passphrase"</a></h3> - - <p> - This secret is a general purpose secret to be used by various libvirt - objects to provide a single passphrase as required by the object in - order to perform its authentication. For example, this secret will - be used either by the - <a href="formatstorage.html#StorageVol">storage volume</a> in order to - provide the passphrase to encrypt a luks volume or by the - <a href="formatdomain.html#elementsDisks">disk device</a> in order to - provide the passphrase to decrypt the luks volume for usage. - <span class="since">Since 2.1.0</span>. The following is an example - of a secret.xml file: - </p> - - <pre> - # cat secret.xml - <secret ephemeral='no' private='yes'> - <description>sample passphrase secret</description> - <usage type='passphrase'> - <name>name_example</name> - </usage> - </secret> - - # virsh secret-define secret.xml - Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created - - # virsh secret-list - UUID Usage - ----------------------------------------------------------- - 718c71bd-67b5-4a2b-87ec-a24e8ca200dc passphrase name_example - # - - </pre> - - <p> - A secret may also be defined via the - <a href="html/libvirt-libvirt-secret.html#virSecretDefineXML"> - <code>virSecretDefineXML</code></a> API. - - Once the secret is defined, a secret value will need to be set. This - value would be the same used to create and use the volume. - The following is a simple example of using - <code>virsh secret-set-value</code> to set the secret value. The - <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> - <code>virSecretSetValue</code></a> API may also be used to set - a more secure secret without using printable/readable characters. - </p> - - <pre> - # MYSECRET=`printf %s "letmein" | base64` - # virsh secret-set-value 718c71bd-67b5-4a2b-87ec-a24e8ca200dc $MYSECRET - Secret value set - - </pre> - </body> </html> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 94277a1..a700e85 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -752,5 +752,21 @@ </permissions> </target> </volume></pre> + + <h3><a name="exampleLuks">Storage volume using LUKS</a></h3> + + <pre> + <volume> + <name>MyLuks.img</name> + <capacity unit="G">5</capacity> + <target> + <path>/var/lib/virt/images/MyLuks.img</path> + <format type='luks'/> + <encryption format='luks'> + <secret type='passphrase' uuid='f52a81b2-424e-490c-823d-6bd4235bc572'/> + </encryption> + </target> + </volume> + </pre> </body> </html> diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in index f2b0ffd..11af97e 100644 --- a/docs/formatstorageencryption.html.in +++ b/docs/formatstorageencryption.html.in @@ -27,9 +27,9 @@ <code>secret</code> tags, each with mandatory attributes <code>type</code> and either <code>uuid</code> or <code>usage</code> (<span class="since">since 2.1.0</span>). The only currently defined - value of <code>type</code> is <code>passphrase</code>. The + value of <code>type</code> is <code>volume</code>. The <code>uuid</code> is "uuid" of the <code>secret</code> while - <code>usage</code> is the value "usage" subelement field. + <code>usage</code> is the "usage" subelement field. A secret value can be set in libvirt by the <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> <code>virSecretSetValue</code></a> API. Alternatively, if supported @@ -40,7 +40,7 @@ <h3><a name="StorageEncryptionDefault">"default" format</a></h3> <p> <code><encryption format="default"/></code> can be specified only - when creating a volume. If the volume is successfully created, the + when creating a qcow volume. If the volume is successfully created, the encryption formats, parameters and secrets will be auto-generated by libvirt and the attached <code>encryption</code> tag will be updated. The unmodified contents of the <code>encryption</code> tag can be used @@ -59,13 +59,9 @@ <h3><a name="StorageEncryptionLuks">"luks" format</a></h3> <p> The <code>luks</code> format is specific to a luks encrypted volume - and the secret used in order to either encrypt or decrypt the volume. - A single <code><secret type='passphrase'...></code> element is - expected. The secret may be referenced via either a <code>uuid</code> or - <code>usage</code> attribute. One of the two must be present. When - present for volume creation, the secret will be used in order for - volume encryption. When present for domain usage, the secret will - be used as the passphrase to decrypt the volume. + and the secret is used in order to either encrypt during volume creation + or decrypt the volume for usage by the domain. A single + <code><secret type='passphrase'...></code> element is expected. <span class="since">Since 2.1.0</span>. </p> <p> @@ -135,22 +131,21 @@ </encryption></pre> <p> - Assuming a <a href="formatsecret.html#luksUsageType"> - <code>luks secret</code></a> is already defined using a - <code>usage</code> element with an <code>name</code> of "luks_example", + Assuming a <a href="formatsecret.html#VolumeUsageType"> + <code>luks volume type secret</code></a> is already defined, 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> <encryption format='luks'> - <secret type='passphrase' usage='luks_example'/> + <secret type='passphrase' uuid='f52a81b2-424e-490c-823d-6bd4235bc572'/> </encryption> </pre> <p> - Here is an example, specifying use of the <code>luks</code> format for - a specific cipher algorihm for volume creation: + Here is an example specifying use of the <code>luks</code> format for + a specific cipher algorithm for volume creation: </p> <pre> <volume> @@ -160,7 +155,7 @@ <path>/var/lib/libvirt/images/demo.luks</path> <format type='luks'/> <encryption format='luks'> - <secret type='passphrase' usage='luks_example'/> + <secret type='passphrase' uuid='f52a81b2-424e-490c-823d-6bd4235bc572'/> <cipher name='twofish' size='256' mode='cbc' hash='sha256'/> <ivgen name='plain64' hash='sha256'/> </encryption> -- 2.5.5

On Mon, Jul 11, 2016 at 02:07:53PM -0400, John Ferlan wrote:
Commit id's 'c8438010', '9bbf0d7e', and '2552fec24' altered the documentation to describe adding a 'passphrase' type secret usage model in order to reference the secret for a luks volume. After commit, it was deemed that a 'volume' usage model should be used, so adjust the various documents in order rephrase descriptions in order to follow the correct usage model.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatsecret.html.in | 104 +++++++++++++---------------------- docs/formatstorage.html.in | 16 ++++++ docs/formatstorageencryption.html.in | 29 ++++------ 3 files changed, 66 insertions(+), 83 deletions(-)
ACK Jan

Commit id 'c84380106' added support for a secret usage type 'passphrase' that was designed to be used for both LUKS encryption and TLS credentials since both used a 'simple' passphrase in order to handle the authentication. However, a post commit review deemed that usage model to be invalid. This patch repurposes the 'passphrase' usage type for 'tls' specific usage. A previous patch has already adjusted the various LUKS usages to utilize a 'volume' secret. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/aclpolkit.html.in | 2 +- docs/formatsecret.html.in | 59 +++++++++++++++++++++++++++++- docs/schemas/secret.rng | 6 +-- include/libvirt/libvirt-secret.h | 2 +- src/access/viraccessdriverpolkit.c | 2 +- src/conf/secret_conf.c | 12 +++--- src/conf/virsecretobj.c | 2 +- tests/secretxml2xmlin/usage-passphrase.xml | 7 ---- tests/secretxml2xmlin/usage-tls.xml | 7 ++++ tests/secretxml2xmltest.c | 2 +- 10 files changed, 78 insertions(+), 23 deletions(-) delete mode 100644 tests/secretxml2xmlin/usage-passphrase.xml create mode 100644 tests/secretxml2xmlin/usage-tls.xml diff --git a/docs/aclpolkit.html.in b/docs/aclpolkit.html.in index 4d0307d..7a1e302 100644 --- a/docs/aclpolkit.html.in +++ b/docs/aclpolkit.html.in @@ -226,7 +226,7 @@ </tr> <tr> <td>secret_usage_name</td> - <td>Name of be associated passphrase secret, if any</td> + <td>Name of the associated tls secret, if any</td> </tr> </tbody> </table> diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 216a83c..de1af20 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -41,8 +41,9 @@ <dd> Specifies what this secret is used for. A mandatory <code>type</code> attribute specifies the usage category, currently - only <code>volume</code>, <code>ceph</code>, and <code>iscsi</code> - are defined. Specific usage categories are described below. + only <code>volume</code>, <code>ceph</code>, <code>iscsi</code>, + and <code>tls</code> are defined. Specific usage categories + are described below. </dd> </dl> @@ -271,5 +272,59 @@ </auth> </pre> + <h3><a name="tlsUsageType">Usage type "tls"</a></h3> + + <p> + This secret may be used in order to provide the passphrase for the + private key used to provide TLS credentials. + The <code><usage type='tls'></code> element must contain a + single <code>name</code> element that specifies a usage name + for the secret. + <span class="since">Since 2.1.0</span>. + The following is an example of the expected XML and processing to + define the secret: + </p> + + <pre> + # cat tls-secret.xml + <secret ephemeral='no' private='yes'> + <description>sample tls secret</description> + <usage type='tls'> + <name>TLS_example</name> + </usage> + </secret> + + # virsh secret-define tls-secret.xml + Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created + + # virsh secret-list + UUID Usage + ----------------------------------------------------------- + 718c71bd-67b5-4a2b-87ec-a24e8ca200dc tls TLS_example + # + + </pre> + + <p> + A secret may also be defined via the + <a href="html/libvirt-libvirt-secret.html#virSecretDefineXML"> + <code>virSecretDefineXML</code></a> API. + + Once the secret is defined, a secret value will need to be set. The + secret would be the passphrase used to access the TLS credentials. + The following is a simple example of using + <code>virsh secret-set-value</code> to set the secret value. The + <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> + <code>virSecretSetValue</code></a> API may also be used to set + a more secure secret without using printable/readable characters. + </p> + + <pre> + # MYSECRET=`printf %s "letmein" | base64` + # virsh secret-set-value 718c71bd-67b5-4a2b-87ec-a24e8ca200dc $MYSECRET + Secret value set + + </pre> + </body> </html> diff --git a/docs/schemas/secret.rng b/docs/schemas/secret.rng index cac8560..1e94d66 100644 --- a/docs/schemas/secret.rng +++ b/docs/schemas/secret.rng @@ -36,7 +36,7 @@ <ref name='usagevolume'/> <ref name='usageceph'/> <ref name='usageiscsi'/> - <ref name='usagepassphrase'/> + <ref name='usagetls'/> <!-- More choices later --> </choice> </element> @@ -72,9 +72,9 @@ </element> </define> - <define name='usagepassphrase'> + <define name='usagetls'> <attribute name='type'> - <value>passphrase</value> + <value>tls</value> </attribute> <element name='name'> <ref name='genericName'/> diff --git a/include/libvirt/libvirt-secret.h b/include/libvirt/libvirt-secret.h index 55b11e0..2ae36f6 100644 --- a/include/libvirt/libvirt-secret.h +++ b/include/libvirt/libvirt-secret.h @@ -43,7 +43,7 @@ typedef enum { VIR_SECRET_USAGE_TYPE_VOLUME = 1, VIR_SECRET_USAGE_TYPE_CEPH = 2, VIR_SECRET_USAGE_TYPE_ISCSI = 3, - VIR_SECRET_USAGE_TYPE_PASSPHRASE = 4, + VIR_SECRET_USAGE_TYPE_TLS = 4, # ifdef VIR_ENUM_SENTINELS VIR_SECRET_USAGE_TYPE_LAST diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index 99b867f..67f2a57 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -338,7 +338,7 @@ virAccessDriverPolkitCheckSecret(virAccessManagerPtr manager, virAccessPermSecretTypeToString(perm), attrs); } break; - case VIR_SECRET_USAGE_TYPE_PASSPHRASE: { + case VIR_SECRET_USAGE_TYPE_TLS: { const char *attrs[] = { "connect_driver", driverName, "secret_uuid", uuidstr, diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index a973aa9..ce1ad92 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -39,7 +39,7 @@ VIR_LOG_INIT("conf.secret_conf"); VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, - "none", "volume", "ceph", "iscsi", "passphrase") + "none", "volume", "ceph", "iscsi", "tls") const char * virSecretUsageIDForDef(virSecretDefPtr def) @@ -57,7 +57,7 @@ virSecretUsageIDForDef(virSecretDefPtr def) case VIR_SECRET_USAGE_TYPE_ISCSI: return def->usage.target; - case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + case VIR_SECRET_USAGE_TYPE_TLS: return def->usage.name; default: @@ -89,7 +89,7 @@ virSecretDefFree(virSecretDefPtr def) VIR_FREE(def->usage.target); break; - case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + case VIR_SECRET_USAGE_TYPE_TLS: VIR_FREE(def->usage.name); break; @@ -153,10 +153,10 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, } break; - case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + case VIR_SECRET_USAGE_TYPE_TLS: if (!(def->usage.name = virXPathString("string(./usage/name)", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("passphrase usage specified, but name is missing")); + _("tls usage specified, but name is missing")); return -1; } break; @@ -313,7 +313,7 @@ virSecretDefFormatUsage(virBufferPtr buf, virBufferEscapeString(buf, "<target>%s</target>\n", def->usage.target); break; - case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + case VIR_SECRET_USAGE_TYPE_TLS: virBufferEscapeString(buf, "<name>%s</name>\n", def->usage.name); break; diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 6714a00..2bdfe08 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -238,7 +238,7 @@ virSecretObjSearchName(const void *payload, found = 1; break; - case VIR_SECRET_USAGE_TYPE_PASSPHRASE: + case VIR_SECRET_USAGE_TYPE_TLS: if (STREQ(secret->def->usage.name, data->usageID)) found = 1; break; diff --git a/tests/secretxml2xmlin/usage-passphrase.xml b/tests/secretxml2xmlin/usage-passphrase.xml deleted file mode 100644 index 2b94b80..0000000 --- a/tests/secretxml2xmlin/usage-passphrase.xml +++ /dev/null @@ -1,7 +0,0 @@ -<secret ephemeral='no' private='no'> - <uuid>f52a81b2-424e-490c-823d-6bd4235bc572</uuid> - <description>Sample Passphrase Secret</description> - <usage type='passphrase'> - <name>mumblyfratz</name> - </usage> -</secret> diff --git a/tests/secretxml2xmlin/usage-tls.xml b/tests/secretxml2xmlin/usage-tls.xml new file mode 100644 index 0000000..8203681 --- /dev/null +++ b/tests/secretxml2xmlin/usage-tls.xml @@ -0,0 +1,7 @@ +<secret ephemeral='no' private='no'> + <uuid>f52a81b2-424e-490c-823d-6bd4235bc572</uuid> + <description>Sample TLS Secret</description> + <usage type='tls'> + <name>TLS-Example</name> + </usage> +</secret> diff --git a/tests/secretxml2xmltest.c b/tests/secretxml2xmltest.c index c444e4d..714c709 100644 --- a/tests/secretxml2xmltest.c +++ b/tests/secretxml2xmltest.c @@ -80,7 +80,7 @@ mymain(void) DO_TEST("usage-volume"); DO_TEST("usage-ceph"); DO_TEST("usage-iscsi"); - DO_TEST("usage-passphrase"); + DO_TEST("usage-tls"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.5.5

On Mon, Jul 11, 2016 at 02:07:54PM -0400, John Ferlan wrote:
Commit id 'c84380106' added support for a secret usage type 'passphrase' that was designed to be used for both LUKS encryption and TLS credentials since both used a 'simple' passphrase in order to handle the authentication. However, a post commit review deemed that usage model to be invalid.
This patch repurposes the 'passphrase' usage type for 'tls' specific usage. A previous patch has already adjusted the various LUKS usages to utilize a 'volume' secret.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/aclpolkit.html.in | 2 +- docs/formatsecret.html.in | 59 +++++++++++++++++++++++++++++- docs/schemas/secret.rng | 6 +-- include/libvirt/libvirt-secret.h | 2 +- src/access/viraccessdriverpolkit.c | 2 +- src/conf/secret_conf.c | 12 +++--- src/conf/virsecretobj.c | 2 +- tests/secretxml2xmlin/usage-passphrase.xml | 7 ---- tests/secretxml2xmlin/usage-tls.xml | 7 ++++ tests/secretxml2xmltest.c | 2 +- 10 files changed, 78 insertions(+), 23 deletions(-) delete mode 100644 tests/secretxml2xmlin/usage-passphrase.xml create mode 100644 tests/secretxml2xmlin/usage-tls.xml
This should be two separate patches. [A] Removing usage type='passphrase' (possibly merged into 2/7), pushed before the next release, to avoid ever releasing it. [B] would only add the TLS usage and be a part of the TLS series. It would basically consist of this patch [3/7] and a revert of [A], so splitting them out should not be much pain. [B] should be pushed along with the rest of TLS functionality, which might or might not end up in this release. Jan

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1301021 If the volume xml was looking to create a luks volume take the necessary steps in order to make that happen. The processing will be: 1. create a temporary file (virStorageBackendCreateQemuImgSecretPath) 1a. use the storage driver state dir path that uses the pool and volume name as a base. 2. create a secret object (virStorageBackendCreateQemuImgSecretObject) 2a. use an alias combinding the volume name and "_luks0" 2b. add the file to the object 3. create/add luks options to the commandline (virQEMUBuildLuksOpts) 3a. at the very least a "key-secret=%s" using the secret object alias 3b. if found in the XML the various "cipher" and "ivgen" options Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 218 ++++++++++++++++++++++++++++++++++++++--- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 +++++ src/util/virqemu.h | 6 ++ tests/storagevolxml2argvtest.c | 3 +- 6 files changed, 240 insertions(+), 14 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ad0af76..7066e91 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2180,6 +2180,7 @@ virProcessWait; # util/virqemu.h +virQEMUBuildLuksOpts; virQEMUBuildObjectCommandlineFromJSON; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 97f6ffe..1acee0d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -55,11 +55,14 @@ #include "viralloc.h" #include "internal.h" #include "secret_conf.h" +#include "secret_util.h" #include "viruuid.h" #include "virstoragefile.h" #include "storage_backend.h" #include "virlog.h" #include "virfile.h" +#include "virjson.h" +#include "virqemu.h" #include "stat-time.h" #include "virstring.h" #include "virxml.h" @@ -907,6 +910,7 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) return ret; } + static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { @@ -925,6 +929,10 @@ virStorageBackendQEMUImgBackingFormat(const char *qemuimg) return ret; } +/* The _virStorageBackendQemuImgInfo separates the command line building from + * the volume definition so that qemuDomainSnapshotCreateInactiveExternal can + * use it without needing to deal with a volume. + */ struct _virStorageBackendQemuImgInfo { int format; const char *path; @@ -941,21 +949,36 @@ struct _virStorageBackendQemuImgInfo { const char *inputPath; const char *inputFormatStr; int inputFormat; + + char *secretAlias; + const char *secretPath; }; + static int -virStorageBackendCreateQemuImgOpts(char **opts, +virStorageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr enc, + char **opts, struct _virStorageBackendQemuImgInfo info) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (info.backingPath) - virBufferAsprintf(&buf, "backing_fmt=%s,", - virStorageFileFormatTypeToString(info.backingFormat)); - if (info.encryption) - virBufferAddLit(&buf, "encryption=on,"); - if (info.preallocate) - virBufferAddLit(&buf, "preallocation=metadata,"); + if (info.format == VIR_STORAGE_FILE_LUKS) { + if (!enc) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing luks encryption information")); + goto error; + } + virQEMUBuildLuksOpts(&buf, enc, info.secretAlias); + } else { + if (info.backingPath) + virBufferAsprintf(&buf, "backing_fmt=%s,", + virStorageFileFormatTypeToString(info.backingFormat)); + if (info.encryption) + virBufferAddLit(&buf, "encryption=on,"); + if (info.preallocate) + virBufferAddLit(&buf, "preallocation=metadata,"); + } + if (info.nocow) virBufferAddLit(&buf, "nocow=on,"); @@ -1025,6 +1048,22 @@ virStorageBackendCreateQemuImgCheckEncryption(int format, if (virStorageGenerateQcowEncryption(conn, vol) < 0) return -1; } + } else if (format == VIR_STORAGE_FILE_LUKS) { + if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported volume encryption format %d"), + vol->target.encryption->format); + return -1; + } + if (enc->nsecrets > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("too many secrets for luks encryption")); + return -1; + } + if (enc->nsecrets == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no secret provided for luks encryption")); + } } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("volume encryption unsupported with format %s"), type); @@ -1069,6 +1108,12 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL; + if (info->format == VIR_STORAGE_FILE_LUKS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot set backing store for luks volume")); + return -1; + } + info->backingFormat = vol->target.backingStore->format; info->backingPath = vol->target.backingStore->path; @@ -1122,6 +1167,7 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, static int virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, int imgformat, + virStorageEncryptionInfoDefPtr enc, struct _virStorageBackendQemuImgInfo info) { char *opts = NULL; @@ -1130,7 +1176,7 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) info.compat = "0.10"; - if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) + if (virStorageBackendCreateQemuImgOpts(enc, &opts, info) < 0) return -1; if (opts) virCommandAddArgList(cmd, "-o", opts, NULL); @@ -1140,6 +1186,43 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, } +/* Add a secret object to the command line: + * --object secret,id=$secretAlias,file=$secretPath + * + * NB: format=raw is assumed + */ +static int +virStorageBackendCreateQemuImgSecretObject(virCommandPtr cmd, + virStorageVolDefPtr vol, + struct _virStorageBackendQemuImgInfo *info) +{ + char *str = NULL; + virJSONValuePtr props = NULL; + char *commandStr = NULL; + + if (virAsprintf(&info->secretAlias, "%s_luks0", vol->name) < 0) { + VIR_FREE(str); + return -1; + } + VIR_FREE(str); + + if (virJSONValueObjectCreate(&props, "s:file", info->secretPath, NULL) < 0) + return -1; + + if (!(commandStr = virQEMUBuildObjectCommandlineFromJSON("secret", + info->secretAlias, + props))) { + virJSONValueFree(props); + return -1; + } + virJSONValueFree(props); + + virCommandAddArgList(cmd, "--object", commandStr, NULL); + + return 0; +} + + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat */ @@ -1150,7 +1233,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags, const char *create_tool, - int imgformat) + int imgformat, + const char *secretPath) { virCommandPtr cmd = NULL; const char *type; @@ -1162,7 +1246,10 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .compat = vol->target.compat, .features = vol->target.features, .nocow = vol->target.nocow, + .secretPath = secretPath, + .secretAlias = NULL, }; + virStorageEncryptionInfoDefPtr enc = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -1192,6 +1279,18 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } + if (info.format == VIR_STORAGE_FILE_LUKS) { + if (inputvol) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot use inputvol with luks volume")); + return NULL; + } + if (!info.encryption) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing encryption description")); + return NULL; + } + } if (inputvol && virStorageBackendCreateQemuImgSetInput(inputvol, &info) < 0) @@ -1226,10 +1325,22 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, if (info.backingPath) virCommandAddArgList(cmd, "-b", info.backingPath, NULL); - if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat, info) < 0) { + if (info.format == VIR_STORAGE_FILE_LUKS) { + if (virStorageBackendCreateQemuImgSecretObject(cmd, vol, &info) < 0) { + VIR_FREE(info.secretAlias); + virCommandFree(cmd); + return NULL; + } + enc = &vol->target.encryption->encinfo; + } + + if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat, + enc, info) < 0) { + VIR_FREE(info.secretAlias); virCommandFree(cmd); return NULL; } + VIR_FREE(info.secretAlias); if (info.inputPath) virCommandAddArg(cmd, info.inputPath); @@ -1240,6 +1351,77 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return cmd; } + +static char * +virStorageBackendCreateQemuImgSecretPath(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + virStorageEncryptionPtr enc = vol->target.encryption; + char *secretPath = NULL; + int fd = -1; + uint8_t *secret = NULL; + size_t secretlen = 0; + + if (!enc) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing encryption description")); + return NULL; + } + + if (!conn || !conn->secretDriver || + !conn->secretDriver->secretLookupByUUID || + !conn->secretDriver->secretLookupByUsage || + !conn->secretDriver->secretGetValue) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to look up encryption secret")); + return NULL; + } + + if (!(secretPath = virStoragePoolObjBuildTempFilePath(pool, vol))) + goto cleanup; + + if ((fd = mkostemp(secretPath, O_CLOEXEC)) < 0) { + virReportSystemError(errno, "%s", + _("failed to open luks secret file for write")); + goto error; + } + + if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef, + VIR_SECRET_USAGE_TYPE_VOLUME, + &secret, &secretlen) < 0) + goto error; + + if (safewrite(fd, secret, secretlen) < 0) { + virReportSystemError(errno, "%s", + _("failed to write luks secret file")); + goto error; + } + VIR_FORCE_CLOSE(fd); + + if ((vol->target.perms->uid != (uid_t) -1) && + (vol->target.perms->gid != (gid_t) -1)) { + if (chown(secretPath, vol->target.perms->uid, + vol->target.perms->gid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to chown luks secret file")); + goto error; + } + } + + cleanup: + VIR_DISPOSE_N(secret, secretlen); + VIR_FORCE_CLOSE(fd); + + return secretPath; + + error: + unlink(secretPath); + VIR_FREE(secretPath); + goto cleanup; +} + + int virStorageBackendCreateQemuImg(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -1251,6 +1433,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, char *create_tool; int imgformat; virCommandPtr cmd; + char *secretPath = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); @@ -1266,8 +1449,15 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (imgformat < 0) goto cleanup; + if (vol->target.format == VIR_STORAGE_FILE_LUKS) { + if (!(secretPath = + virStorageBackendCreateQemuImgSecretPath(conn, pool, vol))) + goto cleanup; + } + cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, pool, vol, inputvol, - flags, create_tool, imgformat); + flags, create_tool, + imgformat, secretPath); if (!cmd) goto cleanup; @@ -1275,6 +1465,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, virCommandFree(cmd); cleanup: + if (secretPath) { + unlink(secretPath); + VIR_FREE(secretPath); + } VIR_FREE(create_tool); return ret; } diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 5bc622c..28e1a65 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -241,7 +241,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags, const char *create_tool, - int imgformat); + int imgformat, + const char *secretPath); /* ------- virStorageFile backends ------------ */ typedef struct _virStorageFileBackend virStorageFileBackend; diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 895168e..dd7a59f 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -140,3 +140,26 @@ virQEMUBuildObjectCommandlineFromJSON(const char *type, virBufferFreeAndReset(&buf); return ret; } + + +void +virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) +{ + virBufferAsprintf(buf, "key-secret=%s,", alias); + + /* If there's any cipher, then add that to the command line */ + if (enc->cipher_name) { + virBufferEscapeString(buf, "cipher-alg=%s-", enc->cipher_name); + virBufferAsprintf(buf, "%u,", enc->cipher_size); + if (enc->cipher_mode) + virBufferEscapeString(buf, "cipher-mode=%s,", enc->cipher_mode); + if (enc->cipher_hash) + virBufferEscapeString(buf, "hash-alg=%s,", enc->cipher_hash); + if (enc->ivgen_name) + virBufferEscapeString(buf, "ivgen-alg=%s,", enc->ivgen_name); + if (enc->ivgen_hash) + virBufferEscapeString(buf, "ivgen-hash-alg=%s,", enc->ivgen_hash); + } +} diff --git a/src/util/virqemu.h b/src/util/virqemu.h index af4ec1d..46e7ae2 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -26,9 +26,15 @@ # include "internal.h" # include "virjson.h" +# include "virstorageencryption.h" char *virQEMUBuildObjectCommandlineFromJSON(const char *type, const char *alias, virJSONValuePtr props); +void virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + #endif /* __VIR_QEMU_H_ */ diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index ccfe9ab..e300821 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -83,7 +83,8 @@ testCompareXMLToArgvFiles(bool shouldFail, cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, &poolobj, vol, inputvol, flags, - create_tool, imgformat); + create_tool, imgformat, + NULL); if (!cmd) { if (shouldFail) { virResetLastError(); -- 2.5.5

On Mon, Jul 11, 2016 at 02:07:55PM -0400, John Ferlan wrote:
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1301021
If the volume xml was looking to create a luks volume take the necessary steps in order to make that happen.
The processing will be: 1. create a temporary file (virStorageBackendCreateQemuImgSecretPath) 1a. use the storage driver state dir path that uses the pool and volume name as a base.
2. create a secret object (virStorageBackendCreateQemuImgSecretObject) 2a. use an alias combinding the volume name and "_luks0" 2b. add the file to the object
3. create/add luks options to the commandline (virQEMUBuildLuksOpts) 3a. at the very least a "key-secret=%s" using the secret object alias 3b. if found in the XML the various "cipher" and "ivgen" options
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 218 ++++++++++++++++++++++++++++++++++++++--- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 +++++ src/util/virqemu.h | 6 ++ tests/storagevolxml2argvtest.c | 3 +- 6 files changed, 240 insertions(+), 14 deletions(-)
@@ -1140,6 +1186,43 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, }
+/* Add a secret object to the command line: + * --object secret,id=$secretAlias,file=$secretPath + * + * NB: format=raw is assumed + */ +static int +virStorageBackendCreateQemuImgSecretObject(virCommandPtr cmd, + virStorageVolDefPtr vol, + struct _virStorageBackendQemuImgInfo *info) +{ + char *str = NULL;
This variable is unused.
+ virJSONValuePtr props = NULL; + char *commandStr = NULL; +
+ if (virAsprintf(&info->secretAlias, "%s_luks0", vol->name) < 0) { + VIR_FREE(str); + return -1; + } + VIR_FREE(str); + + if (virJSONValueObjectCreate(&props, "s:file", info->secretPath, NULL) < 0) + return -1; + + if (!(commandStr = virQEMUBuildObjectCommandlineFromJSON("secret", + info->secretAlias, + props))) { + virJSONValueFree(props); + return -1; + } + virJSONValueFree(props); +
So, this will generate: --object secret,id=volume_luks0,file=/path/to/tmp/luksfile Since we only have one property and one alias here, there is no need to go through JSON, it can be just a single virCommandAddArgFormat call. (or we need to go through a virBuffer to use qemuBufferEscapeComma in case we allow commas in storage pool names).
+ virCommandAddArgList(cmd, "--object", commandStr, NULL); + + return 0; +} + + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat */ @@ -1150,7 +1233,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags, const char *create_tool, - int imgformat) + int imgformat, + const char *secretPath) { virCommandPtr cmd = NULL; const char *type; @@ -1162,7 +1246,10 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .compat = vol->target.compat, .features = vol->target.features, .nocow = vol->target.nocow, + .secretPath = secretPath, + .secretAlias = NULL,
Since we only ever give one secret on the command line, this can be a static string.
}; + virStorageEncryptionInfoDefPtr enc = NULL;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 895168e..dd7a59f 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -140,3 +140,26 @@ virQEMUBuildObjectCommandlineFromJSON(const char *type, virBufferFreeAndReset(&buf); return ret; } + + +void +virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) +{ + virBufferAsprintf(buf, "key-secret=%s,", alias); + + /* If there's any cipher, then add that to the command line */
+ if (enc->cipher_name) { + virBufferEscapeString(buf, "cipher-alg=%s-", enc->cipher_name); + virBufferAsprintf(buf, "%u,", enc->cipher_size); + if (enc->cipher_mode) + virBufferEscapeString(buf, "cipher-mode=%s,", enc->cipher_mode); + if (enc->cipher_hash) + virBufferEscapeString(buf, "hash-alg=%s,", enc->cipher_hash); + if (enc->ivgen_name) + virBufferEscapeString(buf, "ivgen-alg=%s,", enc->ivgen_name); + if (enc->ivgen_hash) + virBufferEscapeString(buf, "ivgen-hash-alg=%s,", enc->ivgen_hash);
s/virBufferEscapeString/qemuBufferEscapeComma/ This is QEMU command line, not XML. Also, both of the functions are no-ops if the string is NULL, so the ifs are not necessary. ACK with that fixed and the unused 'str' variable removed. Jan

[...]
+ +void +virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) +{ + virBufferAsprintf(buf, "key-secret=%s,", alias); + + /* If there's any cipher, then add that to the command line */
+ if (enc->cipher_name) { + virBufferEscapeString(buf, "cipher-alg=%s-", enc->cipher_name); + virBufferAsprintf(buf, "%u,", enc->cipher_size); + if (enc->cipher_mode) + virBufferEscapeString(buf, "cipher-mode=%s,", enc->cipher_mode); + if (enc->cipher_hash) + virBufferEscapeString(buf, "hash-alg=%s,", enc->cipher_hash); + if (enc->ivgen_name) + virBufferEscapeString(buf, "ivgen-alg=%s,", enc->ivgen_name); + if (enc->ivgen_hash) + virBufferEscapeString(buf, "ivgen-hash-alg=%s,", enc->ivgen_hash);
s/virBufferEscapeString/qemuBufferEscapeComma/
Not sure I understand what this is referencing.... Besides qemuBufferEscapeComma is static to qemu_command John
This is QEMU command line, not XML. Also, both of the functions are no-ops if the string is NULL, so the ifs are not necessary.
ACK with that fixed and the unused 'str' variable removed.
Jan

On Thu, Jul 14, 2016 at 16:55:01 -0400, John Ferlan wrote:
[...]
+ +void +virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) +{ + virBufferAsprintf(buf, "key-secret=%s,", alias); + + /* If there's any cipher, then add that to the command line */
+ if (enc->cipher_name) { + virBufferEscapeString(buf, "cipher-alg=%s-", enc->cipher_name); + virBufferAsprintf(buf, "%u,", enc->cipher_size); + if (enc->cipher_mode) + virBufferEscapeString(buf, "cipher-mode=%s,", enc->cipher_mode); + if (enc->cipher_hash) + virBufferEscapeString(buf, "hash-alg=%s,", enc->cipher_hash); + if (enc->ivgen_name) + virBufferEscapeString(buf, "ivgen-alg=%s,", enc->ivgen_name); + if (enc->ivgen_hash) + virBufferEscapeString(buf, "ivgen-hash-alg=%s,", enc->ivgen_hash);
s/virBufferEscapeString/qemuBufferEscapeComma/
Not sure I understand what this is referencing.... Besides
I'd guess that it doesn't make much sense to escape < to < and > to > in code that puts stuff on the command line rather to an XML and that it makes more sense to escape a comma in the strings with two commas as is usual for qemu command lines.
qemuBufferEscapeComma is static to qemu_command
Extracting it to src/util/virbuffer.c could help.

On 07/15/2016 03:17 AM, Peter Krempa wrote:
On Thu, Jul 14, 2016 at 16:55:01 -0400, John Ferlan wrote:
[...]
+ +void +virQEMUBuildLuksOpts(virBufferPtr buf, + virStorageEncryptionInfoDefPtr enc, + const char *alias) +{ + virBufferAsprintf(buf, "key-secret=%s,", alias); + + /* If there's any cipher, then add that to the command line */
+ if (enc->cipher_name) { + virBufferEscapeString(buf, "cipher-alg=%s-", enc->cipher_name); + virBufferAsprintf(buf, "%u,", enc->cipher_size); + if (enc->cipher_mode) + virBufferEscapeString(buf, "cipher-mode=%s,", enc->cipher_mode); + if (enc->cipher_hash) + virBufferEscapeString(buf, "hash-alg=%s,", enc->cipher_hash); + if (enc->ivgen_name) + virBufferEscapeString(buf, "ivgen-alg=%s,", enc->ivgen_name); + if (enc->ivgen_hash) + virBufferEscapeString(buf, "ivgen-hash-alg=%s,", enc->ivgen_hash);
s/virBufferEscapeString/qemuBufferEscapeComma/
Not sure I understand what this is referencing.... Besides
I'd guess that it doesn't make much sense to escape < to < and > to > in code that puts stuff on the command line rather to an XML and that it makes more sense to escape a comma in the strings with two commas as is usual for qemu command lines.
qemuBufferEscapeComma is static to qemu_command
Extracting it to src/util/virbuffer.c could help.
Since it's QEMU specific I chose to put it in virqemu.c (there's patches I posted today...) I have to say, the result is rather ugly... So these have gone from : if (enc->cipher_name) { virBufferAsprintf(buf, "cipher-alg=%s-%u,", enc->cipher_name, enc->cipher_size); if (enc->cipher_mode) virBufferAsprintf(buf, "cipher-mode=%s,", enc->cipher_mode); if (enc->cipher_hash) virBufferAsprintf(buf, "hash-alg=%s,", enc->cipher_hash); if (enc->ivgen_name) virBufferAsprintf(buf, "ivgen-alg=%s,", enc->ivgen_name); if (enc->ivgen_hash) virBufferAsprintf(buf, "ivgen-hash-alg=%s,", enc->ivgen_hash); } to (assuming patches I've posted recently are accepted): if (!enc->cipher_name) return; virBufferAddLit(buf, "cipher-alg="); virQEMUBuildBufferEscapeComma(buf, enc->cipher_name); virBufferAsprintf(buf, "-%u,", enc->cipher_size); if (enc->cipher_mode) { virBufferAddLit(buf, "cipher-mode="); virQEMUBuildBufferEscapeComma(buf, enc->cipher_mode); virBufferAddLit(buf, ","); } if (enc->cipher_hash) { virBufferAddLit(buf, "hash-alg="); virQEMUBuildBufferEscapeComma(buf, enc->cipher_hash); virBufferAddLit(buf, ","); } if (!enc->ivgen_name) return; virBufferAddLit(buf, "cipher-name="); virQEMUBuildBufferEscapeComma(buf, enc->ivgen_name); virBufferAddLit(buf, ","); if (enc->ivgen_hash) { virBufferAddLit(buf, "ivgen-hash-alg="); virQEMUBuildBufferEscapeComma(buf, enc->ivgen_hash); virBufferAddLit(buf, ","); } All because someone could add a "," to one of those names... John

Commit id 'a1344f70a' added AES secret processing for RBD when starting up a guest. As such, when the hotplug code calls qemuDomainSecretDiskPrepare an AES secret could be added to the disk about to be hotplugged. If an AES secret was added, then the hotplug code would need to generate the secret object because qemuBuildDriveStr would add the "password-secret=" to the returned 'driveStr' rather than the base64 encoded password. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 093aaf9..004d6e3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -303,13 +303,16 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - virErrorPtr orig_err; + virErrorPtr orig_err = NULL; char *devstr = NULL; char *drivestr = NULL; char *drivealias = NULL; bool releaseaddr = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); + virJSONValuePtr secobjProps = NULL; + qemuDomainDiskPrivatePtr diskPriv; + qemuDomainSecretInfoPtr secinfo; if (!disk->info.type) { if (qemuDomainMachineIsS390CCW(vm->def) && @@ -342,6 +345,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + secinfo = diskPriv->secinfo; + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) + goto error; + } + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; @@ -354,9 +364,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error; - /* Attach the device - 2 step process */ qemuDomainObjEnterMonitor(driver, vm); + if (secobjProps && qemuMonitorAddObject(priv->mon, "secret", + secinfo->s.aes.alias, + secobjProps) < 0) + goto exit_monitor; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -368,12 +382,18 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto failexitmonitor; } + /* qemuMonitorAddObject consumes the props, but we need to wait + * for successful exit from monitor to clear; otherwise, error + * paths wouldn't clean up properly */ + secobjProps = NULL; + virDomainAuditDisk(vm, NULL, disk->src, "attach", true); virDomainDiskInsertPreAlloced(vm->def, disk); ret = 0; cleanup: + virJSONValueFree(secobjProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); @@ -387,14 +407,23 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); } + + failadddrive: + if (secobjProps) { + if (!orig_err) + orig_err = virSaveLastError(); + ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); + } + if (orig_err) { virSetError(orig_err); virFreeError(orig_err); } - failadddrive: + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; + secobjProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -2808,6 +2837,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, const char *src = virDomainDiskGetSource(disk); qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr; + char *objAlias = NULL; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -2818,7 +2848,24 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) return -1; + /* Let's look for some markers for a secret object and create an alias + * object to be used to attempt to delete the object that was created */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && + qemuDomainSecretDiskCapable(disk->src)) { + + if (!(objAlias = qemuDomainGetSecretAESAlias(disk->info.alias))) { + VIR_FREE(drivestr); + return -1; + } + } + qemuDomainObjEnterMonitor(driver, vm); + + /* If it fails, then so be it - it was a best shot */ + if (objAlias) + ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); + VIR_FREE(objAlias); + qemuMonitorDriveDel(priv->mon, drivestr); VIR_FREE(drivestr); if (qemuDomainObjExitMonitor(driver, vm) < 0) -- 2.5.5

On Mon, Jul 11, 2016 at 02:07:56PM -0400, John Ferlan wrote:
Commit id 'a1344f70a' added AES secret processing for RBD when starting up a guest. As such, when the hotplug code calls qemuDomainSecretDiskPrepare an AES secret could be added to the disk about to be hotplugged. If an AES secret was added, then the hotplug code would need to generate the secret object because qemuBuildDriveStr would add the "password-secret=" to the returned 'driveStr' rather than the base64 encoded password.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 093aaf9..004d6e3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -303,13 +303,16 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - virErrorPtr orig_err; + virErrorPtr orig_err = NULL; char *devstr = NULL; char *drivestr = NULL; char *drivealias = NULL; bool releaseaddr = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); + virJSONValuePtr secobjProps = NULL; + qemuDomainDiskPrivatePtr diskPriv; + qemuDomainSecretInfoPtr secinfo;
if (!disk->info.type) { if (qemuDomainMachineIsS390CCW(vm->def) && @@ -342,6 +345,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error;
+ diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + secinfo = diskPriv->secinfo; + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) + goto error; + } + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error;
@@ -354,9 +364,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error;
- /* Attach the device - 2 step process */ qemuDomainObjEnterMonitor(driver, vm);
+ if (secobjProps && qemuMonitorAddObject(priv->mon, "secret", + secinfo->s.aes.alias, + secobjProps) < 0) + goto exit_monitor; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive;
@@ -368,12 +382,18 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto failexitmonitor; }
+ /* qemuMonitorAddObject consumes the props, but we need to wait + * for successful exit from monitor to clear; otherwise, error + * paths wouldn't clean up properly */ + secobjProps = NULL; +
I would rather set it to NULL right after the AddObject call and track whether we need to remove the object in a separate variable. This way when qemuDomainObjExitMonitor fails, we jump to 'failexitmonitor' without setting it to NULL and free it again.
virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
virDomainDiskInsertPreAlloced(vm->def, disk); ret = 0;
cleanup: + virJSONValueFree(secobjProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); @@ -387,14 +407,23 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); } + + failadddrive: + if (secobjProps) {
+ if (!orig_err) + orig_err = virSaveLastError();
This will need to be repeated on every label. In hindsight, using a set of bools for rollback (like qemuDomainAttachHostUSBDevice does) might have been more readable than separate labels.
+ ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); + } + if (orig_err) { virSetError(orig_err); virFreeError(orig_err); }
- failadddrive: + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; + secobjProps = NULL; /* qemuMonitorAddObject consumes props on failure too */
failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -2808,6 +2837,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, const char *src = virDomainDiskGetSource(disk); qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr; + char *objAlias = NULL;
VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -2818,7 +2848,24 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) return -1;
+ /* Let's look for some markers for a secret object and create an alias + * object to be used to attempt to delete the object that was created */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && + qemuDomainSecretDiskCapable(disk->src)) { + + if (!(objAlias = qemuDomainGetSecretAESAlias(disk->info.alias))) {
Can't we access the disk private info here, to use the same conditions as we did for adding it? ACK with the double free fixed. Jan

[...]
qemuDomainObjEnterMonitor(driver, vm);
+ if (secobjProps && qemuMonitorAddObject(priv->mon, "secret", + secinfo->s.aes.alias, + secobjProps) < 0) + goto exit_monitor; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive;
@@ -368,12 +382,18 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto failexitmonitor; }
+ /* qemuMonitorAddObject consumes the props, but we need to wait + * for successful exit from monitor to clear; otherwise, error + * paths wouldn't clean up properly */ + secobjProps = NULL; +
I would rather set it to NULL right after the AddObject call and track whether we need to remove the object in a separate variable.
This way when qemuDomainObjExitMonitor fails, we jump to 'failexitmonitor' without setting it to NULL and free it again.
OK - this is fixed up in my local branch - awaiting ACK's for the cleanup series..
virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
virDomainDiskInsertPreAlloced(vm->def, disk); ret = 0;
cleanup: + virJSONValueFree(secobjProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); @@ -387,14 +407,23 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); } + + failadddrive: + if (secobjProps) {
+ if (!orig_err) + orig_err = virSaveLastError();
This will need to be repeated on every label. In hindsight, using a set of bools for rollback (like qemuDomainAttachHostUSBDevice does) might have been more readable than separate labels.
Well hindsight is 20/20 - so I posted another series dealing with the error path adjustments. Hopefully that works better and then this just is a simple addition... Already done in a local branch. The secobjProps processing would follow the similar mechanism as AttachRNG and AttachMemory w/r/t "rv = qemuMonitorAddObject()", clear props, if (rv < 0), goto monitor cleanup, and setting the cleanup boolean.
+ ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); + } + if (orig_err) { virSetError(orig_err); virFreeError(orig_err); }
- failadddrive: + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; + secobjProps = NULL; /* qemuMonitorAddObject consumes props on failure too */
failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -2808,6 +2837,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, const char *src = virDomainDiskGetSource(disk); qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr; + char *objAlias = NULL;
VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -2818,7 +2848,24 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) return -1;
+ /* Let's look for some markers for a secret object and create an alias + * object to be used to attempt to delete the object that was created */ + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && + qemuDomainSecretDiskCapable(disk->src)) { + + if (!(objAlias = qemuDomainGetSecretAESAlias(disk->info.alias))) {
Can't we access the disk private info here, to use the same conditions as we did for adding it?
Cut-n-paste from my response to the v3 review series http://www.redhat.com/archives/libvir-list/2016-June/msg01999.html: "From Peter's review comments of the previous series - no. In fact, the cleanup of qemuProcessLaunch was designed to remove the Secrets that we saved away temporarily so that they wouldn't be kept in memory forever (e.g call to qemuDomainSecretDiskDestroy). Hence why the Attach processing needs the qemuDomainSecretDiskPrepare (and also calls qemuDomainSecretDiskDestroy). Thus, the detach processing, then can only work off the alias to remove." But I could augment the comment a bit. I think libvirtd restart wouldn't have them either... John

Soon we will be adding luks encryption support. Since a volume could require both a luks secret and a secret to give to the server to use of the device, alter the alias generation to create a slightly different alias so that we don't have two objects with the same alias. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 10 ++++++++-- src/qemu/qemu_alias.h | 3 ++- src/qemu/qemu_domain.c | 17 ++++++++++------- src/qemu/qemu_hotplug.c | 3 ++- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index d624071..51a654a 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -485,13 +485,16 @@ qemuDomainGetMasterKeyAlias(void) /* qemuDomainGetSecretAESAlias: + * @srcalias: Source alias used to generate the secret alias + * @isLuks: True when we are generating a secret for LUKS encrypt/decrypt * * Generate and return an alias for the encrypted secret * * Returns NULL or a string containing the alias */ char * -qemuDomainGetSecretAESAlias(const char *srcalias) +qemuDomainGetSecretAESAlias(const char *srcalias, + bool isLuks) { char *alias; @@ -501,7 +504,10 @@ qemuDomainGetSecretAESAlias(const char *srcalias) return NULL; } - ignore_value(virAsprintf(&alias, "%s-secret0", srcalias)); + if (isLuks) + ignore_value(virAsprintf(&alias, "%s-luks-secret0", srcalias)); + else + ignore_value(virAsprintf(&alias, "%s-secret0", srcalias)); return alias; } diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index e328a9b..d1c6ba8 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -69,6 +69,7 @@ char *qemuAliasFromDisk(const virDomainDiskDef *disk); char *qemuDomainGetMasterKeyAlias(void); -char *qemuDomainGetSecretAESAlias(const char *srcalias); +char *qemuDomainGetSecretAESAlias(const char *srcalias, + bool isLuks); #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 286f096..d046633 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -895,6 +895,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, * @secretUsageType: The virSecretUsageType * @username: username to use for authentication (may be NULL) * @seclookupdef: Pointer to seclookupdef data + * @isLuks: True/False for is for luks (alias generation) * * Taking a secinfo, fill in the AES specific information using the * @@ -907,7 +908,8 @@ qemuDomainSecretAESSetup(virConnectPtr conn, const char *srcalias, virSecretUsageType secretUsageType, const char *username, - virSecretLookupTypeDefPtr seclookupdef) + virSecretLookupTypeDefPtr seclookupdef, + bool isLuks) { int ret = -1; uint8_t *raw_iv = NULL; @@ -921,7 +923,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, if (VIR_STRDUP(secinfo->s.aes.username, username) < 0) return -1; - if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias))) + if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias, isLuks))) return -1; /* Create a random initialization vector */ @@ -970,6 +972,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, * @secretUsageType: The virSecretUsageType * @username: username to use for authentication (may be NULL) * @seclookupdef: Pointer to seclookupdef data + * @isLuks: True when is luks (generates different alias) * * If we have the encryption API present and can support a secret object, then * build the AES secret; otherwise, build the Plain secret. This is the magic @@ -985,14 +988,15 @@ qemuDomainSecretSetup(virConnectPtr conn, const char *srcalias, virSecretUsageType secretUsageType, const char *username, - virSecretLookupTypeDefPtr seclookupdef) + virSecretLookupTypeDefPtr seclookupdef, + bool isLuks) { if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, secretUsageType, username, - seclookupdef) < 0) + seclookupdef, isLuks) < 0) return -1; } else { if (qemuDomainSecretPlainSetup(conn, secinfo, secretUsageType, @@ -1052,7 +1056,6 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, qemuDomainSecretInfoPtr secinfo = NULL; if (conn && qemuDomainSecretDiskCapable(src)) { - virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); @@ -1064,7 +1067,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, secretUsageType, src->auth->username, - &src->auth->seclookupdef) < 0) + &src->auth->seclookupdef, false) < 0) goto error; diskPriv->secinfo = secinfo; @@ -1131,7 +1134,7 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias, VIR_SECRET_USAGE_TYPE_ISCSI, iscsisrc->auth->username, - &iscsisrc->auth->seclookupdef) < 0) + &iscsisrc->auth->seclookupdef, false) < 0) goto error; hostdevPriv->secinfo = secinfo; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 004d6e3..376e6aa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2853,7 +2853,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && qemuDomainSecretDiskCapable(disk->src)) { - if (!(objAlias = qemuDomainGetSecretAESAlias(disk->info.alias))) { + if (!(objAlias = + qemuDomainGetSecretAESAlias(disk->info.alias, false))) { VIR_FREE(drivestr); return -1; } -- 2.5.5

On Mon, Jul 11, 2016 at 02:07:57PM -0400, John Ferlan wrote:
Soon we will be adding luks encryption support. Since a volume could require both a luks secret and a secret to give to the server to use of the device, alter the alias generation to create a slightly different alias so that we don't have two objects with the same alias.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 10 ++++++++-- src/qemu/qemu_alias.h | 3 ++- src/qemu/qemu_domain.c | 17 ++++++++++------- src/qemu/qemu_hotplug.c | 3 ++- 4 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index d624071..51a654a 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -485,13 +485,16 @@ qemuDomainGetMasterKeyAlias(void)
ACK Jan

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1301021 Generate the luks command line using the AES secret key to encrypt the luks secret. A luks secret object will be in addition to a an AES secret. For hotplug, check if the encinfo exists and if so, add the AES secret for the passphrase for the secret object used to decrypt the device. Modify/augment the fakeSecret* in qemuxml2argvtest in order to handle find a uuid or a volume usage with a specific path prefix in the XML (corresponds to the already generated XML tests). Add error message when the 'usageID' is not 'mycluster_myname'. Commit id '1d632c39' altered the error message generation to rely on the errors from the secret_driver (or it's faked replacement). Add the .args output for adding the LUKS disk to the domain Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 9 +++ src/qemu/qemu_domain.c | 25 +++++++- src/qemu/qemu_hotplug.c | 74 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 +++++++++++ tests/qemuxml2argvtest.c | 24 ++++++- 5 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c97d0f6..c356450 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1098,6 +1098,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus); if (idx < 0) { @@ -1237,6 +1238,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, secinfo->s.aes.alias); } + if (encinfo) + virQEMUBuildLuksOpts(&opt, &disk->src->encryption->encinfo, + encinfo->s.aes.alias); + if (disk->src->format > 0 && disk->src->type != VIR_STORAGE_TYPE_DIR) virBufferAsprintf(&opt, "format=%s,", @@ -1940,6 +1945,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk = def->disks[i]; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; /* PowerPC pseries based VMs do not support floppy device */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && @@ -1976,6 +1982,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0) return -1; + if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); if (!(optstr = qemuBuildDriveStr(disk, driveBoot, qemuCaps))) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d046633..ef7e3c3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -993,7 +993,8 @@ qemuDomainSecretSetup(virConnectPtr conn, { if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && - secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH) { + (secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH || + secretUsageType == VIR_SECRET_USAGE_TYPE_VOLUME)) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, secretUsageType, username, seclookupdef, isLuks) < 0) @@ -1053,11 +1054,14 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = NULL; - if (conn && qemuDomainSecretDiskCapable(src)) { + if (!conn) + return 0; + + if (qemuDomainSecretDiskCapable(src)) { virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); if (VIR_ALLOC(secinfo) < 0) return -1; @@ -1073,6 +1077,21 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, diskPriv->secinfo = secinfo; } + if (!virStorageSourceIsEmpty(src) && src->encryption && + src->format == VIR_STORAGE_FILE_LUKS) { + + if (VIR_ALLOC(secinfo) < 0) + return -1; + + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, + VIR_SECRET_USAGE_TYPE_VOLUME, NULL, + &src->encryption->secrets[0]->seclookupdef, + true) < 0) + goto error; + + diskPriv->encinfo = secinfo; + } + return 0; error: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 376e6aa..d8a9fee 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -311,8 +311,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; + virJSONValuePtr encProps = NULL; qemuDomainDiskPrivatePtr diskPriv; qemuDomainSecretInfoPtr secinfo; + qemuDomainSecretInfoPtr encinfo; if (!disk->info.type) { if (qemuDomainMachineIsS390CCW(vm->def) && @@ -352,6 +354,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; } + encinfo = diskPriv->encinfo; + if (encinfo && qemuBuildSecretInfoProps(encinfo, &encProps) < 0) + goto error; + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; @@ -371,6 +377,11 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, secobjProps) < 0) goto exit_monitor; + if (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto failaddencsecret; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -386,6 +397,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, * for successful exit from monitor to clear; otherwise, error * paths wouldn't clean up properly */ secobjProps = NULL; + encProps = NULL; virDomainAuditDisk(vm, NULL, disk->src, "attach", true); @@ -394,6 +406,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, cleanup: virJSONValueFree(secobjProps); + virJSONValueFree(encProps); qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); @@ -409,6 +422,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, } failadddrive: + if (encProps) { + if (!orig_err) + orig_err = virSaveLastError(); + ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + } + + failaddencsecret: if (secobjProps) { if (!orig_err) orig_err = virSaveLastError(); @@ -423,7 +443,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; - secobjProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ + /* qemuMonitorAddObject consumes props on failure too */ + secobjProps = NULL; + encProps = NULL; failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -575,10 +597,14 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, { size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err = NULL; char *drivestr = NULL; char *devstr = NULL; int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virJSONValuePtr encProps = NULL; + qemuDomainDiskPrivatePtr diskPriv; + qemuDomainSecretInfoPtr encinfo; if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -609,6 +635,11 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + encinfo = diskPriv->encinfo; + if (encinfo && qemuBuildSecretInfoProps(encinfo, &encProps) < 0) + goto error; + if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) goto error; @@ -618,9 +649,13 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error; - /* Attach the device - 2 step process */ qemuDomainObjEnterMonitor(driver, vm); + if (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto exit_monitor; + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive; @@ -630,6 +665,11 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto failexitmonitor; + /* qemuMonitorAddObject consumes the props, but we need to wait + * for successful exit from monitor to clear; otherwise, error + * paths wouldn't clean up properly */ + encProps = NULL; + virDomainAuditDisk(vm, NULL, disk->src, "attach", true); virDomainDiskInsertPreAlloced(vm->def, disk); @@ -647,7 +687,17 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr); failadddrive: + orig_err = virSaveLastError(); + if (encProps) + ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + exit_monitor: ignore_value(qemuDomainObjExitMonitor(driver, vm)); + encProps = NULL; /* qemuMonitorAddObject consumes props on failure too */ failexitmonitor: virDomainAuditDisk(vm, NULL, disk->src, "attach", false); @@ -2838,6 +2888,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr; char *objAlias = NULL; + char *encAlias = NULL; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -2860,6 +2911,20 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } + /* Similarly, if this is possible a device using LUKS encryption, we + * can remove the luks object password too + */ + if (!virStorageSourceIsEmpty(disk->src) && disk->src->encryption && + disk->src->format == VIR_STORAGE_FILE_LUKS) { + + if (!(encAlias = + qemuDomainGetSecretAESAlias(disk->info.alias, true))) { + VIR_FREE(objAlias); + VIR_FREE(drivestr); + return -1; + } + } + qemuDomainObjEnterMonitor(driver, vm); /* If it fails, then so be it - it was a best shot */ @@ -2867,6 +2932,11 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); VIR_FREE(objAlias); + /* If it fails, then so be it - it was a best shot */ + if (encAlias) + ignore_value(qemuMonitorDelObject(priv->mon, encAlias)); + VIR_FREE(encAlias); + qemuMonitorDriveDel(priv->mon, drivestr); VIR_FREE(drivestr); if (qemuDomainObjExitMonitor(driver, vm) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args new file mode 100644 index 0000000..270322f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args @@ -0,0 +1,36 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name encryptdisk \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-encryptdisk/master-key.aes \ +-M pc-i440fx-2.1 \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 496898a6-e6ff-f7c8-5dc2-3cf410945ee9 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-encryptdisk/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-object secret,id=virtio-disk0-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk,\ +key-secret=virtio-disk0-luks-secret0,format=luks,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object secret,id=virtio-disk1-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk2,\ +key-secret=virtio-disk1-luks-secret0,format=luks,if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2bfd1ca..500e78c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -49,12 +49,22 @@ fakeSecretGetValue(virSecretPtr obj ATTRIBUTE_UNUSED, static virSecretPtr fakeSecretLookupByUsage(virConnectPtr conn, - int usageType ATTRIBUTE_UNUSED, + int usageType, const char *usageID) { unsigned char uuid[VIR_UUID_BUFLEN]; - if (STRNEQ(usageID, "mycluster_myname")) + if (usageType == VIR_SECRET_USAGE_TYPE_VOLUME) { + if (!STRPREFIX(usageID, "/storage/guest_disks/")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "test provided invalid volume storage prefix '%s'", + usageID); + return NULL; + } + } else if (STRNEQ(usageID, "mycluster_myname")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "test provided incorrect usage '%s'", usageID); return NULL; + } if (virUUIDGenerate(uuid) < 0) return NULL; @@ -62,10 +72,17 @@ fakeSecretLookupByUsage(virConnectPtr conn, return virGetSecret(conn, uuid, usageType, usageID); } +static virSecretPtr +fakeSecretLookupByUUID(virConnectPtr conn, + const unsigned char *uuid) +{ + return virGetSecret(conn, uuid, 0, ""); +} + static virSecretDriver fakeSecretDriver = { .connectNumOfSecrets = NULL, .connectListSecrets = NULL, - .secretLookupByUUID = NULL, + .secretLookupByUUID = fakeSecretLookupByUUID, .secretLookupByUsage = fakeSecretLookupByUsage, .secretDefineXML = NULL, .secretGetXMLDesc = NULL, @@ -1345,6 +1362,7 @@ mymain(void) DO_TEST("encrypted-disk", NONE); DO_TEST("encrypted-disk-usage", NONE); + DO_TEST("luks-disks", QEMU_CAPS_OBJECT_SECRET); DO_TEST("memtune", NONE); DO_TEST("memtune-unlimited", NONE); -- 2.5.5

On Mon, Jul 11, 2016 at 02:07:58PM -0400, John Ferlan wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1301021
Generate the luks command line using the AES secret key to encrypt the luks secret. A luks secret object will be in addition to a an AES secret.
For hotplug, check if the encinfo exists and if so, add the AES secret for the passphrase for the secret object used to decrypt the device.
Modify/augment the fakeSecret* in qemuxml2argvtest in order to handle find a uuid or a volume usage with a specific path prefix in the XML (corresponds to the already generated XML tests). Add error message when the 'usageID' is not 'mycluster_myname'. Commit id '1d632c39' altered the error message generation to rely on the errors from the secret_driver (or it's faked replacement).
Add the .args output for adding the LUKS disk to the domain
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 9 +++ src/qemu/qemu_domain.c | 25 +++++++- src/qemu/qemu_hotplug.c | 74 +++++++++++++++++++++- .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 +++++++++++ tests/qemuxml2argvtest.c | 24 ++++++- 5 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c97d0f6..c356450 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1098,6 +1098,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus);
if (idx < 0) { @@ -1237,6 +1238,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, secinfo->s.aes.alias); }
+ if (encinfo) + virQEMUBuildLuksOpts(&opt, &disk->src->encryption->encinfo, + encinfo->s.aes.alias); + if (disk->src->format > 0 && disk->src->type != VIR_STORAGE_TYPE_DIR) virBufferAsprintf(&opt, "format=%s,", @@ -1940,6 +1945,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk = def->disks[i]; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
/* PowerPC pseries based VMs do not support floppy device */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && @@ -1976,6 +1982,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0) return -1;
+ if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0) + return -1; + virCommandAddArg(cmd, "-drive");
if (!(optstr = qemuBuildDriveStr(disk, driveBoot, qemuCaps))) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d046633..ef7e3c3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -993,7 +993,8 @@ qemuDomainSecretSetup(virConnectPtr conn, { if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && - secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH) { + (secretUsageType == VIR_SECRET_USAGE_TYPE_CEPH || + secretUsageType == VIR_SECRET_USAGE_TYPE_VOLUME)) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, secretUsageType, username, seclookupdef, isLuks) < 0) @@ -1053,11 +1054,14 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = NULL;
- if (conn && qemuDomainSecretDiskCapable(src)) { + if (!conn) + return 0; + + if (qemuDomainSecretDiskCapable(src)) { virSecretUsageType secretUsageType = VIR_SECRET_USAGE_TYPE_ISCSI; - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
if (VIR_ALLOC(secinfo) < 0) return -1; @@ -1073,6 +1077,21 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, diskPriv->secinfo = secinfo; }
+ if (!virStorageSourceIsEmpty(src) && src->encryption && + src->format == VIR_STORAGE_FILE_LUKS) { + + if (VIR_ALLOC(secinfo) < 0) + return -1; + + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, + VIR_SECRET_USAGE_TYPE_VOLUME, NULL, + &src->encryption->secrets[0]->seclookupdef, + true) < 0) + goto error; + + diskPriv->encinfo = secinfo; + } + return 0;
error: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 376e6aa..d8a9fee 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -311,8 +311,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; + virJSONValuePtr encProps = NULL; qemuDomainDiskPrivatePtr diskPriv; qemuDomainSecretInfoPtr secinfo; + qemuDomainSecretInfoPtr encinfo;
if (!disk->info.type) { if (qemuDomainMachineIsS390CCW(vm->def) && @@ -352,6 +354,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; }
+ encinfo = diskPriv->encinfo; + if (encinfo && qemuBuildSecretInfoProps(encinfo, &encProps) < 0) + goto error; + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error;
@@ -371,6 +377,11 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, secobjProps) < 0) goto exit_monitor;
+ if (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto failaddencsecret;
Naming the labels after what they do instead of where we came from makes the main body easier to read. The downside is that you don't know where you jumped from in the rollback section, but it should be simple enough not to need it. I suggest 'remove_secret' (and the next step would do 'remove_encryption_secret'), if we don't need the bool-based cleanup as I suggested in 5/7.
+ if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive;
@@ -386,6 +397,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, * for successful exit from monitor to clear; otherwise, error * paths wouldn't clean up properly */ secobjProps = NULL; + encProps = NULL;
Same comments as in 5/7 regarding the props stealing and *DiskDevice object removal conditions. ACK with the double frees on qemuDomainObjExitMonitor fixed. Jan

[...]
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 376e6aa..d8a9fee 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -311,8 +311,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; + virJSONValuePtr encProps = NULL; qemuDomainDiskPrivatePtr diskPriv; qemuDomainSecretInfoPtr secinfo; + qemuDomainSecretInfoPtr encinfo;
if (!disk->info.type) { if (qemuDomainMachineIsS390CCW(vm->def) && @@ -352,6 +354,10 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; }
+ encinfo = diskPriv->encinfo; + if (encinfo && qemuBuildSecretInfoProps(encinfo, &encProps) < 0) + goto error; + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error;
@@ -371,6 +377,11 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, secobjProps) < 0) goto exit_monitor;
+ if (encProps && qemuMonitorAddObject(priv->mon, "secret", + encinfo->s.aes.alias, + encProps) < 0) + goto failaddencsecret;
Naming the labels after what they do instead of where we came from makes the main body easier to read. The downside is that you don't know where you jumped from in the rollback section, but it should be simple enough not to need it.
I suggest 'remove_secret' (and the next step would do 'remove_encryption_secret'), if we don't need the bool-based cleanup as I suggested in 5/7.
This is now cleaner with the bool-based cleanup...
+ if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto failadddrive;
@@ -386,6 +397,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, * for successful exit from monitor to clear; otherwise, error * paths wouldn't clean up properly */ secobjProps = NULL; + encProps = NULL;
Same comments as in 5/7 regarding the props stealing and *DiskDevice object removal conditions.
Much cleaner now with bool-based cleanup John

On 07/11/2016 02:07 PM, John Ferlan wrote:
According to Dan's post commit response:
http://www.redhat.com/archives/libvir-list/2016-July/msg00088.html
to the v3 series:
http://www.redhat.com/archives/libvir-list/2016-June/msg01935.html
using a 'passphrase' usage is not desired, rather a 'volume' usage model should be used for LUKS.
So patches 1 & 2 make those alterations to already pushed docs and tests
Patch 3 then repurposes the 'passphrase' usage to a 'tls' usage type. I posted with this series since it removed the 'passphrase' usage and thus flushed out any errors in subsequent patches. I could hold off and repost it with the TLS changes that will also need to be made...
Patches 4-7 were reviewed previously and had been given what I took as provisional ACK's; however, I reposted the changes after the most recent review "just in case". Fortunately (I guess) I didn't push them along with the other changes. In any case, there are once again posted here - the primary difference between what's posted in this series vs. what was posted previously is the change to use a "volume" secret plus a tweak to the qemuxml2argvtest to fix some issues found while making the change.
John Ferlan (7): tests: Adjust LUKS tests to use 'volume' secret type docs: Update docs to reflect LUKS secret changes Repurpose the 'passphrase' secret to 'tls' storage: Add support to create a luks volume qemu: Add secinfo for hotplug virtio disk qemu: Alter the qemuDomainGetSecretAESAlias to add new arg qemu: Add luks support for domain disk
docs/aclpolkit.html.in | 2 +- docs/formatsecret.html.in | 81 +++++--- docs/formatstorage.html.in | 16 ++ docs/formatstorageencryption.html.in | 29 ++- docs/schemas/secret.rng | 6 +- include/libvirt/libvirt-secret.h | 2 +- src/access/viraccessdriverpolkit.c | 2 +- src/conf/secret_conf.c | 12 +- src/conf/virsecretobj.c | 2 +- src/libvirt_private.syms | 1 + src/qemu/qemu_alias.c | 10 +- src/qemu/qemu_alias.h | 3 +- src/qemu/qemu_command.c | 9 + src/qemu/qemu_domain.c | 40 +++- src/qemu/qemu_hotplug.c | 126 +++++++++++- src/storage/storage_backend.c | 218 +++++++++++++++++++-- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 +++ src/util/virqemu.h | 6 + .../qemuxml2argv-luks-disk-cipher.xml | 45 ----- .../qemuxml2argvdata/qemuxml2argv-luks-disks.args | 36 ++++ tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 2 +- tests/qemuxml2argvtest.c | 24 ++- .../qemuxml2xmlout-luks-disk-cipher.xml | 1 - tests/qemuxml2xmltest.c | 1 - tests/secretxml2xmlin/usage-passphrase.xml | 7 - tests/secretxml2xmlin/usage-tls.xml | 7 + tests/secretxml2xmltest.c | 2 +- tests/storagevolxml2argvtest.c | 3 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 33 files changed, 577 insertions(+), 150 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args delete mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml delete mode 100644 tests/secretxml2xmlin/usage-passphrase.xml create mode 100644 tests/secretxml2xmlin/usage-tls.xml
Based on the ACK's here and the changes already ACK'd/pushed for adjusting the hotplug error paths, I've made the appropriate alterations here as requested in code review and as a result of the hotplug changes and pushed this. Again, thanks for the persistence on this. John
participants (3)
-
John Ferlan
-
Ján Tomko
-
Peter Krempa