Jeff Cody <jc...@redhat.com> writes:
> On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> 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/image[@snapshot][: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 :)
