BlockdevOptionsRbd member auth-supported is a list of struct
RbdAuthMethod, whose only member is enum RbdAuthSupport.  There is no
reason for wrapping the enum in a struct.  Delete the wrapper, and
rename the enum.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/rbd.c          |  2 +-
 qapi/block-core.json | 15 ++-------------
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 8ba0f79..f460d71 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -566,7 +566,7 @@ static char *rbd_auth(QDict *options)
     int i;
 
     for (i = 0;; i++) {
-        sprintf(keybuf, "auth-supported.%d.auth", i);
+        sprintf(keybuf, "auth-supported.%d", i);
         val = qdict_get(options, keybuf);
         if (!val) {
             break;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f132fc..fe1e40f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2601,25 +2601,14 @@
 
 
 ##
-# @RbdAuthSupport:
-#
-# An enumeration of RBD auth support
-#
-# Since: 2.9
-##
-{ 'enum': 'RbdAuthSupport',
-  'data': [ 'cephx', 'none' ] }
-
-
-##
 # @RbdAuthMethod:
 #
 # An enumeration of rados auth_supported types
 #
 # Since: 2.9
 ##
-{ 'struct': 'RbdAuthMethod',
-  'data': { 'auth': 'RbdAuthSupport' } }
+{ 'enum': 'RbdAuthMethod',
+  'data': [ 'cephx', 'none' ] }
 
 ##
 # @BlockdevOptionsRbd:
-- 
2.7.4

On 03/23/2017 05:55 AM, Markus Armbruster wrote:
> BlockdevOptionsRbd member auth-supported is a list of struct
> RbdAuthMethod, whose only member is enum RbdAuthSupport.  There is no
> reason for wrapping the enum in a struct.

Well, the previous patch removed the QemuOpts madness as a technical
reason that required the wrapper, leaving nothing else technically using
it at the moment.  But there's still the design to think about...

>  Delete the wrapper, and
> rename the enum.

This patch changes the QMP wire format. It MUST go in 2.9, otherwise,
we've baked in the insanity; that is, if we decide it is insanity [1]

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/rbd.c          |  2 +-
>  qapi/block-core.json | 15 ++-------------
>  2 files changed, 3 insertions(+), 14 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -2601,25 +2601,14 @@
>  
>  
>  ##
> -# @RbdAuthSupport:
> -#
> -# An enumeration of RBD auth support
> -#
> -# Since: 2.9
> -##
> -{ 'enum': 'RbdAuthSupport',
> -  'data': [ 'cephx', 'none' ] }
> -
> -
> -##
>  # @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" } ],

to the potentially much nicer:

..., "auth-supported": [ "none" ],

[1] But what if we want to make auth-supported be an array of flat
unions, where 'auth' is the discriminator that determines if additional
members are needed?  In other words, the existing API would allow:

"auth-supported": [ { "auth": "cephx" },
                    { "auth": "new", "password-id": "foo" } ]

for specifying a new auth type 'new' that comes with an associated
'password-id' field for looking up a corresponding 'secret' object used
for retrieving the associated password when using that type of
authorization.  In that scenario, RbdAuthMethod would look more like
this pseudo-qapi:

{ 'union': 'RbdAuthMethod', 'base': { 'auth': 'RbdAuthSupport' },
  'discriminator': 'auth',
  'data': { 'none': {},
            'cephx': {},
            'new': { 'password-id': 'str' } } }

Assuming we are okay with not needing to extend the current one-member
RbdAuthMethod struct into a flat union, then this patch is fine, and you
can add:

Reviewed-by: Eric Blake <eblake@redhat.com>

But if you think the flat union expansion path may ever prove useful,
then maybe we don't want this patch after all.

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

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?

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

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

On 03/23/2017 04:43 PM, Eric Blake wrote:

> 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.

Or, if it becomes something that libvirt wants to allow users to tune in
their XML (right now, users don't get a choice, they get either 'none'
or 'cephx;none'), a forward-compatible solution under my QMP proposal
would be to have qemu add a third enum state, "none", "cephx", and
"cephx-no-fallback".

On the other hand, if supporting multiple methods at once makes sense
(say librados adds a 'cephy' method, and that users could legitimately
use both 'cephx;cephy' and 'cephy;cephx' with different behaviors), then
keeping auth as an array of instances of a simple flat union scales
nicer than having to add a combinatorial explosion of branches to the
flat union to cover every useful combination of auth_supported methods.
Maybe I'm overthinking it.

> 
> 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

On Thu, Mar 23, 2017 at 04:56:30PM -0500, Eric Blake wrote:
> On 03/23/2017 04:43 PM, Eric Blake wrote:
> 
> > 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.
> 
> Or, if it becomes something that libvirt wants to allow users to tune in
> their XML (right now, users don't get a choice, they get either 'none'
> or 'cephx;none'), a forward-compatible solution under my QMP proposal
> would be to have qemu add a third enum state, "none", "cephx", and
> "cephx-no-fallback".
> 
> On the other hand, if supporting multiple methods at once makes sense
> (say librados adds a 'cephy' method, and that users could legitimately
> use both 'cephx;cephy' and 'cephy;cephx' with different behaviors), then
> keeping auth as an array of instances of a simple flat union scales
> nicer than having to add a combinatorial explosion of branches to the
> flat union to cover every useful combination of auth_supported methods.
> Maybe I'm overthinking it.
> 

I spoke over email with Jason Dillaman (cc'ed), and this is what he told me
with regards to the authentication methods, and what cephx;none means:

On Thu, Mar 23, 2017 at 06:04:30PM -0400, Jason Dillaman wrote:
> It's saying that the client is willing to connect to a server that
> uses cephx auth or a server that uses no auth. Looking at the code,
> the "auth_supported" configuration key is actually deprecated and
> "auth_client_required" should be used instead (which defaults to
> 'cephx, none' already). Since it's been deprecated since v0.55 and if
> you are cleaning things up, feel free to switch to the new one if
> possible.

So from what Jason is saying, it seems like the conclusion we reached over
IRC is correct: it will attempt cephx but also fallback to no auth.

Also, since the preferred auth option may be named different depending on
ceph versions, I know think we probably should not tie the QAPI parameter to
the name of the older deprecated option.

I suggest that the 'auth_supported' parameter for QAPI be renamed to
'auth_method'.  If you don't like the array and the flexibility it provides
at the cost of complexity, I think a flat enum consisting of 3 values like
you mentioned is reasonable.  Since the QAPI does not need to map to the
legacy commandline used by libvirt, I would suggest maybe naming them
slightly different, though: any, none, strict

For 2.9, they could each map to 'auth_supported' like so:
    
    any:     "cephx;none"
    none:    "none"
    strict:  "cephx"

For 2.10, we could try to discover if 'auth_client_required' is supported or
not, and use either it or 'auth_supported' as appropriate (with the same
mappings as above).

The reason I like "any" and "strict", is it gives consistent meanings to
options even if new auth methods are introduced.  For a hypothetical "cephy"
method example:

    any:     "cephy;cephx;none"
    none:    "none"
    strict:  "cephy;cephx"

-Jeff

Am 23.03.2017 um 11:55 hat Markus Armbruster geschrieben:
> BlockdevOptionsRbd member auth-supported is a list of struct
> RbdAuthMethod, whose only member is enum RbdAuthSupport.  There is no
> reason for wrapping the enum in a struct.  Delete the wrapper, and
> rename the enum.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>