Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct

[Eric Blake]

On 03/23/2017 03:59 PM, Eric Blake wrote:
> On 03/23/2017 01:10 PM, Eric Blake wrote:
> 
>>>  # @RbdAuthMethod:
>>>  #
>>>  # An enumeration of rados auth_supported types
>>>  #
>>>  # Since: 2.9
>>>  ##
>>> -{ 'struct': 'RbdAuthMethod',
>>> -  'data': { 'auth': 'RbdAuthSupport' } }
>>> +{ 'enum': 'RbdAuthMethod',
>>> +  'data': [ 'cephx', 'none' ] }
>>
>> Changes BlockdevOptionsRbd from:
>>
>> ..., "auth-supported": [ { "auth": "none" } ],
> 
> Another question - why does auth-supported take an array?  Can you
> really pass both 'none' and 'cephx' at the same time? Or can you only
> pass an array of at most one element, at which point why is it an array?

In fact, the more I look at this, the more I wonder if we really want
'auth' to be a flat union and move the 'password-secret' key to be part
of the cephx authorization scheme, since 'password-secret' only makes
sense with 'cephx', and not with 'none'.  Jeff pointed out to me that
libvirt is currently passing both at once; qemuBuildRBDSecinfoURI():

static int
qemuBuildRBDSecinfoURI(virBufferPtr buf,
                       qemuDomainSecretInfoPtr secinfo)
{
    char *base64secret = NULL;

    if (!secinfo) {
        virBufferAddLit(buf, ":auth_supported=none");
        return 0;
    }

    switch ((qemuDomainSecretInfoType) secinfo->type) {
    case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN:
        if (!(base64secret = virStringEncodeBase64(secinfo->s.plain.secret,

secinfo->s.plain.secretlen)))
            return -1;
        virBufferEscape(buf, '\\', ":", ":id=%s",
secinfo->s.plain.username);
        virBufferEscape(buf, '\\', ":",
                        ":key=%s:auth_supported=cephx\\;none",
                        base64secret);

But maybe what that _really_ means is that librados is letting us say
"I'd really prefer cephx authorization, and here's my key; but if you
can't honor that, I'm also okay with a fallback to no auth".

That feels wrong to me. If you've gone to the bother of providing an
encryption key, falling back to none should probably be an error.

So my proposal is to have:

{ 'enum': 'RbdAuthSupport', 'data': [ 'cephx', 'none' ] }
{ 'struct': 'RbdAuthNone', 'data': { } }
{ 'struct': 'RbdAuthCephx', 'data': { 'password-secret': 'str' } }
{ 'union', 'RbdAuthMethod', 'base': { 'type': 'RbdAuthSupport' },
  'discriminator': 'type',
  'data': { 'none': 'RbdAuthNone',
            'cephx': 'RbdAuthCephx' } } }
{ 'struct': 'BlockdevOptionsRbd',
  'data': { 'pool': 'str',
            'image': 'str',
            '*conf': 'str',
            '*snapshot': 'str',
            '*user': 'str',
            '*server': ['InetSocketAddress'],
            '*auth': 'RbdAuthMethod' } }

so that you pass at most one auth method, looking like:

..., "image": ..., "auth": { "type": "none" }

or like:

..., "image": ..., "auth": { "type": "cephx", "password-secret": "..." }

note that password-secret is NOT at the same level as image, so from the
command line, supporting either old-style insecure 'key=' or new-style
'password-secret' at the top-level of the QemuOpts will have to have
glue code that maps it to the right nested location within the QAPI
type.  If we like it, we'd have to get it implemented before 2.9 bakes
in any other design.

We'd still have to allow libvirt's use of
":key=...:auth_supported=cephx\\;none" on the command line, but from the
QMP side, we would support exactly one auth method, and libvirt will be
updated to match when it starts targetting blockdev-add instead of
legacy command line.

If librados really needs 'cephx;none' any time cephx authorization is
requested, then even though the QMP only requests 'cephx', we can still
map it to 'cephx;none' in qemu - but I'm hoping that setting
auth_supported=cephx rather than auth_supported=cephx;none on the
librados side gives us what we (and libvirt) really want in the first place.

Jeff or Josh, if you could chime in, that would be helpful.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature