Re: [libvirt] [PATCH 2/2] qemu: Use secret objects to pass iSCSI passwords

On Tue, Sep 12, 2017 at 11:55:29 -0400, John Ferlan wrote: > > > On 09/12/2017 09:36 AM, Peter Krempa wrote: >> On Tue, Sep 05, 2017 at 15:09:35 -0400, John Ferlan wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1425757 >>> >>> The blockdev-add code provides a mechanism to sanely provide user >>> and password-secret arguments for iscsi without placing them on the >>> command line to be viewable by a 'ps -ef' type command or needing >>> to create separate -iscsi devices for each disk/volume found. >>> >>> So modify the iSCSI command line building to check for the presence >>> of the capability in order properly setup and use the domain master >>> secret object to encrypt the password in a secret object and alter >>> the parameters for the command line to utilize. >>> >>> Modify the xml2argvtest to exhibit the syntax for both disk and >>> hostdev configurations. >>> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_command.c | 19 ++++++++- >>> src/qemu/qemu_domain.c | 4 ++ >>> ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 39 ++++++++++++++++++ >>> ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++++++++ >>> ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 35 ++++++++++++++++ >>> ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++++++++++ >>> tests/qemuxml2argvtest.c | 10 +++++ >>> 7 files changed, 196 insertions(+), 2 deletions(-) >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args >>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml >> >> Most of the stuff here looks reasonable but I don't think we should mix >> the URI syntax with the file.param= syntax generated from the JSON >> objects. Since there's a capability when this is supported, the command >> line generator should use the new syntax. >> >> You can mark it in qemuDiskSourceNeedsProps so that it uses the new >> generator if it's needed and supported and implement the JSON generator. >> >> The rest should then work as expected. >> > > This is certainly where things didn't exactly match up the way I thought > you have desired the virstoragefile and virstoragetest code to work. > > In particular, it's the "user" and "password-secret" options that cause > the disconnect since they're a @disk private object and not a @disk->src > object. I considered a number of different ways to cheat, but came up > empty on each, so I just followed the existing RBD code. Well and that is a problem. We can either consider the disk authentication secrests to be the same for every single level of the backing chain. In such case the user and secret are correctly placed in the 'Disk' structure. In such case, still every single level of the backing chain will eventually need to access them so that the authentication can be done for every member. We can also allow specifying a different one per every level of the backing chain, which would make more sense in some configurations and allow more flexibility. In such case we need to move them to the virStorageSource structure so that every level can have individual authentication data. In the long term I think that solution 1 would bite us and we'd need to add individual authentication for the volumes anyways.

> One cannot reconstruct the <auth> element properly given that all the > arguments have is a username and an alias, but would need to have either > a "usage" or "uuid" string. The secret object doesn't contain that > either, so it'd need to be stored somehow. You mean from parsing of the backing chain? That won't be necessary.

> Perhaps if the @secinfo moved from private into source that would help, > although perhaps not following the RNG. Still there'd also have to be a We don't follow the RNG in internal structures. In some cases the RNG does not make sense, but the strucutres can be changed to make sense.

> way to save the string used in the original <auth> element used to look > up the secret (either by uuid or by usage). Having the password-secret > alias is OK, but really not useful. That is needed only for formatting of the command line (or monitor command) and for removing the correct secret on unplug. Most of the times you can infer it. We can as well as store it and not have to generate it all the time. That is an implementation detail though.

> Maybe "some future" adjustment could modify the password-secret alias to > be (for example) "virtio-disk0-%s-secret0" where the %s is either a UUID > or a Usage string of the secret used to generate the object. Perhaps > even the unsigned char UUID too so as to not have too many extra "-"'s > to parse/read. That'd be an awful alias, but serve a purpose as well. No, this is not necessary. We can store the data in the status XML. The alias (in password secret) is needed only to remove them in case of unplug and it's not really necessary to trace back which secret was used to populate it.

> A similar problem exists for RBD, but the RBD code in virstoragefile and > virstoragetest totally ignore the authentication pieces... That syntax > is a bit more hairly because it's only the RBD password-secret field > that needs the "file." (or not if -drive driver=rbd,... was supported). Ummm. The test does not need to deal with this necessarily. My issue is with the JSON structure generator, which should be fully used. And that generator is private to the qemu driver.