rbd: Allow configuration of authentication scheme
RFC 1/1 rbd: Allow configuration of authentication scheme
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.
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 and creating images to the new set of options.
Note that the old option didn't allow to explicitly specify the set of 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 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.
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.
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.
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.
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.
(d. Get rid of the QDict detour and work only with QAPI objects
everywhere. Support rbd authentication only in QEMU 4.0.)
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).
However, an empty dict cannot currently be represented by flattened QDicts. 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.
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(-)
qapi/block-core.json +22 -0
@@ -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'] } } ##
block/rbd.c +77 -25
163 lines not on this page · raw message
Signature · 1 line
2.13.6
Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
Quoted 9 lines
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.
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 and creating images to the new set of options.
Quoted 5 lines
Note that the old option didn't allow to explicitly specify the set of 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.
There is another problem here that suggests that maybe this is not the right QAPI schema after all: The defaults needed for command line compatibility and those promised in the QAPI schema are conflicting.
The required command line behaviour is as described above:
* password-secret given: only cephx
* no options given: cephx, none
The desired QMP default behaviour is:
* auth-cephx given: allow cephx
* auth-none given: allow none
* both given: allow both
* no options given: error
In .bdrv_open() there is no way to distinguish the "no options given" of the command line from that of QMP. The current implementation allows everything if no options are given, i.e. it keeps existing command lines working, but it doesn't correctly implement the behaviour described in the QAPI schema.
I don't think changing the description of the QAPI schema would be a good idea, it would be a rather surprising interface.
Quoted 40 lines
Signed-off-by Kevin Wolf <[email protected]>---
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.
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.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.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.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.(d. Get rid of the QDict detour and work only with QAPI objects
everywhere. Support rbd authentication only in QEMU 4.0.)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).
Quoted 7 lines
However, an empty dict cannot currently be represented by flattened QDicts. 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.
Any thoughts on the proposed QAPI schema or the two implementation problems are welcome.
Kevin
Am 06.04.2018 um 10:04 hat Kevin Wolf geschrieben:
Quoted 32 lines
Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
Quoted 9 lines
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.
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 and creating images to the new set of options.Quoted 5 lines
Note that the old option didn't allow to explicitly specify the set of 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.
There is another problem here that suggests that maybe this is not the right QAPI schema after all: The defaults needed for command line compatibility and those promised in the QAPI schema are conflicting.
The required command line behaviour is as described above:
* password-secret given: only cephx
* no options given: cephx, noneThe desired QMP default behaviour is:
* auth-cephx given: allow cephx
* auth-none given: allow none
* both given: allow both
* no options given: error
Quoted 56 lines
In .bdrv_open() there is no way to distinguish the "no options given" of the command line from that of QMP. The current implementation allows everything if no options are given, i.e. it keeps existing command lines working, but it doesn't correctly implement the behaviour described in the QAPI schema.
I don't think changing the description of the QAPI schema would be a good idea, it would be a rather surprising interface.
Quoted 47 lines
Signed-off-by Kevin Wolf <[email protected]>---
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.
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.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.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.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.(d. Get rid of the QDict detour and work only with QAPI objects
everywhere. Support rbd authentication only in QEMU 4.0.)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).
However, an empty dict cannot currently be represented by flattened QDicts. 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.
Any thoughts on the proposed QAPI schema or the two implementation problems are welcome.
Ping?
If nobody has an opinion, we might as well just revert the revert and bring the legacy interface 1:1 to QMP.
Kevin
Quoted 32 lines · Kevin Wolf
Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
Quoted 9 lines
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.
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 and creating images to the new set of options.Quoted 5 lines
Note that the old option didn't allow to explicitly specify the set of 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.
There is another problem here that suggests that maybe this is not the right QAPI schema after all: The defaults needed for command line compatibility and those promised in the QAPI schema are conflicting.
The required command line behaviour is as described above:
* password-secret given: only cephx
* no options given: cephx, noneThe desired QMP default behaviour is:
* auth-cephx given: allow cephx
* auth-none given: allow none
* both given: allow both
* no options given: error
Quoted 5 lines
In .bdrv_open() there is no way to distinguish the "no options given" of the command line from that of QMP. The current implementation allows everything if no options are given, i.e. it keeps existing command lines working, but it doesn't correctly implement the behaviour described in the QAPI schema.
Can we use a QAPI alternate with an explicit JSON null for the command line 'no options given' case?
Quoted 11 lines
I don't think changing the description of the QAPI schema would be a good idea, it would be a rather surprising interface.
Quoted 7 lines
Signed-off-by Kevin Wolf <[email protected]>---
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.
Can we skip the intermediate QemuOpts? If we can go straight from command line to QDict using just QAPI, won't that give us what we really need?
Quoted 22 lines
Quoted 22 lines
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.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.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.
Quoted 6 lines
Quoted 6 lines
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.(d. Get rid of the QDict detour and work only with QAPI objects
everywhere. Support rbd authentication only in QEMU 4.0.)
Oh, even one step further than just avoiding QemuOpts, but using REAL types everywhere in the block layer! It might be a nice cleanup, but it would probably take a lot of effort in the meantime to get to that point?
Quoted 9 lines
Quoted 9 lines
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).
However, an empty dict cannot currently be represented by flattened QDicts. 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.
Can we just require users to give -blockdev with the JSON format (rather than dotted format) when they need to use that particular feature on the command line?
Quoted 6 lines
Quoted 3 lines
Any thoughts on the proposed QAPI schema or the two implementation problems are welcome.
Kevin
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Am 18.04.2018 um 15:21 hat Eric Blake geschrieben:
Quoted 33 lines · Kevin Wolf
Quoted 32 lines
Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
Quoted 9 lines
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.
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 and creating images to the new set of options.Quoted 5 lines
Note that the old option didn't allow to explicitly specify the set of 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.
There is another problem here that suggests that maybe this is not the right QAPI schema after all: The defaults needed for command line compatibility and those promised in the QAPI schema are conflicting.
The required command line behaviour is as described above:
* password-secret given: only cephx
* no options given: cephx, noneThe desired QMP default behaviour is:
* auth-cephx given: allow cephx
* auth-none given: allow none
* both given: allow both
* no options given: error
Quoted 8 lines
Quoted 5 lines
In .bdrv_open() there is no way to distinguish the "no options given" of the command line from that of QMP. The current implementation allows everything if no options are given, i.e. it keeps existing command lines working, but it doesn't correctly implement the behaviour described in the QAPI schema.
Can we use a QAPI alternate with an explicit JSON null for the command line 'no options given' case?
The fundamental problem is that we can only have a single default value for both command line and QMP. I don't think an alternate would change anything there.
Both the commands line and the QMP 'no options given' cases would end up being represented by a missing key in the options QDict. null would only be used if explicitly given in blockdev-add (I don't think you can specify null on the command line at all).
Quoted 7 lines
Quoted 7 lines
I don't think changing the description of the QAPI schema would be a good idea, it would be a rather surprising interface.
Quoted 3 lines
Signed-off-by Kevin Wolf <[email protected]>---
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
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.
Can we skip the intermediate QemuOpts? If we can go straight from command line to QDict using just QAPI, won't that give us what we really need?
In fact, I think that's what -blockdev already does. The one that I had in mind is -drive, which adds the additional QemuOpts step, but we don't have to support -drive with a new syntax.
However, the problem is with the representation in the QDict, so skipping QemuOpts doesn't help.
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
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.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.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.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.
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
(d. Get rid of the QDict detour and work only with QAPI objects
everywhere. Support rbd authentication only in QEMU 4.0.)Oh, even one step further than just avoiding QemuOpts, but using REAL types everywhere in the block layer! It might be a nice cleanup, but it would probably take a lot of effort in the meantime to get to that point?
Yes, I would like to get there, but it's definitely a long term goal (that's why I wrote "QEMU 4.0"). It definitely means getting rid of any options that work on the command line, but are not part of the QAPI schema (including "internal" options that are only supposed to be added by .bdrv_parse_filename implementations). And even then, it's probably far from trivial.
Realistically, only a/b/c are the options we have for 2.13. Or maybe another option that I'm missing.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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).
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
However, an empty dict cannot currently be represented by flattened QDicts. 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.
Can we just require users to give -blockdev with the JSON format (rather than dotted format) when they need to use that particular feature on the command line?
Yes, requiring -blockdev with the JSON format would be fine. But the next thing we do with the resulting QDict is flattening it, which loses empty dicts.
Kevin
Quoted 65 lines · Kevin Wolf
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.
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 and creating images to the new set of options.Note that the old option didn't allow to explicitly specify the set of 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.
Signed-off-by Kevin Wolf <[email protected]>---
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(-)qapi/block-core.json +20
-0@@ -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. #
qapi/block-core.json line 3220
Quoted 6 lines
qapi/block-core.json +2
-0@@ -3195,6 +3215,8 @@ '*conf': 'str', '*snapshot': 'str', '*user': 'str', + '*auth-none': 'bool', + '*auth-cephx': 'RbdAuthCephx', '*server': ['InetSocketAddressBase'] } }
Would it be better to have this be a flat union with 'auth' with enum values 'none', 'cephx', 'both' as a discriminator that determines which additional fields can be present? Or does that require that we first fix the QAPI generator to allow nesting a flat union within another flat union (probably doable, just no one has needed it before now)? Is it also time to improve the QAPI generator to allow a default value to the discriminator field, rather than requiring the field to be present?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Am 18.04.2018 um 15:26 hat Eric Blake geschrieben:
Quoted 73 lines · Kevin Wolf
Quoted 65 lines
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.
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 and creating images to the new set of options.Note that the old option didn't allow to explicitly specify the set of 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.
Signed-off-by Kevin Wolf <[email protected]>---
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(-)qapi/block-core.json +20
-0@@ -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. #qapi/block-core.json line 3220
Quoted 6 lines
qapi/block-core.json +2
-0@@ -3195,6 +3215,8 @@ '*conf': 'str', '*snapshot': 'str', '*user': 'str', + '*auth-none': 'bool', + '*auth-cephx': 'RbdAuthCephx', '*server': ['InetSocketAddressBase'] } }
Quoted 7 lines
Would it be better to have this be a flat union with 'auth' with enum values 'none', 'cephx', 'both' as a discriminator that determines which additional fields can be present? Or does that require that we first fix the QAPI generator to allow nesting a flat union within another flat union (probably doable, just no one has needed it before now)? Is it also time to improve the QAPI generator to allow a default value to the discriminator field, rather than requiring the field to be present?
Both options can be enabled at the same time, so that the client connects to a server no matter whether it does 'cephx' authentication or only 'none. This is even the default for rbd driver (in the existing command line interface, but I think we need to stay compatible with it). With a union you would have to explicitly choose one or the other, but could never accept both.
The other option we were considering was a list of authentication options, which would be easier to implement, but isn't really an accurate representation of what we really accept. There is no way we could meaningfully implement something like this:
'auth': [ { 'type': 'cephx', 'key-secret': 'foo' },
{ 'type': 'cephx', 'key-secret': 'bar' } ]Because Ceph only allows us to enable the 'cephx' authentication method and to set a single key for it.
Kevin
Quoted 28 lines · Kevin Wolf
Quoted 8 lines
qapi/block-core.json line 3220
Quoted 7 lines
qapi/block-core.json +2
-0@@ -3195,6 +3215,8 @@ '*conf': 'str', '*snapshot': 'str', '*user': 'str', + '*auth-none': 'bool', + '*auth-cephx': 'RbdAuthCephx', '*server': ['InetSocketAddressBase'] } }Quoted 7 lines
Would it be better to have this be a flat union with 'auth' with enum values 'none', 'cephx', 'both' as a discriminator that determines which additional fields can be present? Or does that require that we first fix the QAPI generator to allow nesting a flat union within another flat union (probably doable, just no one has needed it before now)? Is it also time to improve the QAPI generator to allow a default value to the discriminator field, rather than requiring the field to be present?
Both options can be enabled at the same time, so that the client connects to a server no matter whether it does 'cephx' authentication or only 'none. This is even the default for rbd driver (in the existing command line interface, but I think we need to stay compatible with it). With a union you would have to explicitly choose one or the other, but could never accept both.
The other option we were considering was a list of authentication options, which would be easier to implement, but isn't really an accurate representation of what we really accept. There is no way we could meaningfully implement something like this:
Quoted 5 lines
'auth': [ { 'type': 'cephx', 'key-secret': 'foo' }, { 'type': 'cephx', 'key-secret': 'bar' } ]Because Ceph only allows us to enable the 'cephx' authentication method and to set a single key for it.
How does it look as a choice between:
{'enum':'CephxAuth', 'data': ['none', 'cephx', 'both' ]}
where both 'cephx' and 'both' support the optional 'key-secret' parameter, but 'none' does not?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Am 18.04.2018 um 16:16 hat Eric Blake geschrieben:
Quoted 36 lines · Kevin Wolf
Quoted 29 lines
Quoted 16 lines
qapi/block-core.json line 3220
Quoted 8 lines
qapi/block-core.json +2
-0@@ -3195,6 +3215,8 @@ '*conf': 'str', '*snapshot': 'str', '*user': 'str', + '*auth-none': 'bool', + '*auth-cephx': 'RbdAuthCephx', '*server': ['InetSocketAddressBase'] } }Would it be better to have this be a flat union with 'auth' with enum values 'none', 'cephx', 'both' as a discriminator that determines which additional fields can be present? Or does that require that we first fix the QAPI generator to allow nesting a flat union within another flat union (probably doable, just no one has needed it before now)? Is it also time to improve the QAPI generator to allow a default value to the discriminator field, rather than requiring the field to be present?
Both options can be enabled at the same time, so that the client connects to a server no matter whether it does 'cephx' authentication or only 'none. This is even the default for rbd driver (in the existing command line interface, but I think we need to stay compatible with it). With a union you would have to explicitly choose one or the other, but could never accept both.
The other option we were considering was a list of authentication options, which would be easier to implement, but isn't really an accurate representation of what we really accept. There is no way we could meaningfully implement something like this:
Quoted 5 lines
'auth': [ { 'type': 'cephx', 'key-secret': 'foo' }, { 'type': 'cephx', 'key-secret': 'bar' } ]Because Ceph only allows us to enable the 'cephx' authentication method and to set a single key for it.
Quoted 6 lines
How does it look as a choice between:
{'enum':'CephxAuth', 'data': ['none', 'cephx', 'both' ]}
where both 'cephx' and 'both' support the optional 'key-secret' parameter, but 'none' does not?
Doesn't really look extensible for the case that Ceph adds a third mode. At least I don't think we want to have an enum and associated union branches for all possible combinations?
Kevin
Quoted 3 lines · Kevin Wolf
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 <[email protected]> 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.
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.
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.
Message-id: [email protected]
Quoted 3 lines
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'.
Quoted 3 lines
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'.
Quoted 4 lines
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.
Quoted 7 lines
Signed-off-by Kevin Wolf <[email protected]>---
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.
Quoted 5 lines
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?
Quoted 10 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
(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.
Quoted 3 lines
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."
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 46 lines
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(-)qapi/block-core.json +20
-0@@ -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. #
qapi/block-core.json line 3222
Quoted 8 lines
qapi/block-core.json +2
-0@@ -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?
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"
(4) neither: rejected with "No authentication scheme allowed" Uh, why isn't this a compatibility break? Or am I confused?
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.
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?
Quoted 55 lines
block/rbd.c +23
-12@@ -105,8 +105,7 @@ typedef struct BDRVRBDState {static int qemu_rbd_connect (rados_t *cluster,rados_ioctx_t *io_ctx,BlockdevOptionsRbd *opts,bool cache, -const char *keypairs,const char *secretid, -Error **errp); +const char *keypairs,Error **errp);static char *qemu_rbd_next_tok (char *src,char delim,char **p) {@@ -231,21 +230,40 @@ done: } -static int qemu_rbd_set_auth (rados_t cluster,const char *secretid, +static int qemu_rbd_set_auth (rados_t cluster,BlockdevOptionsRbd *opts,Error **errp) { -if (secretid ==0 ) { -return 0 ; - } +int r; -gchar *secret =qcrypto_secret_lookup_as_base64 (secretid, - errp); -if (!secret) { -return -1 ; +if (opts->auth_none && opts->has_auth_cephx) { + r =rados_conf_set (cluster,"auth_client_required" ,"none;cephx" ); + }else if (opts->auth_none) { + r =rados_conf_set (cluster,"auth_client_required" ,"none" ); + }else if (opts->has_auth_cephx) { + r =rados_conf_set (cluster,"auth_client_required" ,"cephx" ); + }else { +error_setg (errp,"No authentication scheme allowed" ); +return -EINVAL ; + } +if (r <0 ) { +error_setg_errno (errp, -r,"Could not set 'auth_client_required'" ); +return r; } -rados_conf_set (cluster,"key" , secret); -g_free (secret); +if (opts->has_auth_cephx && opts->auth_cephx->has_key_secret) { +gchar *secret = +qcrypto_secret_lookup_as_base64 (opts->auth_cephx->key_secret, errp); +if (!secret) { +return -EIO ; + } +
block/rbd.c done: · line 269
Quoted 10 lines
block/rbd.c +7
-0+ r =rados_conf_set (cluster,"key" , secret); +g_free (secret); +if (r <0 ) { +error_setg_errno (errp, -r,"Could not set 'key'" ); +return r; + } + }return 0 ; }
After the patch: cases (1), (2a), (2b), (3a), (3b), (4) described above. Note that we now set "auth_client_required" in addition to "key".
Before the patch: If @secretid is null, do nothing, else set "key" to the QCryptoSecret named by it. "auth_client_required" could be set in the configuration file, or default to "cephx".
How does backward compatibility work?
block/rbd.c static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
Quoted 34 lines
block/rbd.c +4
-7@@ -337,12 +355,9 @@ static QemuOptsList runtime_opts = { }, }; -/* FIXME Deprecate and remove keypairs or make it available in QMP. -* password_secret should eventually be configurable in opts->location. Support -* for it in .bdrv_open will make it work here as well. */ +/* FIXME Deprecate and remove keypairs or make it available in QMP. */ static int qemu_rbd_do_create (BlockdevCreateOptions *options, -const char *keypairs,const char *password_secret, -Error **errp) +const char *keypairs,Error **errp) {BlockdevCreateOptionsRbd *opts = &options->u.rbd;rados_t cluster;@@ -370,7 +385,7 @@ static int qemu_rbd_do_create(BlockdevCreateOptions *options, } ret =qemu_rbd_connect (&cluster, &io_ctx, opts->location, false, keypairs, - password_secret, errp); + errp);if (ret <0 ) {return ret; }@@ -390,7 +405,7 @@ out:static int qemu_rbd_co_create (BlockdevCreateOptions *options,Error **errp) { -return qemu_rbd_do_create (options,NULL ,NULL , errp); +return qemu_rbd_do_create (options,NULL , errp); } static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,@@ -443,7 +458,21 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename,
This is the code mapping QemuOpts to QAPI for "qemu-img create" and such.
block/rbd.c static int coroutine_fn qemu_rbd_co_create_opts(const char *filename, · line 462
Quoted 6 lines
block/rbd.c +2
-1loc->image =g_strdup (qdict_get_try_str (options,"image" )); keypairs =qdict_get_try_str (options,"=keyvalue-pairs" ); - ret =qemu_rbd_do_create (create_options, keypairs, password_secret, errp); +/* Always allow the cephx authentication scheme, even if no password-secret +* is given; the key might come from the config file. Without password-secret,
It could also come from a default keyring, couldn't it?
block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Quoted 38 lines
block/rbd.c +16
-4+* also allow none. */ + loc->has_auth_cephx = true; + loc->auth_cephx =g_new0 (RbdAuthCephx,1 ); + +if (password_secret) { + loc->auth_cephx->has_key_secret = true; + loc->auth_cephx->key_secret =g_strdup (password_secret); + }else { + loc->has_auth_none = true; + loc->auth_none = true; + } + + ret =qemu_rbd_do_create (create_options, keypairs, errp);if (ret <0 ) { goto exit; }@@ -538,8 +567,7 @@ static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp)static int qemu_rbd_connect (rados_t *cluster,rados_ioctx_t *io_ctx,BlockdevOptionsRbd *opts,bool cache, -const char *keypairs,const char *secretid, -Error **errp) +const char *keypairs,Error **errp) {char *mon_host =NULL ;Error *local_err =NULL ;@@ -577,8 +605,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, } } -if (qemu_rbd_set_auth (*cluster, secretid, errp) <0 ) { - r = -EIO ; + r =qemu_rbd_set_auth (*cluster, opts, errp); +if (r <0 ) { goto failed_shutdown; }@@ -671,8 +699,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
This is the code mapping QemuOpts to QAPI for -drive and such.
block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 709
Quoted 11 lines
block/rbd.c +8
-0goto out; } +/* Always allow the cephx authentication scheme, even if no password-secret +* is given; the key might come from the config file. Without password-secret, +* also allow none. If the new QAPI-style options are given, don't override +* them, though. */ +if (opts->auth_cephx ==NULL ) { + opts->has_auth_cephx = true; + opts->auth_cephx =g_new0 (RbdAuthCephx,1 ); + }
@auth_cephx defaults to {}.
block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 717
Quoted 8 lines
block/rbd.c +8
-0+ +if (secretid) { +if (opts->auth_cephx->has_key_secret) { +error_setg (errp,"'auth-ceph.key-secret' and its alias " +"'password-secret' can't be used at the " +"same time" ); + r = -EINVAL ; + goto out;
Legacy @password-secret clashes with @auth_cephx.key-secret. Okay.
block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 720
Quoted 3 lines
block/rbd.c +3
-0+ } + opts->auth_cephx->has_key_secret = true; + opts->auth_cephx->key_secret =g_strdup (secretid);
@auth_cephx.key_secret defaults to legacy @password-secret. Okay.
block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 723
Quoted 3 lines
block/rbd.c +3
-0+ }else if (!opts->has_auth_none) { + opts->has_auth_none = true; + opts->auth_none = true;
@auth_none defaults to true.
The QAPI schema documents default false. I'll be hanged if I know what the actual default is for each of -drive, -blockdev, blockdev-add.
How does backward compatibility work?
block/rbd.c static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, · line 730
Quoted 8 lines
block/rbd.c +3
-1+ } + r =qemu_rbd_connect (&s->cluster, &s->io_ctx, opts, - !(flags &BDRV_O_NOCACHE ), keypairs, secretid, errp); + !(flags &BDRV_O_NOCACHE ), keypairs, errp);if (r <0 ) { goto out; }
Did I mention I've forgotten pretty much everything about this stuff? I'm afraid it shows in my review.
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 42 lines
Quoted 3 lines · Kevin Wolf
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
464444fcc161284ac0e743b99251debc297d5236Author: Markus Armbruster <[email protected]> Date: Tue Mar 28 10:56:06 2017 +0200rbd: 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.
Quoted 6 lines
* 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.
Quoted 3 lines
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.
Quoted 47 lines
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 <[email protected]>Reviewed-by Max Reitz <[email protected]>Reviewed-by Eric Blake <[email protected]>Reviewed-by Jeff Cody <[email protected]>Message-id: [email protected]
Signed-off-by Jeff Cody <[email protected]>Quoted 3 lines
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 openingSuggest 'the old "password-secret" command line option parameter'.
Quoted 3 lines
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'.
Quoted 4 lines
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.
Quoted 7 lines
Signed-off-by Kevin Wolf <[email protected]>---
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 7 lines
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:
Quoted 7 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 3 lines
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.hto reduce the temptation to use them outside the block layer.
Unrelated cleanup, but seems reasonable. block/qdict.h?
Quoted 14 lines
Quoted 3 lines
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.
Quoted 2 lines
(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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 20 lines
Quoted 2 lines
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.
Quoted 3 lines
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".
Quoted 7 lines
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.
Quoted 53 lines
Quoted 53 lines
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(-)qapi/block-core.json +22
-0@@ -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'] } }
Quoted 7 lines
qapi/block-core.json line 3222
Quoted 1 line
qapi/block-core.json +0
-0##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.)
Quoted 10 lines
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_requiredto "none"(2) just auth-cephx: we set
auth_client_requiredto "cephx"(3) both: we set
auth_client_requiredto "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...
Quoted 2 lines
(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.
Quoted 11 lines
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 guessRelated: 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.
Quoted 15 lines
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.
Quoted 4 lines
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
Quoted 6 lines · Kevin Wolf
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 3 lines
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.
Quoted 6 lines
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.
Even if the config file does have auth info setup, we can't assume that the QEMU VMs are supposed to use the same auth info. In fact to properly protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user.
So from libvirt POV we want to pretend the config file does not exist at all and explicitly pass everything that is needed via normal per-disk setup for blockdev.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Am 18.04.2018 um 18:34 hat Daniel P. Berrangé geschrieben:
Quoted 20 lines · Kevin Wolf
Quoted 6 lines
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 3 lines
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.
Quoted 6 lines
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.
Even if the config file does have auth info setup, we can't assume that the QEMU VMs are supposed to use the same auth info. In fact to properly protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user.
Quoted 5 lines
So from libvirt POV we want to pretend the config file does not exist at all and explicitly pass everything that is needed via normal per-disk setup for blockdev.
From the rbd driver:
* The "conf" option specifies a Ceph configuration file to read. If
* it is not specified, we will read from the default Ceph locations
* (e.g., /etc/ceph/ceph.conf). To avoid reading _any_ configuration
* file, specify conf=/dev/null.
So what we actually expected libvirt to do is to create a config file for each rbd image and pass that to qemu. However, libvirt allows the user to specify their own config file and passes that, and therefore doesn't want to create its own config file. If the user doesn't specify a config file, libvirt should probably indeed use /dev/null at least.
Kevin
Quoted 32 lines · Kevin Wolf
Am 18.04.2018 um 18:34 hat Daniel P. Berrangé geschrieben:
Quoted 20 lines · Kevin Wolf
Quoted 6 lines
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 3 lines
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.
Quoted 6 lines
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.
Even if the config file does have auth info setup, we can't assume that the QEMU VMs are supposed to use the same auth info. In fact to properly protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user.
Quoted 3 lines
So from libvirt POV we want to pretend the config file does not exist at all and explicitly pass everything that is needed via normal per-disk setup for blockdev.
From the rbd driver:
* The "conf" option specifies a Ceph configuration file to read. If
* it is not specified, we will read from the default Ceph locations
* (e.g.,/etc/ceph/ceph.conf). To avoid reading _any_ configuration
* file, specify conf=/dev/null.
Quoted 5 lines
So what we actually expected libvirt to do is to create a config file for each rbd image and pass that to qemu. However, libvirt allows the user to specify their own config file and passes that, and therefore doesn't want to create its own config file. If the user doesn't specify a config file, libvirt should probably indeed use
/dev/nullat least.
Yeah this is a mess - I wish we had never allowed users to pass a config file, and had used /dev/null all the time. Unfortunately changing either of these aspects would cause backcompat problems for existing deployments now :-( So we just have to accept that the global config file is always in present, but none the less libvirt should try to specify things as fully as possible.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoted 9 lines · Daniel P. Berrangé
Quoted 8 lines · Kevin Wolf
Am 18.04.2018 um 18:34 hat Daniel P. Berrangé geschrieben:
Quoted 7 lines · Kevin Wolf
Quoted 6 lines
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 3 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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.
Yes.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Even if the config file does have auth info setup, we can't assume that the QEMU VMs are supposed to use the same auth info.
If the user tells you to use this config file, you better assume that's what he wants.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In fact to properly
protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user.
Yes, unless the user tells you otherwise.
Quoted 17 lines
Quoted 11 lines
Quoted 3 lines
So from libvirt POV we want to pretend the config file does not exist at all and explicitly pass everything that is needed via normal per-disk setup for blockdev.
From the rbd driver:
* The "conf" option specifies a Ceph configuration file to read. If
* it is not specified, we will read from the default Ceph locations
* (e.g.,/etc/ceph/ceph.conf). To avoid reading _any_ configuration
* file, specify conf=/dev/null.Quoted 5 lines
So what we actually expected libvirt to do is to create a config file for each rbd image and pass that to qemu. However, libvirt allows the user to specify their own config file and passes that, and therefore doesn't want to create its own config file. If the user doesn't specify a config file, libvirt should probably indeed use
/dev/nullat least.
Quoted 6 lines
Yeah this is a mess - I wish we had never allowed users to pass a config file, and had used
/dev/nullall the time. Unfortunately changing either of these aspects would cause backcompat problems for existing deployments now :-( So we just have to accept that the global config file is always in present, but none the less libvirt should try to specify things as fully as possible.
I'm afraid you get backward compatibility problems no matter what. Whenever you extend libvirt to pass configuration C "via normal per-disk setup for blockdev", it breaks user config files containing C, doesn't it?
<heresy>If you're going to break config files on every new feature anyway, why not break them once and for all, then use them the way we actually expected libvirt to do.</heresy>
Quoted 57 lines · Markus Armbruster
Quoted 9 lines · Daniel P. Berrangé
Quoted 8 lines · Kevin Wolf
Am 18.04.2018 um 18:34 hat Daniel P. Berrangé geschrieben:
Quoted 7 lines · Kevin Wolf
Quoted 6 lines
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 3 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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.
Yes.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Even if the config file does have auth info setup, we can't assume that the QEMU VMs are supposed to use the same auth info.
If the user tells you to use this config file, you better assume that's what he wants.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In fact to properly
protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user.Yes, unless the user tells you otherwise.
Quoted 17 lines
Quoted 11 lines
Quoted 3 lines
So from libvirt POV we want to pretend the config file does not exist at all and explicitly pass everything that is needed via normal per-disk setup for blockdev.
From the rbd driver:
* The "conf" option specifies a Ceph configuration file to read. If
* it is not specified, we will read from the default Ceph locations
* (e.g.,/etc/ceph/ceph.conf). To avoid reading _any_ configuration
* file, specify conf=/dev/null.Quoted 5 lines
So what we actually expected libvirt to do is to create a config file for each rbd image and pass that to qemu. However, libvirt allows the user to specify their own config file and passes that, and therefore doesn't want to create its own config file. If the user doesn't specify a config file, libvirt should probably indeed use
/dev/nullat least.Quoted 6 lines
Yeah this is a mess - I wish we had never allowed users to pass a config file, and had used
/dev/nullall the time. Unfortunately changing either of these aspects would cause backcompat problems for existing deployments now :-( So we just have to accept that the global config file is always in present, but none the less libvirt should try to specify things as fully as possible.
Quoted 4 lines
I'm afraid you get backward compatibility problems no matter what. Whenever you extend libvirt to pass configuration C "via normal per-disk setup for blockdev", it breaks user config files containing C, doesn't it?
That's not actually a problem here. We are only passing things to QEMU that the user already provided us in the XML file. If we gain support for passing a new item via the blockdev schema, then we'd only be passing that to QEMU if the user actually provided that item in the XML too. We're not likely to pass a new config field without the user having asked us to first.
In this specific case of passwords, historically the user providd the password in the XML, and we passed that to QEMU using the URI format. We want to change to the blockdev schema instead of URI so we can use secrets.
IOW, if the user has been relying on the global config file to provide passwords QEMU needed, we would never have been setting a password in the URI in the past, and we would also not set a password secret in the blockdev schema either. So there's no compat problem here.
Quoted 3 lines
<heresy>If you're going to break config files on every new feature anyway, why not break them once and for all, then use them the way we actually expected libvirt to do.</heresy>
We're not breaking it, so there's no issue here.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoted 15 lines · Daniel P. Berrangé
Quoted 9 lines · Markus Armbruster
Quoted 6 lines · Daniel P. Berrangé
Yeah this is a mess - I wish we had never allowed users to pass a config file, and had used
/dev/nullall the time. Unfortunately changing either of these aspects would cause backcompat problems for existing deployments now :-( So we just have to accept that the global config file is always in present, but none the less libvirt should try to specify things as fully as possible.Quoted 4 lines
I'm afraid you get backward compatibility problems no matter what. Whenever you extend libvirt to pass configuration C "via normal per-disk setup for blockdev", it breaks user config files containing C, doesn't it?
Quoted 6 lines
That's not actually a problem here. We are only passing things to QEMU that the user already provided us in the XML file. If we gain support for passing a new item via the blockdev schema, then we'd only be passing that to QEMU if the user actually provided that item in the XML too. We're not likely to pass a new config field without the user having asked us to first.
What made me guess otherwise: "to properly protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user" led me to assume libvirt would do this automatically.
[...]
Quoted 24 lines · Markus Armbruster
Quoted 15 lines · Daniel P. Berrangé
Quoted 9 lines · Markus Armbruster
Quoted 6 lines · Daniel P. Berrangé
Yeah this is a mess - I wish we had never allowed users to pass a config file, and had used
/dev/nullall the time. Unfortunately changing either of these aspects would cause backcompat problems for existing deployments now :-( So we just have to accept that the global config file is always in present, but none the less libvirt should try to specify things as fully as possible.Quoted 4 lines
I'm afraid you get backward compatibility problems no matter what. Whenever you extend libvirt to pass configuration C "via normal per-disk setup for blockdev", it breaks user config files containing C, doesn't it?
Quoted 6 lines
That's not actually a problem here. We are only passing things to QEMU that the user already provided us in the XML file. If we gain support for passing a new item via the blockdev schema, then we'd only be passing that to QEMU if the user actually provided that item in the XML too. We're not likely to pass a new config field without the user having asked us to first.
Quoted 5 lines
What made me guess otherwise: "to properly protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user" led me to assume libvirt would do this automatically.
No, a mgmt app like OpenStack would have to take care of that, as it needs the ability to manage the RBD user accounts & volume ACLs, to match the VMs you're creating.
I just meant that even if you have auth info in the global RBD file, we shouldn't assume that auth info is desirable to use with QEMU. The global auth config file may be an administrative account, while each QEMU has its own restricted account.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoted 31 lines · Daniel P. Berrangé
Quoted 24 lines · Markus Armbruster
Quoted 15 lines · Daniel P. Berrangé
Quoted 9 lines · Markus Armbruster
Quoted 6 lines · Daniel P. Berrangé
Yeah this is a mess - I wish we had never allowed users to pass a config file, and had used
/dev/nullall the time. Unfortunately changing either of these aspects would cause backcompat problems for existing deployments now :-( So we just have to accept that the global config file is always in present, but none the less libvirt should try to specify things as fully as possible.Quoted 4 lines
I'm afraid you get backward compatibility problems no matter what. Whenever you extend libvirt to pass configuration C "via normal per-disk setup for blockdev", it breaks user config files containing C, doesn't it?
Quoted 6 lines
That's not actually a problem here. We are only passing things to QEMU that the user already provided us in the XML file. If we gain support for passing a new item via the blockdev schema, then we'd only be passing that to QEMU if the user actually provided that item in the XML too. We're not likely to pass a new config field without the user having asked us to first.
Quoted 5 lines
What made me guess otherwise: "to properly protect against compromised QEMU, ideally every QEMU would use a completely separate RBD user+password, so that compromised QEMU can't then access RBD disks belonging to a different user" led me to assume libvirt would do this automatically.
Quoted 8 lines
No, a mgmt app like OpenStack would have to take care of that, as it needs the ability to manage the RBD user accounts & volume ACLs, to match the VMs you're creating.
I just meant that even if you have auth info in the global RBD file, we shouldn't assume that auth info is desirable to use with QEMU. The global auth config file may be an administrative account, while each QEMU has its own restricted account.
I understand now, thanks.
Quoted 117 lines · Kevin Wolf
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 42 lines
Quoted 3 lines · Kevin Wolf
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
464444fcc161284ac0e743b99251debc297d5236Author: Markus Armbruster <[email protected]> Date: Tue Mar 28 10:56:06 2017 +0200rbd: 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.
Quoted 6 lines
* 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.
Quoted 3 lines
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.
Quoted 53 lines
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 <[email protected]>Reviewed-by Max Reitz <[email protected]>Reviewed-by Eric Blake <[email protected]>Reviewed-by Jeff Cody <[email protected]>Message-id: [email protected]
Signed-off-by Jeff Cody <[email protected]>Quoted 3 lines
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 openingSuggest 'the old "password-secret" command line option parameter'.
Quoted 3 lines
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'.
Quoted 4 lines
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.
Quoted 7 lines
Signed-off-by Kevin Wolf <[email protected]>---
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.
Quoted 5 lines
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.
Quoted 5 lines
Quoted 1 line
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.
I see.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
For historical reasons. If I remember correctly, QemuOpts got there first, QDict was spliced in later.
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
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.hto reduce the temptation to use them outside the block layer.Unrelated cleanup, but seems reasonable.
block/qdict.h?
Works for me. No rush.
Quoted 14 lines
Quoted 14 lines
Quoted 3 lines
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.
Quoted 2 lines
(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.
Quoted 8 lines
Quoted 6 lines
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.
Good.
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
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.
Good.
As mentioned in my reply to Dan, the config file can't serve as stable interface here: we break it whenever we add options to QAPI. Can't see anything we could do but document the issue.
Quoted 26 lines
Quoted 26 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Quoted 1 line
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.
That's fine. We explore the design space, and pick what we hate least. This RFC is a fine start.
Quoted 70 lines
Quoted 53 lines
Quoted 53 lines
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(-)qapi/block-core.json +22
-0@@ -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'] } }Quoted 7 lines
qapi/block-core.json line 3222
Quoted 1 line
qapi/block-core.json +0
-0##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.)Quoted 7 lines
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_requiredto "none"(2) just auth-cephx: we set
auth_client_requiredto "cephx"
Quoted 7 lines
Quoted 3 lines
(3) both: we set
auth_client_requiredto "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...
Right on both counts.
Quoted 6 lines
Quoted 2 lines
(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.
You mean I'm supposed to read the whole thread before I start posting to it?!?
Quoted 9 lines
Quoted 9 lines
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
Quoted 6 lines
Quoted 2 lines
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.
I pestered Jason Dillaman about this stuff back then, and he explained
If the auth is set to none, the user is technically irreverent. I can run a command like "rbd --id this_user_does_not_exist rm image_name" and it will succeed if cephx is disabled on the server.
Off list, Jeff was cc'ed, you weren't.
Quoted 15 lines
Quoted 15 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
For the stringify idea to work, the keyval visitor needs to map the string right back to the original value.
The keyval visitor currently requires the value to be null, not a string.
Therefore, the stringify operation must leave null alone. Not pretty, but works.
You might ask why not change the keyval visitor instead so it expects "" rather than null. That's no good, because it makes StrOrNull ambiguous.
keyval.c can only create string scalars. I think "use JSON if you want to specify null" is still good enough. We can make keyval.c more expressive if we need to, but (1) I don't think we should block this work on it, and (2) see "hairy" above.
Quoted 2 lines
So (A) doesn't work unless we can specify what "other ways" are that are acceptable for libvirt,
Yes.
Quoted 2 lines
and (C) probably doesn't play well with b. above
(theqdict_stringify_entries()for handling mixed type QDicts).
I think it could be done, and tried to sketch how.
Quoted 3 lines
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.
I could accept (B), in particular since it mirrors existing @user.
I'm happy to help with exploring (C).
What's the next step?
Quoted 410 lines · Markus Armbruster
Quoted 117 lines · Kevin Wolf
Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
Quoted 42 lines
Quoted 3 lines · Kevin Wolf
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
464444fcc161284ac0e743b99251debc297d5236Author: Markus Armbruster <[email protected]> Date: Tue Mar 28 10:56:06 2017 +0200rbd: 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.
Quoted 6 lines
* 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.
Quoted 3 lines
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.
Quoted 53 lines
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 <[email protected]>Reviewed-by Max Reitz <[email protected]>Reviewed-by Eric Blake <[email protected]>Reviewed-by Jeff Cody <[email protected]>Message-id: [email protected]
Signed-off-by Jeff Cody <[email protected]>Quoted 3 lines
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 openingSuggest 'the old "password-secret" command line option parameter'.
Quoted 3 lines
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'.
Quoted 4 lines
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.
Quoted 7 lines
Signed-off-by Kevin Wolf <[email protected]>---
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.
Quoted 5 lines
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.Quoted 5 lines
Quoted 1 line
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.
I see.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
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.Quoted 7 lines
Quoted 3 lines
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.
For historical reasons. If I remember correctly, QemuOpts got there first, QDict was spliced in later.
Quoted 11 lines
Quoted 9 lines
Quoted 3 lines
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.hto reduce the temptation to use them outside the block layer.Unrelated cleanup, but seems reasonable.
block/qdict.h?Works for me. No rush.
Quoted 14 lines
Quoted 14 lines
Quoted 3 lines
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.
Quoted 2 lines
(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.
Quoted 8 lines
Quoted 6 lines
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.
Good.
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
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.Good.
As mentioned in my reply to Dan, the config file can't serve as stable interface here: we break it whenever we add options to QAPI. Can't see anything we could do but document the issue.
Quoted 26 lines
Quoted 26 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Quoted 1 line
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.
That's fine. We explore the design space, and pick what we hate least. This RFC is a fine start.
Quoted 70 lines
Quoted 53 lines
Quoted 53 lines
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(-)qapi/block-core.json +22
-0@@ -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'] } }Quoted 7 lines
qapi/block-core.json line 3222
Quoted 1 line
qapi/block-core.json +0
-0##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.)Quoted 7 lines
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_requiredto "none"(2) just auth-cephx: we set
auth_client_requiredto "cephx"Quoted 7 lines
Quoted 3 lines
(3) both: we set
auth_client_requiredto "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...
Right on both counts.
Quoted 6 lines
Quoted 2 lines
(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.
You mean I'm supposed to read the whole thread before I start posting to it?!?
Quoted 9 lines
Quoted 9 lines
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 guessQuoted 6 lines
Quoted 2 lines
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.
I pestered Jason Dillaman about this stuff back then, and he explained
If the auth is set to none, the user is technically irreverent. I can run a command like "rbd --id
this_user_does_not_existrmimage_name" and it will succeed if cephx is disabled on the server.Off list, Jeff was cc'ed, you weren't.
Quoted 15 lines
Quoted 15 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
For the stringify idea to work, the keyval visitor needs to map the string right back to the original value.
The keyval visitor currently requires the value to be null, not a string.
Therefore, the stringify operation must leave null alone. Not pretty, but works.
You might ask why not change the keyval visitor instead so it expects "" rather than null. That's no good, because it makes StrOrNull ambiguous.
keyval.ccan only create string scalars. I think "use JSON if you want to specify null" is still good enough. We can makekeyval.cmore expressive if we need to, but (1) I don't think we should block this work on it, and (2) see "hairy" above.Quoted 2 lines
So (A) doesn't work unless we can specify what "other ways" are that are acceptable for libvirt,
Yes.
Quoted 2 lines
and (C) probably doesn't play well with b. above
(theqdict_stringify_entries()for handling mixed type QDicts).I think it could be done, and tried to sketch how.
Quoted 3 lines
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.
Quoted 5 lines
I could accept (B), in particular since it mirrors existing @user.
I'm happy to help with exploring (C).
What's the next step?
My preference is (B). Primarily because I don't like the idea of breaking dotted key syntax for null keys. I'd rather see something more verbose like (B), that can still be navigated correctly both ways.
How about I put together a patch with (B) for an RFC v2?
-Jeff
Am 24.04.2018 um 20:26 hat Jeff Cody geschrieben:
Quoted 25 lines · Markus Armbruster
Quoted 24 lines
Quoted 23 lines
Quoted 15 lines
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.
Quoted 4 lines
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.
Quoted 8 lines
Quoted 8 lines
For the stringify idea to work, the keyval visitor needs to map the string right back to the original value.
The keyval visitor currently requires the value to be null, not a string.
Therefore, the stringify operation must leave null alone. Not pretty, but works.
Okay, I didn't know that the keyval visitor has any way to specify null. It doesn't really matter what the exact representation is as long as we can generate it. So sure, that's workable then.
Quoted 16 lines
Quoted 16 lines
You might ask why not change the keyval visitor instead so it expects "" rather than null. That's no good, because it makes StrOrNull ambiguous.
keyval.ccan only create string scalars. I think "use JSON if you want to specify null" is still good enough. We can makekeyval.cmore expressive if we need to, but (1) I don't think we should block this work on it, and (2) see "hairy" above.Quoted 2 lines
So (A) doesn't work unless we can specify what "other ways" are that are acceptable for libvirt,
Yes.
Quoted 2 lines
and (C) probably doesn't play well with b. above
(theqdict_stringify_entries()for handling mixed type QDicts).
Quoted 7 lines
Quoted 7 lines
I think it could be done, and tried to sketch how.
Quoted 3 lines
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.
I could accept (B), in particular since it mirrors existing @user.
I don't like (B) much because it adds additional rules which options must, may or can be present depending on the presence or value of other options. This kind of dependencies should be visible in the schema with nested structs and unions, and checked for consistency by QAPI, rather than being checked individually in .bdrv_open() implementations.
As for @user, you had sources to confirm that it is indeed irrelevant for 'none', so I'd rather do the opposite thing and move it to RbdAuthCephx.
Quoted 7 lines
Quoted 3 lines
I'm happy to help with exploring (C).
What's the next step?
My preference is (B). Primarily because I don't like the idea of breaking dotted key syntax for null keys. I'd rather see something more verbose like (B), that can still be navigated correctly both ways.
Yes, it's not perfect, but it doesn't make any functionality unavailable because you can always using JSON, even on the command line. Dotted syntax is nicer for manual use, but in this specific case I think null will be the default, so there is no need to specify it explicitly anyway - neither with dotted key syntax nor with JSON.
I prefer slightly unwieldy command line syntax to getting the internally used data types wrong (= hard to use correctly).
Quoted 1 line
How about I put together a patch with (B) for an RFC v2?
How about doing the same with (C) and moving @user? :-)
Kevin
Quoted 81 lines · Kevin Wolf
Am 24.04.2018 um 20:26 hat Jeff Cody geschrieben:
Quoted 25 lines · Markus Armbruster
Quoted 24 lines
Quoted 23 lines
Quoted 15 lines
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.
Quoted 4 lines
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.
Quoted 8 lines
Quoted 8 lines
For the stringify idea to work, the keyval visitor needs to map the string right back to the original value.
The keyval visitor currently requires the value to be null, not a string.
Therefore, the stringify operation must leave null alone. Not pretty, but works.
Okay, I didn't know that the keyval visitor has any way to specify null. It doesn't really matter what the exact representation is as long as we can generate it. So sure, that's workable then.
Quoted 16 lines
Quoted 16 lines
You might ask why not change the keyval visitor instead so it expects "" rather than null. That's no good, because it makes StrOrNull ambiguous.
keyval.ccan only create string scalars. I think "use JSON if you want to specify null" is still good enough. We can makekeyval.cmore expressive if we need to, but (1) I don't think we should block this work on it, and (2) see "hairy" above.Quoted 2 lines
So (A) doesn't work unless we can specify what "other ways" are that are acceptable for libvirt,
Yes.
Quoted 2 lines
and (C) probably doesn't play well with b. above
(theqdict_stringify_entries()for handling mixed type QDicts).Quoted 7 lines
Quoted 7 lines
I think it could be done, and tried to sketch how.
Quoted 3 lines
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.
I could accept (B), in particular since it mirrors existing @user.
I don't like (B) much because it adds additional rules which options must, may or can be present depending on the presence or value of other options. This kind of dependencies should be visible in the schema with nested structs and unions, and checked for consistency by QAPI, rather than being checked individually in .bdrv_open() implementations.
As for @user, you had sources to confirm that it is indeed irrelevant for 'none', so I'd rather do the opposite thing and move it to RbdAuthCephx.
Quoted 7 lines
Quoted 3 lines
I'm happy to help with exploring (C).
What's the next step?
My preference is (B). Primarily because I don't like the idea of breaking dotted key syntax for null keys. I'd rather see something more verbose like (B), that can still be navigated correctly both ways.
Quoted 6 lines
Yes, it's not perfect, but it doesn't make any functionality unavailable because you can always using JSON, even on the command line. Dotted syntax is nicer for manual use, but in this specific case I think null will be the default, so there is no need to specify it explicitly anyway - neither with dotted key syntax nor with JSON.
Good point on the default case, that essentially negates my concerns.
Quoted 3 lines
I prefer slightly unwieldy command line syntax to getting the internally used data types wrong (= hard to use correctly).
Fair enough.
Quoted 4 lines
Quoted 1 line
How about I put together a patch with (B) for an RFC v2?
How about doing the same with (C) and moving @user? :-)
Sounds like a plan!
-Jeff