Re: [Qemu-block] [Qemu-devel] [RFC][BROKEN] rbd: Allow configuration of authentication scheme

[Kevin Wolf]

Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > The legacy command line syntax supports a "password-secret" option that
> > allows to pass an authentication key to Ceph. This was not supported in
> > QMP so far.
> 
> We tried during development of v2.9.0 (commit 0a55679b4a5), but reverted
> before the release:
> 
> commit 464444fcc161284ac0e743b99251debc297d5236
> Author: Markus Armbruster <address@hidden>
> Date:   Tue Mar 28 10:56:06 2017 +0200
> 
>     rbd: Revert -blockdev and -drive parameter auth-supported
>     
>     This reverts half of commit 0a55679.  We're having second thoughts on
>     the QAPI schema (and thus the external interface), and haven't reached
>     consensus, yet.  Issues include:
>     
>     * The implementation uses deprecated rados_conf_set() key
>       "auth_supported".  No biggie.
>     
>     * The implementation makes -drive silently ignore invalid parameters
>       "auth" and "auth-supported.*.X" where X isn't "auth".  Fixable (in
>       fact I'm going to fix similar bugs around parameter server), so
>       again no biggie.
>     
>     * BlockdevOptionsRbd member @password-secret applies only to
>       authentication method cephx.  Should it be a variant member of
>       RbdAuthMethod?
>     
>     * BlockdevOptionsRbd member @user could apply to both methods cephx
>       and none, but I'm not sure it's actually used with none.  If it
>       isn't, should it be a variant member of RbdAuthMethod?
>     
>     * The client offers a *set* of authentication methods, not a list.
>       Should the methods be optional members of BlockdevOptionsRbd instead
>       of members of list @auth-supported?  The latter begs the question
>       what multiple entries for the same method mean.  Trivial question
>       now that RbdAuthMethod contains nothing but @type, but less so when
>       RbdAuthMethod acquires other members, such the ones discussed above.
>     
>     * How BlockdevOptionsRbd member @auth-supported interacts with
>       settings from a configuration file specified with @conf is
>       undocumented.  I suspect it's untested, too.
>     
>     Let's avoid painting ourselves into a corner now, and revert the
>     feature for 2.9.

We tried to address all of these points in the schema of this RFC. Maybe
things become easier if we compromise on some.

>     Note that users can still configure authentication methods with a
>     configuration file.  They probably do that anyway if they use Ceph
>     outside QEMU as well.

This solution that we originally intended to offer was dismissed by
libvirt as unpractical: libvirt allows the user to specify both a config
file and a key, and if it wanted to use a config file to pass the key,
it would have to create a merged config file and keep it sync with the
user config file at all times. Understandable that they want to avoid
this.

>     Further note that this doesn't affect use of key "auth-supported" in
>     -drive file=rbd:...:key=value.
>     
>     qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
>     which is silly.  This will be cleaned up shortly.
>     
>     Signed-off-by: Markus Armbruster <address@hidden>
>     Reviewed-by: Max Reitz <address@hidden>
>     Reviewed-by: Eric Blake <address@hidden>
>     Reviewed-by: Jeff Cody <address@hidden>
>     Message-id: address@hidden
>     Signed-off-by: Jeff Cody <address@hidden>
> 
> > This patch introduces authentication options in the QAPI schema, makes
> > them do the corresponding rados_conf_set() calls and adds compatibility
> > code that translates the old "password-secret" option both for opening
> 
> Suggest 'the old "password-secret" command line option parameter'.
> 
> > and creating images to the new set of options.
> >
> > Note that the old option didn't allow to explicitly specify the set of
> 
> Likewise, 'the old option parameter'.
> 
> > allowed authentication schemes. The compatibility code assumes that if
> > "password-secret" is given, only the cephx scheme is allowed. If it's
> > missing, both none and cephx are allowed because the configuration file
> > could still provide a key.
> 
> This is a bit terse for readers who aren't familiar with the way things
> work now (or have since forgotten pretty much everything about it, like
> me).
> 
> Perhaps spelling out how the compatibility code translates the old
> option parameter to BlockdevOptionsRbd would help.
> 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >
> > This doesn't actually work correctly yet because the way that options
> > are passed through the block layer (QAPI -> QemuOpts -> QDict). Before
> > we fix or hack around this, let's make sure this is the schema that we
> > want.
> 
> Makes sense.
> 
> > The two known problems are:
> >
> > 1. QDict *options in qemu_rbd_open() may contain options either in their
> >    proper QObject types (if passed from blockdev-add) or as strings
> >    (-drive). Both forms can be mixed in the same options QDict.
> 
> Remind me, how can such a mix happen?

The way I'm aware of is that you use blockdev-add, so you get the real
types for some options, and then the block layer adds strings for
default values.

> >    rbd uses the keyval visitor to convert the options into a QAPI
> >    object. This means that it requires all options to be strings. This
> >    patch, however, introduces a bool property, so the visitor breaks
> >    when it gets its input from blockdev-add.
> >
> >    Options to hack around the problem:
> >
> >    a. Do an extra conversion step or two and go through QemuOpts like
> >       some other block drivers. When I offered something like this to
> >       Markus a while ago in a similar case, he rejected the idea.
> 
> Going through QemuOpts just to get rid of strings is a bit like going
> down Niagara Falls in a barrel just to get rid of sleepiness: it'll do
> that, sure, but it's probably not all it'll do.

Nice comparison. I tend to agree, though there is much more precedence
for this one than for all other options. Almost all block drivers use
QemuOpts to parse their driver-specific options from the QDict.

> >    b. Introduce a qdict_stringify_entries() that just does what its name
> >       says. It would be called before the running keyval visitor so that
> >       only strings will be present in the QDict.
> 
> Precedence: a bunch of other QDict operations that are used only by the
> block layer.  One more won't make a difference, I guess.
> 
> Aside: I'm tempted to move them out of qdict.h to reduce the temptation
> to use them outside the block layer.

Unrelated cleanup, but seems reasonable. block/qdict.h?

> >    c. Do a local one-off hack that checks if the entry with the key
> >       "auth-none" is a QBool, and if so, overwrite it with a string. The
> >       problem will reappear with the next non-string option.
> 
> Defensible only if we're fairly confident such options will remain rare.
> 
> >    (d. Get rid of the QDict detour and work only with QAPI objects
> >        everywhere. Support rbd authentication only in QEMU 4.0.)
> 
> This is of course the proper solution, but it's a big project that will
> take time.  The occasional stop gaps are unavoidable.  We just need to
> take care not to spend all of our cycles on stop gaps, and none on
> actual progress.
> 
>      e. Make the keyval visitor accept scalars other than strings.
> 
> More efficient than b., doubt it matters.  Complicates the visitor.
> Harder to back out than b. when we no longer need it.
> 
> I'm leaning towards b.

Okay, that's what I was leaning towards, too.

> > 2. The proposed schema allows 'auth-cephx': {} as a valid option with
> >    the meaning that the cephx authentication scheme is enabled, but no
> >    key is given (e.g. it is taken from the config file).
> 
> Apropos config file: we need to be careful to maintain the QAPI schema's
> promise that "Values in the configuration file will be overridden by
> options specified via QAPI."

Yes, though it's made a bit easier by the fact that most options are
implemented with a rados_conf_set() call. If you call the function, you
override the config, if not, you don't.

> >    However, an empty dict cannot currently be represented by flattened
> >    QDicts.
> 
> Flattening a QDict moves nested scalars to the root (with dotted keys),
> and drops nested non-scalars, i.e. QDict, QList.  A nested empty QDict
> (or QList) vanishes without trace.
> 
> >            We need to find a way to enable this. I think this will be
> >    externally visible because it directly translates into the dotted
> >    syntax of -blockdev, so we may want to be careful.
> 
> Quote keyval.c:
> 
>  * Design flaw: there is no way to denote an empty array or non-root
>  * object.  While interpreting "key absent" as empty seems natural
>  * (removing a key-val from the input string removes the member when
>  * there are more, so why not when it's the last), it doesn't work:
>  * "key absent" already means "optional object/array absent", which
>  * isn't the same as "empty object/array present".
> 
> Getting that syntax right was hairy (you'll probably remember the
> lengthy e-mail discussions).  I'm reluctant to revisit it.  We concluded
> back then that dotted key syntax capable of expressing arbitrary JSON
> wasn't in the cards, but that the cases it can't do are so exotic that
> users should just fall back to JSON.  And that would be my advice here.
> 
> Can we design a schema that avoids this case?  Let's see below.

Can we design such a schema? Yes.

Will we like it enough to accept it as a compromise? Maybe.

> > Any thoughts on the proposed QAPI schema or the two implementation
> > problems are welcome.
> >
> >  qapi/block-core.json |  22 +++++++++++
> >  block/rbd.c          | 102 
> > ++++++++++++++++++++++++++++++++++++++-------------
> >  2 files changed, 99 insertions(+), 25 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index c50517bff3..d5ce588add 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3170,6 +3170,19 @@
> >  
> >  
> >  ##
> > +# @RbdAuthCephx:
> > +#
> > +# @key-secret:         ID of a QCryptoSecret object providing a key for 
> > cephx
> > +#                      authentication. If not specified, a key from the
> > +#                      specified configuration file, or the system default
> > +#                      configuration is used, if present.
> > +#
> > +# Since: 2.13
> > +##
> > +{ 'struct': 'RbdAuthCephx',
> > +  'data': { '*key-secret': 'str' } }
> > +
> > +##
> >  # @BlockdevOptionsRbd:
> >  #
> >  # @pool:               Ceph pool name.
> > @@ -3184,6 +3197,13 @@
> >  #
> >  # @user:               Ceph id name.
> >  #
> > +# @auth-none:          true if connecting to a server without 
> > authentication
> > +#                      should be allowed (default: false; since 2.13)
> > +#
> > +# @auth-cephx:         Configuration for cephx authentication if 
> > specified. If
> > +#                      not specified, cephx authentication is not allowed.
> > +#                      (since 2.13)
> > +#
> >  # @server:             Monitor host address and port.  This maps
> >  #                      to the "mon_host" Ceph option.
> >  #
> > @@ -3195,6 +3215,8 @@
> >              '*conf': 'str',
> >              '*snapshot': 'str',
> >              '*user': 'str',
> > +            '*auth-none': 'bool',
> > +            '*auth-cephx': 'RbdAuthCephx',
> >              '*server': ['InetSocketAddressBase'] } }
> >  
> >  ##
> 
> Two new optional members @auth-none, @auth-cephx.
> 
> For backwards compatibility, behavior needs to remain unchanged when
> both are absent.  What is the behavior we need to preserve?  Config
> file, then default?

Yes. (rados_conf_set("auth_client_required") is not called at all.)

> The schema permits four cases, which get translated to an auth client
> required setting by qemu_rbd_set_auth(), visible below:
> 
> (1) just auth-none: we set auth_client_required to "none"
> 
> (2) just auth-cephx: we set auth_client_required to "cephx"
> 
> (3) both: we set auth_client_required to "none;cephx", which I can't
>     find in the documentation right now, but according to my notes means
>     "either method"

The state of the Ceph documentation indeed wasn't very helpful. On the
other hand, I'm afraid we're not in a position to complain about bad
documentation...

> (4) neither: rejected with "No authentication scheme allowed"
>     Uh, why isn't this a compatibility break?  Or am I confused?

It is, and that is one of the reasons why this patch is broken. I failed
to mention this in the commit message, but I replied to the patch the
next day to add this.

> When auth-cephx is present, we get subcases
> 
> (2a) and (3a) key-secret present: key is in the QCryptoSecret named, and
> we set
> 
> (2b) and (3b) key-secret absent: key must be provided some other way,
>      say in a configuration file, default keyring, whatever, or else
>      we'll fail somehow, I guess
> 
> Related: existing BlockdevOptionsRbd member @user.  As far as I know,
> it's meaningless with auth-none.

I don't know otherwise and suspect you are right, but Jeff and I
couldn't find any definite confirmation for this assumption, so we left
it where it is instead of moving it into RbdAuthCephx.

> Ways to avoid the troublesome auth-cephx: {}:
> 
> (A) Make auth-cephx a bool, rely on the other ways to provide the key
> 
> (B) Make auth-cephx a bool and add the @key-secret member next to it
> 
>     Like @user, the member is meaningless with auth-none.
> 
> (C) Make auth-cephx.key-secret a mandatory StrOrNull
> 
>     Should take care of "vanishes on flattening" problem, but dotted key
>     syntax is still screwed: since any value is a valid string, there's
>     none left for null.  My take would be that if you want to say
>     auth-cephx.key-secret: {}, you get to say it in JSON.
> 
>     Aside: check_alternate() tries to catch such alternates, but we
>     failed to update it when we added first class null.  *sigh*
> 
> Which one do you hate least?

What should happen with null when you stringify it? If we want to take
the options QDict, stringify all entries and run them through the keyval
visitor, I'm almost sure that null will be lost.

So (A) doesn't work unless we can specify what "other ways" are that are
acceptable for libvirt, and (C) probably doesn't play well with b. above
(the qdict_stringify_entries() for handling mixed type QDicts).

Looks like only (B) is left as viable, so that's automatically the one I
hate least of these. If we can fix the problems, I think I'd prefer (C),
though.

Kevin