[libvirt] [PATCH] storage: Fix incorrect format for <disk> <auth> XML

Tue Oct 6 21:22:12 UTC 2015

On 10/06/2015 09:19 AM, Michal Privoznik wrote:
> On 23.09.2015 22:54, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1256999
>>
>> When creating a copy of the 'authdef', need to take into account the
>> slight variation between <disk> and <pool> before blindly copying
>> the 'authType' field. This ensures virStorageAuthDefFormat will
>> properly format the <auth> XML for a <disk>.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virstoragefile.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 2aa1d90..0b72cb3 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1522,7 +1522,12 @@ virStorageAuthDefCopy(const virStorageAuthDef *src)
>>      /* Not present for storage pool, but used for disk source */
>>      if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0)
>>          goto error;
>> -    ret->authType = src->authType;
>> +    /* A <disk> uses secrettype, while a <pool> uses authType, so
>> +     * if we have a secrettype, then don't copy authType; otherwise,
>> +     * we will format the authType in <disk>
>> +     */
>> +    if (!ret->secrettype)
>> +        ret->authType = src->authType;
>>      ret->secretType = src->secretType;
> 
> This seems like we have a bug somewhere if authType and secretType are
> both set at the same time. Because the way I understand the code is that
> only one from the pair can be non-null at the time. If so, I think we
> should fix the bug and drop this if() here. 

The short answer is the bug was written long ago when disk and pool's
were "allowed" to use a different formatted authdef XML and now we're
stuck with it. The longer answer and details follow.

First thing to understand "secretType" is a bit of a misnomer. It's
actually the type for the secret usage field. It's not the translation
for 'secrettype'. The secretType is used for both disk and pool XML. For
getting the "type" of auth/secret data a pool uses authType, but a disk
uses secrettype.

The authType is "supposed" to be only present in the pool definition;
however, when generating a "disk" definition from the source pool for a
direct iSCSI pool virStorageTranslateDiskSourcePoolAuth is used to
generate an 'authdef' structure in the disk def in order to be used to
create the command since we cannot rely upon the source pool for the
authentication. This causes the authType copy from source pool to the
disk def (which shouldn't be done), but virStorageAuthDefCopy doesn't
know if it's copying pool->pool, pool->disk, or disk->disk.

So, as an alternative, virStorageTranslateDiskSourcePoolAuth could set
"def->src->auth->authType = VIR_STORAGE_AUTH_TYPE_NONE;" since it's the
only place that knows we're copying from pool->disk. The 'secrettype'
field gets filled after returning from Translate if auth is set, so
that's where that magic happens. I could move the clear of authType
there as well, although I don't think that matters.

Alternatively, virStorageAuthDefFormat could be adjusted to not format
the authType if secrettype is set, but that's a different workaround.

Another (partially related to the bug) change that "could" happen is in
virDomainDiskDefFormat prior to calling virDomainDiskSourceFormat to add
a second condition to whether we print the auth data of "&&
!src->srcpool". That is don't print the authdef data from the source
pool. The authdef data only exists for a running domain using the
'direct'. Of course since we have been printing it already - one could
argue don't change that.

> Moreover, I'd expect a copy
> function to create the exact copy of the original image instead of
> masking a bug somewhere.

While I agree in principle about a copy function, sometimes that copy
function is better to have the logic than having to remember to "fix
things up" after the copy (or even know one has to do that). The Parse
and Format functions know the nuances, so it doesn't seem totally out of
place to have the Copy function have a bit of that knowledge too.

Rmemeber how the auth/secret data can look

<disk>
    <auth username='myuser'>
        <secret type='ceph' usage='mypassid'/>
    </auth>

or
    <auth username='myuser'>
        <secret type='iscsi' usage='libvirtiscsi'/>
    </auth>

or

and

<pool>
    <auth type='chap' username='myname'>
        <secret usage='mycluster_myname'/>
    </auth>

The disk secret type is conceptually the same as the pool auth type.
There's also a secret mechanism involving <encryption> inside a <disk>
which supports a "volume" format:

<disk>
    <encryption format='qcow'>
        <secret type='passphrase'
uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
    </encryption>

Fortunately this one isn't involved with the common parsing of the
<auth> between the storage pool and domain xml parsing, but it is
accounted for as a disk secrettype.

> BTW isn't authType set to VIR_STORAGE_AUTH_TYPE_NONE by default?
> And secretType to VIR_STORAGE_SECRET_TYPE_NONE?
> 

The "authType" can be NONE, CHAP ("chap"), or CEPHX ("ceph"). It's used
by the iSCSI backend.

The "secretType" is not a conversion of "secrettype".  Rather it is
either NONE, UUID, or USAGE. That is it let's the code know the usage
model to lookup the secret via either UUID or USAGE.  I think it's
misnamed and could be "usageType"... Changing it would mean changing
virStorageSecretType to virStorageUsageType as well. Then there's the
similarity to virSecretUsageType that'll certainly cause some confusion.
I'm personally not a fan of massive renames.

The "secrettype" can be NONE, VOLUME ("volume"), CEPH ("ceph"), or ISCSI
("iscsi"). So much for similarity to authType as it's not "chap" or
"ceph" like it would be for a pool...  Also "volume" is special - it's
only used with encryption and is never formatted into secrettype.

In any case, I'll submit a slightly different patch which clears
authType in the translation routine.  I can also generate one to not
print authdef data when there's a srcpool if desired - that's another
simple change.

John