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

On 09/13/2017 03:08 AM, Peter Krempa wrote: > 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. > caveat: my knowledge of backing chain details is quite limited... Can different levels of the backing chain use different source servers? IOW: Assume the top level is: <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'> <host name='example.com' port='3260'/> </source> could some layer deeper then have: <source protocol='iscsi' name='iqn.2013-07.com.example.adifferent:iscsi-nopool/1'> <host name='adifferent.example.com' port='3260'/> </source>

? If so, then <auth> should be a child of <source> rather than <disk>. If not, then <auth> should stay as child of <disk>.

I would think it would be quite difficult to configure usage of multiple servers. Would that mean if one of the sources wasn't available or had some sort of failure, then the whole chain is invalid? The complexity is rather mind boggling, if not fragile...

>> 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. > I was thinking more about virstoragefile.c and virstoragetest.c. It's easy enough to fetch the user/password-secret in virStorageSourceParseBackingJSONiSCSI: + const char *user = virJSONValueObjectGetString(json, "user"); + const char *secret = virJSONValueObjectGetString(json, "password-secret"); But there's not much one can do with it as you get (for example): user = "myname" secret = "virtio-disk1-secret0" but an <auth> would look like: <auth username='myname'> <secret type='iscsi' usage='mycluster_myname'/> </auth> So to a degree it's not testable to build the XML as virstoragetest does. Sine the secret can be built up using the disk alias, it's perhaps not even worth storing away.

>> 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. > But we only support one <auth> per <disk>, so in order to be associated with <source>, the RNG would need to support that and the code would need to be modified such that <auth> is a child of <source>. We'd have to support "both" formats. If <auth> was a child of <disk>, then perhaps it only supports one-level depth. We could also read under <disk>, but move to under <source> on output if we were so inclined. Still that would then require every level of the backing chain to have it's own <auth> even if that is the same <auth> as the predecessor. I'm thinking and typing here, so I haven't thought through all the possibilities.

>> 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. > Again - I was thinking of the test environment that builds the XML from the JSON. I had actually started down this path originally, but got stuck at precisely that point. So I have code in a branch already that can do everything except the virstoragetest rebuild of <auth>. >> 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. > OK... that's good because that would get ugly fast. Saving the secret alias in the status can be done, but is it useful since it's easily generated from the disk/drive alias?

>> 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. > Well RBD would seemingly need to be updated as well, but it's a disjoint thought to the iSCSI, although still related since both RBD and iSCSI can use secrets in this manner. The usage of "-drive driver=XXX" was a separate thought. I wasn't sure if you were going down the path of converting usage of using that newer syntax rather than the "-drive file...". In a way, I suppose I'm surprised that usage of "file" was chosen over "driver=XXX". But that's a different discussion. One that keeps creeping back though as all the QEMU examples show the driver=XXX syntax and it seems they're trying to move away from the "file" syntax.