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

2017-04-03 Thread Eric Blake 
On 04/03/2017 06:25 AM, Daniel P. Berrange wrote:
> On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
>> = 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.
> 
> Maybe I'm mis-understanding what you're suggesting wrt QemuOpts, but
> 'password-secret' with RBD is not new in 2.9.0

We updated things in the respin of this series; the actual committed
version (ending in commit d1c13688) did NOT kill password-secret from
QemuOpts, but merely removed it from QMP (it was the QMP portion that
was attempted to be new in 2.9).

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



signature.asc
Description: OpenPGP digital signature

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

2017-04-03 Thread Daniel P. Berrange 
On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
> = 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.

Maybe I'm mis-understanding what you're suggesting wrt QemuOpts, but
'password-secret' with RBD is not new in 2.9.0

It was added in 2.6.0 in this commit:

commit 60390a2192e7b38aee18db6ce7fb740498709737
Author: Daniel P. Berrange 
Date:   Thu Jan 21 14:19:19 2016 +

rbd: add support for getting password from QCryptoSecret object

Currently RBD passwords must be provided on the command line
via

  $QEMU -drive file=rbd:pool/image:id=myname:\
   key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
   auth_supported=cephx

This is insecure because the key is visible in the OS process
listing.

This adds support for an 'password-secret' parameter in the RBD
parameters that can be used with the QCryptoSecret object to
provide the password via a file:

  echo "QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=" > poolkey.b64
  $QEMU -object secret,id=secret0,file=poolkey.b64,format=base64 \
-drive driver=rbd,filename=rbd:pool/image:id=myname:\
   auth_supported=cephx,password-secret=secret0

Reviewed-by: Josh Durgin 
Signed-off-by: Daniel P. Berrange 
Message-id: 1453385961-10718-2-git-send-email-berra...@redhat.com
Signed-off-by: Jeff Cody 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

2017-03-27 Thread Markus Armbruster 
Jeff Cody  writes:

> On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
>> Eric Blake  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
>>

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

2017-03-27 Thread Jeff Cody 
On Mon, Mar 27, 2017 at 07:58:51AM +0200, Markus Armbruster wrote:
> Eric Blake  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.


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

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

2017-03-26 Thread Markus Armbruster 
Eric Blake  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.

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

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"

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

2017-03-24 Thread Kevin Wolf 
Am 24.03.2017 um 14:49 hat Eric Blake geschrieben:
> 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.

Not sure if anybody cares, but I came to the same conclusion, so I like
this.

Kevin

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

2017-03-24 Thread Eric Blake 
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.

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



signature.asc
Description: OpenPGP digital signature

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

2017-03-24 Thread Jeff Cody 
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?

Jeff

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

2017-03-24 Thread Eric Blake 
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.

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



signature.asc
Description: OpenPGP digital signature

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

2017-03-24 Thread Jeff Cody 
On Fri, Mar 24, 2017 at 08:05:49AM +0100, Markus Armbruster wrote:
> Adding Daniel Berrange for QCryptoSecret expertise.
> 
> Jeff Cody  writes:
> 
> > 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.
> 
> So the client offers the server a list of authentication methods with
> credentials, and the server picks one it likes.  Makes sense to me.
> 
> Inluding "none" in the default less so, but that's of no concern to the
> QMP interface, so let's ignore it for now.
> 
> > 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.
> 
> Yes.
> 
> > 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"
> 
> Two years later, an unfixable cryptograhic weakness in cephx is
> discovered.  Some users really, really want to select only "cephy", but
> they can't.  We react by pushing out a QEMU update adding method
> "stricter", which expands into "cephy".  Libvirt then reacts and pushes
> out an update.  And so forth, up the stack.  Many moons later, users can
> actually select "cephy".  Thanks, but no thanks.
> 

Good point; it could be dealt with, but only clumsily.


> I think we should expose the current, recommended way to configure
> authentication, in a form that is suitable for QAPI/QMP, i.e. structured
> data as JSON objects, not strings you need to parse.
> 
> If a future authentication method might need something else than a
> QCryptoSecret object, we need to take Eric's proposal and make

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

2017-03-24 Thread Markus Armbruster 
Adding Daniel Berrange for QCryptoSecret expertise.

Jeff Cody  writes:

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

So the client offers the server a list of authentication methods with
credentials, and the server picks one it likes.  Makes sense to me.

Inluding "none" in the default less so, but that's of no concern to the
QMP interface, so let's ignore it for now.

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

Yes.

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

Two years later, an unfixable cryptograhic weakness in cephx is
discovered.  Some users really, really want to select only "cephy", but
they can't.  We react by pushing out a QEMU update adding method
"stricter", which expands into "cephy".  Libvirt then reacts and pushes
out an update.  And so forth, up the stack.  Many moons later, users can
actually select "cephy".  Thanks, but no thanks.

I think we should expose the current, recommended way to configure
authentication, in a form that is suitable for QAPI/QMP, i.e. structured
data as JSON objects, not strings you need to parse.

If a future authentication method might need something else than a
QCryptoSecret object, we need to take Eric's proposal and make this an
array of unions, where the union tag is the method, and the variant
members supply whatever data the method needs (none: nothing, cephx: a
QCryptoSecret).  Member @password-secret goes away.

Else, we can make it an array of methods, and keep member
@password-secret.

We need to decide quickly to keep rbd fully

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

2017-03-23 Thread Jeff Cody 
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

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

2017-03-23 Thread Eric Blake 
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



signature.asc
Description: OpenPGP digital signature

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

2017-03-23 Thread 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

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

2017-03-23 Thread Eric Blake 
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



signature.asc
Description: OpenPGP digital signature

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

2017-03-23 Thread Kevin Wolf 
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 

Reviewed-by: Kevin Wolf

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

2017-03-23 Thread Eric Blake 
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 
> ---
>  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 

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



signature.asc
Description: OpenPGP digital signature

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

2017-03-23 Thread Markus Armbruster 
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 
---
 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