qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH for-2.9 4/5] rbd: Peel off redundant RbdAuthMethod wrapper struct
Date: Mon, 27 Mar 2017 20:20:15 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)

Jeff Cody <address@hidden> writes:

> On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>> > On 03/24/2017 09:10 AM, Jeff Cody wrote:
>> >> On Fri, Mar 24, 2017 at 08:49:20AM -0500, Eric Blake wrote:
>> >>> On 03/24/2017 07:42 AM, Jeff Cody wrote:
>> >>>
>> >>>> Agree.  My preference is to leave it as an array of methods, so long as 
>> >>>> that
>> >>>> is tenable to libvirt.
>> >>>
>> >>> The -drive syntax should remain unchanged (that's an absolute must for
>> >>> libvirt).  But the QMP syntax being an array of methods sounds best to
>> >>> me, and I think password-secret should be part of the array.  So my vote
>> >>> would be for:
>> >>>
>> >>> { "driver": "rbd", "image": "foo",
>> >>>   "auth": [ { "type": "cephx", "password-secret": "sec0" },
>> >>>             { "type": "none" } ],
>> >>>   "pool": "bar" }
>> >>>
>> >>> It makes mapping -drive arguments into QMP form a bit trickier, but the
>> >>> QMP form is then easily extensible if we add another auth method down
>> >>> the road.
>> >>>
>> >> 
>> >> In that case, what about adding user as well, and not just 
>> >> password-secret?
>> >
>> > Makes sense - anything that is associated solely with cephx should
>> > belong to the same flat-union branch for cephx, rather than at the top
>> > level.
>> 
>> I've thought about this some more and have come to the conclusion that
>> this design is clumsy and overly complex.  Moreover, I suspect our
>> testing has been poor.  Let me explain.
>> 
>> = Overview =
>> 
>> What we're trying to configure is authentication methods the client is
>> to offer to the server.
>> 
>> This is not a list, it's a set: the order doesn't matter, and multiple
>> entries of the same type make no sense.  We could reject multiple
>> entries, or let the last one win silently, but this is just unnecessary
>> complexity.
>> 
>> Type "cephx" uses a user name and a key.
>> 
>> Type "none" uses neither (it could theoretically use the user name, but
>> I've been assured it doesn't).
>> 
>> The user name defaults to "admin".
>> 
>> The key defaults to the user name's entry in a keyring.  There is a
>> configurable list of key ring files, with a default.  The default
>> commonly includes /etc/ceph/client.keyring.
>> 
>
> I don't think 'client.keyring' is one of the defaults.  I know
> /etc/ceph/keyring is, however.

Double-checking...  source suggests the default is actually
/etc/ceph/$cluster.$name.keyring,/etc/ceph/$cluster.keyring,/etc/ceph/keyring,/etc/ceph/keyring.bin

https://github.com/ceph/ceph/blob/master/src/common/config_opts.h#L172

>> = Librados configuration =
>> 
>> Librados takes these configuration bits as follows:
>> 
>> * The user name is to be passed to rados_create().  NULL means default
>>   to "admin".
>> 
>> * The key may be passed to rados_conf_set() with key "key" (value is the
>>   key) or "keyfile" (value is name of the file containing the key).
>>   Documentation discourages use of "key".
>> 
>> * The list of keyrings may passed to rados_conf_set() with key
>>   "keyring" and a value listing file names separated by ','.  At least
>>   according to the documentation; the source looks like any non-empty
>>   sequence of [;,= \t]* serves as separator.
>> 
>> * The offered authentication methods may be passed to rados_conf_set()
>>   with key "auth_supported" (deprecated) or "auth_client_required", and
>>   a value listing methods separated by "," (or maybe [;,= \t]*, see
>>   above).  The methods are "cephx" and "none".  Default is "cephx,none".
>> 
>> = Command line -drive =
>> 
>> With -drive file=rbd:pool/address@hidden:key=value...], the
>> key=value... part lets you specify arbitrary rados_conf_set() settings,
>> except pseudo-key "id" is mapped to the user name instead, and
>> pseudo-key "conf" names a rados configuration file.  This overloading of
>> rados_conf_set() keys and our own pseudo-keys isn't pretty, but it's a
>> done deal.  The full set of authentication configuration discussed above
>> is available as keys "id", "key", "keyfile", "keyring", "auth_supported"
>> and "auth_client_required".  Also via "conf", of course.
>> 
>> These -drive rbd:...:key=value settings are also available in -drive
>> QemuOpts syntax -drive driver=rbd,key=value:
>> 
>> * Pseudo-key "id" is "user" in QemuOpts.
>> 
>> * Pseudo-key "conf" is "conf" in QemuOpts, too
>> 
>> * Any other keys together become "keyvalue-pairs" in QemuOpts, with
>>   the same key=value:... syntax.
>> 
>> Additionally, QemuOpts-only "password-secret" lets you set
>> rados_conf_set() key "key" to a value obtained from the QCryptoSecret
>> interface.
>> 
>> Note that @password-secret is a misnomer: this is *not* a password, it's
>> a *key*.  Calling it a password is confusing, and makes it harder to
>> mentally connect QMP and Ceph configuration.
>> 
>
> Good point; @key-secret would be a better name
>
>
>> The settings in file=rbd:... override the ones in QemuOpts (that's how
>> ->bdrv_parse_filename() works), which in turn override (I think)
>> settings from a config file.  Note that *any* key other than "id" and
>> "conf" in file=rbd:... completely overrides *all* keys in
>> "keyvalue-pairs".
>> 
>> Except "password-secret" works the other way: it overrides "key" set in
>> file=rbd:... or "keyvalue-pairs".
>> 
>> As so often with syntactic sugar, once you actually explain how it
>> works, you realize what a bloody mess it is.
>> 
>> It's not quite clear whether "keyvalue-pairs" is really meant for the
>> user, or just for internal use to implement file=rbd:...  I posted a
>> patch to hide it from the user.
>> 
>> = QMP blockdev-add and command line -blockdev =
>> 
>> The current QAPI/QMP schema lets you specify only a few settings
>> directly: pseudo-keys "id" and "conf" (optional members @user and
>> @conf), keys "key" and "auth_supported" (optional members
>> @password-secret and @auth_supported).  The only way to specify other
>> rados_conf_set() settings such as "keyfile" and "keyring" is via "conf".
>> 
>> Begs the question how the settings you can specify directly interact
>> with the config file.  I figure they override the config file.
>> 
>
> Yes, looking at the code rados_conf_set() will override both the config
> file, and environment variables.
>
>
> However, certain environment variables will override settings in the
> specified "conf" file.  I think "CEPH_KEYRING" is the only one (corresponds
> to "keyring"), but I am not sure there are not others.  
>
> This is probably a reason to provide an option for "keyring" via QAPI.

I'm open to provide more configuration knobs via QAPI, but for 2.9, I
propose to stick to the smallest interface that can possibly work.

>> Let's review how this could be used.
>> 
>> If you specify no optional members, you get the Ceph defaults described
>> above.  Good.
>> 
>> If you want to override the default user, there's @user.  Good.
>> 
>> If you want to override the keyring, you have to fall back to @conf.
>> Not ideal, but good enough for 2.9.  I guess most users will be fine
>> with the default keyrings.
>> 
>
> Unless the environment variable CEPH_KEYRING is set, at which point "conf"
> won't override it, unless I am missing something.  However, we could either
> provide a QAPI interface to set the keyring, or keep a user/key option.

For 2.9 QAPI/QMP, I'd say our story for authentication should be "stick
to *Ceph* configuration; how you use it is up to you".  I.e. set up
suitable keyrings, and how exactly you tell Ceph to use non-default
keyrings (config file, environment variable, whatever) is your problem.

>> If you want to override the authentication methods, you have several
>> options:
>> 
>> * Use @conf and set auth_supported (deprecated) or auth_client_required
>>   in the config file
>> 
>> * Use @auth_supported like this:
>> 
>>       "auth_supported": [ { "auth": METHOD}, ... ]
>> 
>>   Clumsy.
>> 
>> If you want to override the key, you again have several options:
>> 
>> * Use @conf and set keyfile or key in the config file
>> 
>> * Use @password-secret to get it from the QCryptoSecret interface
>> 
>>   Did we actually test this?
>
> Yes, and it works.  One caveat: the key obtained from the keyring or
> "ceph auth list" are base64 encoded, and qemu_rbd_set_auth() base64 encodes
> the key itself into base64.  The rados_conf_set() value for "key" is assumed
> to be base64 encoded.  This means the secret file needs to have the key not
> base64 encoded, with the current code.

Good to know, thanks.  Documentation or at least comments explaining
this would be welcome.

>> = What to do for 2.9 =
>> 
>> I propose to
>> 
>> * drop both "auth_supported" and "password-secret" from the QAPI schema
>> 
>> * drop "password-secret" from QemuOpts
>> 
>> * hide "keyvalue-pairs" in QemuOpts
>> 
>> No existing usage is affected, since all these things are new in 2.9.
>> 
>> Users who need something else than the default keyring can continue to
>> configure alternate keys and keyrings with -drive, both directly with
>> file=rbd:...:key=value and with a config file.  With -blockdev /
>> blockdev_add, they need to use the config file.
>> 
>> We can consider improvements post 2.9.
>
> I am fine with this; the only real functional limitation with dropping this
> for 2.9 is that default keyring cannot be overridden in certain scenarios
> (it is set via an environment variable, and 'conf' won't override it).

My PATCH RFC v3 implements this, let's get it reviewed and merged.

Regarding the limitation: if you pass an environment variable to QEMU,
then realize it gets in your way, consider not passing it :)



reply via email to

[Prev in Thread] Current Thread [Next in Thread]