qapi/block-core.json | 22 +++++++++++ block/rbd.c | 102 ++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 99 insertions(+), 25 deletions(-)
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 <kwolf@redhat.com>
---
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(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c50517bff3..d5ce588add 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3170,6 +3170,19 @@
##
+# @RbdAuthCephx:
+#
+# @key-secret: ID of a QCryptoSecret object providing a key for cephx
+# authentication. If not specified, a key from the
+# specified configuration file, or the system default
+# configuration is used, if present.
+#
+# Since: 2.13
+##
+{ 'struct': 'RbdAuthCephx',
+ 'data': { '*key-secret': 'str' } }
+
+##
# @BlockdevOptionsRbd:
#
# @pool: Ceph pool name.
@@ -3184,6 +3197,13 @@
#
# @user: Ceph id name.
#
+# @auth-none: true if connecting to a server without authentication
+# should be allowed (default: false; since 2.13)
+#
+# @auth-cephx: Configuration for cephx authentication if specified. If
+# not specified, cephx authentication is not allowed.
+# (since 2.13)
+#
# @server: Monitor host address and port. This maps
# to the "mon_host" Ceph option.
#
@@ -3195,6 +3215,8 @@
'*conf': 'str',
'*snapshot': 'str',
'*user': 'str',
+ '*auth-none': 'bool',
+ '*auth-cephx': 'RbdAuthCephx',
'*server': ['InetSocketAddressBase'] } }
##
diff --git a/block/rbd.c b/block/rbd.c
index 5b64849dc6..c2a9a92dd5 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -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;
+ }
+
+ 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;
}
@@ -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,
loc->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,
+ * 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,
goto 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);
+ }
+
+ 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;
+ }
+ opts->auth_cephx->has_key_secret = true;
+ opts->auth_cephx->key_secret = g_strdup(secretid);
+ } else if (!opts->has_auth_none) {
+ opts->has_auth_none = true;
+ opts->auth_none = true;
+ }
+
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;
}
--
2.13.6
Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
> 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.
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.
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>
> 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.
Kevin
Am 06.04.2018 um 10:04 hat Kevin Wolf geschrieben:
> Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
> > 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.
>
> 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.
>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >
> > 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
On 04/06/2018 03:04 AM, Kevin Wolf wrote:
> Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
>> 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.
>
> 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.
Can we use a QAPI alternate with an explicit JSON null for the command
line 'no options given' case?
>
> I don't think changing the description of the QAPI schema would be a
> good idea, it would be a rather surprising interface.
>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>
>> 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?
>>
>> 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.)
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?
>>
>> 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?
>>
>> Any thoughts on the proposed QAPI schema or the two implementation
>> problems are welcome.
>
> Kevin
>
--
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:
> On 04/06/2018 03:04 AM, Kevin Wolf wrote:
> > Am 05.04.2018 um 19:06 hat Kevin Wolf geschrieben:
> >> 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.
> >
> > 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.
>
> 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).
> >
> > I don't think changing the description of the QAPI schema would be a
> > good idea, it would be a rather surprising interface.
> >
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> ---
> >>
> >> 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.
> >>
> >> 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.)
>
> 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.
> >>
> >> 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?
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
On 04/05/2018 12:06 PM, Kevin Wolf wrote:
> 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 <kwolf@redhat.com>
> ---
> Any thoughts on the proposed QAPI schema or the two implementation
> problems are welcome.
>
> qapi/block-core.json | 22 +++++++++++
> block/rbd.c | 102 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 99 insertions(+), 25 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c50517bff3..d5ce588add 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3170,6 +3170,19 @@
>
>
> ##
> +# @RbdAuthCephx:
> +#
> +# @key-secret: ID of a QCryptoSecret object providing a key for cephx
> +# authentication. If not specified, a key from the
> +# specified configuration file, or the system default
> +# configuration is used, if present.
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'RbdAuthCephx',
> + 'data': { '*key-secret': 'str' } }
> +
> +##
> # @BlockdevOptionsRbd:
> #
> # @pool: Ceph pool name.
> @@ -3184,6 +3197,13 @@
> #
> # @user: Ceph id name.
> #
> +# @auth-none: true if connecting to a server without authentication
> +# should be allowed (default: false; since 2.13)
> +#
> +# @auth-cephx: Configuration for cephx authentication if specified. If
> +# not specified, cephx authentication is not allowed.
> +# (since 2.13)
> +#
> # @server: Monitor host address and port. This maps
> # to the "mon_host" Ceph option.
> #
> @@ -3195,6 +3215,8 @@
> '*conf': 'str',
> '*snapshot': 'str',
> '*user': 'str',
> + '*auth-none': 'bool',
> + '*auth-cephx': 'RbdAuthCephx',
> '*server': ['InetSocketAddressBase'] } }
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?
--
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:
> On 04/05/2018 12:06 PM, Kevin Wolf wrote:
> > 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 <kwolf@redhat.com>
> > ---
>
> > Any thoughts on the proposed QAPI schema or the two implementation
> > problems are welcome.
> >
>
> > qapi/block-core.json | 22 +++++++++++
> > block/rbd.c | 102 ++++++++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 99 insertions(+), 25 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index c50517bff3..d5ce588add 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3170,6 +3170,19 @@
> >
> >
> > ##
> > +# @RbdAuthCephx:
> > +#
> > +# @key-secret: ID of a QCryptoSecret object providing a key for cephx
> > +# authentication. If not specified, a key from the
> > +# specified configuration file, or the system default
> > +# configuration is used, if present.
> > +#
> > +# Since: 2.13
> > +##
> > +{ 'struct': 'RbdAuthCephx',
> > + 'data': { '*key-secret': 'str' } }
> > +
> > +##
> > # @BlockdevOptionsRbd:
> > #
> > # @pool: Ceph pool name.
> > @@ -3184,6 +3197,13 @@
> > #
> > # @user: Ceph id name.
> > #
> > +# @auth-none: true if connecting to a server without authentication
> > +# should be allowed (default: false; since 2.13)
> > +#
> > +# @auth-cephx: Configuration for cephx authentication if specified. If
> > +# not specified, cephx authentication is not allowed.
> > +# (since 2.13)
> > +#
> > # @server: Monitor host address and port. This maps
> > # to the "mon_host" Ceph option.
> > #
> > @@ -3195,6 +3215,8 @@
> > '*conf': 'str',
> > '*snapshot': 'str',
> > '*user': 'str',
> > + '*auth-none': 'bool',
> > + '*auth-cephx': 'RbdAuthCephx',
> > '*server': ['InetSocketAddressBase'] } }
>
> 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
On 04/18/2018 08:50 AM, Kevin Wolf wrote:
>>> @@ -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:
>
> '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?
--
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:
> On 04/18/2018 08:50 AM, Kevin Wolf wrote:
>
> >>> @@ -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:
> >
> > '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?
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
Kevin Wolf <kwolf@redhat.com> writes:
> The legacy command line syntax supports a "password-secret" option that
> allows to pass an authentication key to Ceph. This was not supported in
> QMP so far.
We tried during development of v2.9.0 (commit 0a55679b4a5), but reverted
before the release:
commit 464444fcc161284ac0e743b99251debc297d5236
Author: Markus Armbruster <armbru@redhat.com>
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.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: 1490691368-32099-9-git-send-email-armbru@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
> This patch introduces authentication options in the QAPI schema, makes
> them do the corresponding rados_conf_set() calls and adds compatibility
> code that translates the old "password-secret" option both for opening
Suggest 'the old "password-secret" command line option parameter'.
> and creating images to the new set of options.
>
> Note that the old option didn't allow to explicitly specify the set of
Likewise, 'the old option parameter'.
> allowed authentication schemes. The compatibility code assumes that if
> "password-secret" is given, only the cephx scheme is allowed. If it's
> missing, both none and cephx are allowed because the configuration file
> could still provide a key.
This is a bit terse for readers who aren't familiar with the way things
work now (or have since forgotten pretty much everything about it, like
me).
Perhaps spelling out how the compatibility code translates the old
option parameter to BlockdevOptionsRbd would help.
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>
> This doesn't actually work correctly yet because the way that options
> are passed through the block layer (QAPI -> QemuOpts -> QDict). Before
> we fix or hack around this, let's make sure this is the schema that we
> want.
Makes sense.
> The two known problems are:
>
> 1. QDict *options in qemu_rbd_open() may contain options either in their
> proper QObject types (if passed from blockdev-add) or as strings
> (-drive). Both forms can be mixed in the same options QDict.
Remind me, how can such a mix happen?
> 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.
> 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.
> c. Do a local one-off hack that checks if the entry with the key
> "auth-none" is a QBool, and if so, overwrite it with a string. The
> problem will reappear with the next non-string option.
Defensible only if we're fairly confident such options will remain rare.
> (d. Get rid of the QDict detour and work only with QAPI objects
> everywhere. Support rbd authentication only in QEMU 4.0.)
This is of course the proper solution, but it's a big project that will
take time. The occasional stop gaps are unavoidable. We just need to
take care not to spend all of our cycles on stop gaps, and none on
actual progress.
e. Make the keyval visitor accept scalars other than strings.
More efficient than b., doubt it matters. Complicates the visitor.
Harder to back out than b. when we no longer need it.
I'm leaning towards b.
> 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."
> However, an empty dict cannot currently be represented by flattened
> QDicts.
Flattening a QDict moves nested scalars to the root (with dotted keys),
and drops nested non-scalars, i.e. QDict, QList. A nested empty QDict
(or QList) vanishes without trace.
> We need to find a way to enable this. I think this will be
> externally visible because it directly translates into the dotted
> syntax of -blockdev, so we may want to be careful.
Quote keyval.c:
* Design flaw: there is no way to denote an empty array or non-root
* object. While interpreting "key absent" as empty seems natural
* (removing a key-val from the input string removes the member when
* there are more, so why not when it's the last), it doesn't work:
* "key absent" already means "optional object/array absent", which
* isn't the same as "empty object/array present".
Getting that syntax right was hairy (you'll probably remember the
lengthy e-mail discussions). I'm reluctant to revisit it. We concluded
back then that dotted key syntax capable of expressing arbitrary JSON
wasn't in the cards, but that the cases it can't do are so exotic that
users should just fall back to JSON. And that would be my advice here.
Can we design a schema that avoids this case? Let's see below.
> Any thoughts on the proposed QAPI schema or the two implementation
> problems are welcome.
>
> qapi/block-core.json | 22 +++++++++++
> block/rbd.c | 102 ++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 99 insertions(+), 25 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c50517bff3..d5ce588add 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3170,6 +3170,19 @@
>
>
> ##
> +# @RbdAuthCephx:
> +#
> +# @key-secret: ID of a QCryptoSecret object providing a key for cephx
> +# authentication. If not specified, a key from the
> +# specified configuration file, or the system default
> +# configuration is used, if present.
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'RbdAuthCephx',
> + 'data': { '*key-secret': 'str' } }
> +
> +##
> # @BlockdevOptionsRbd:
> #
> # @pool: Ceph pool name.
> @@ -3184,6 +3197,13 @@
> #
> # @user: Ceph id name.
> #
> +# @auth-none: true if connecting to a server without authentication
> +# should be allowed (default: false; since 2.13)
> +#
> +# @auth-cephx: Configuration for cephx authentication if specified. If
> +# not specified, cephx authentication is not allowed.
> +# (since 2.13)
> +#
> # @server: Monitor host address and port. This maps
> # to the "mon_host" Ceph option.
> #
> @@ -3195,6 +3215,8 @@
> '*conf': 'str',
> '*snapshot': 'str',
> '*user': 'str',
> + '*auth-none': 'bool',
> + '*auth-cephx': 'RbdAuthCephx',
> '*server': ['InetSocketAddressBase'] } }
>
> ##
Two new optional members @auth-none, @auth-cephx.
For backwards compatibility, behavior needs to remain unchanged when
both are absent. What is the behavior we need to preserve? Config
file, then default?
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?
> diff --git a/block/rbd.c b/block/rbd.c
> index 5b64849dc6..c2a9a92dd5 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -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;
> + }
> +
> + 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?
> @@ -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.
> loc->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?
> + * 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.
> goto 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 {}.
> +
> + 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.
> + }
> + 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.
> + } 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?
> + }
> +
> 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:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > The legacy command line syntax supports a "password-secret" option that
> > allows to pass an authentication key to Ceph. This was not supported in
> > QMP so far.
>
> We tried during development of v2.9.0 (commit 0a55679b4a5), but reverted
> before the release:
>
> commit 464444fcc161284ac0e743b99251debc297d5236
> Author: Markus Armbruster <armbru@redhat.com>
> Date: Tue Mar 28 10:56:06 2017 +0200
>
> rbd: Revert -blockdev and -drive parameter auth-supported
>
> This reverts half of commit 0a55679. We're having second thoughts on
> the QAPI schema (and thus the external interface), and haven't reached
> consensus, yet. Issues include:
>
> * The implementation uses deprecated rados_conf_set() key
> "auth_supported". No biggie.
>
> * The implementation makes -drive silently ignore invalid parameters
> "auth" and "auth-supported.*.X" where X isn't "auth". Fixable (in
> fact I'm going to fix similar bugs around parameter server), so
> again no biggie.
>
> * BlockdevOptionsRbd member @password-secret applies only to
> authentication method cephx. Should it be a variant member of
> RbdAuthMethod?
>
> * BlockdevOptionsRbd member @user could apply to both methods cephx
> and none, but I'm not sure it's actually used with none. If it
> isn't, should it be a variant member of RbdAuthMethod?
>
> * The client offers a *set* of authentication methods, not a list.
> Should the methods be optional members of BlockdevOptionsRbd instead
> of members of list @auth-supported? The latter begs the question
> what multiple entries for the same method mean. Trivial question
> now that RbdAuthMethod contains nothing but @type, but less so when
> RbdAuthMethod acquires other members, such the ones discussed above.
>
> * How BlockdevOptionsRbd member @auth-supported interacts with
> settings from a configuration file specified with @conf is
> undocumented. I suspect it's untested, too.
>
> Let's avoid painting ourselves into a corner now, and revert the
> feature for 2.9.
We tried to address all of these points in the schema of this RFC. Maybe
things become easier if we compromise on some.
> Note that users can still configure authentication methods with a
> configuration file. They probably do that anyway if they use Ceph
> outside QEMU as well.
This solution that we originally intended to offer was dismissed by
libvirt as unpractical: libvirt allows the user to specify both a config
file and a key, and if it wanted to use a config file to pass the key,
it would have to create a merged config file and keep it sync with the
user config file at all times. Understandable that they want to avoid
this.
> Further note that this doesn't affect use of key "auth-supported" in
> -drive file=rbd:...:key=value.
>
> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> which is silly. This will be cleaned up shortly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> Message-id: 1490691368-32099-9-git-send-email-armbru@redhat.com
> Signed-off-by: Jeff Cody <jcody@redhat.com>
>
> > This patch introduces authentication options in the QAPI schema, makes
> > them do the corresponding rados_conf_set() calls and adds compatibility
> > code that translates the old "password-secret" option both for opening
>
> Suggest 'the old "password-secret" command line option parameter'.
>
> > and creating images to the new set of options.
> >
> > Note that the old option didn't allow to explicitly specify the set of
>
> Likewise, 'the old option parameter'.
>
> > allowed authentication schemes. The compatibility code assumes that if
> > "password-secret" is given, only the cephx scheme is allowed. If it's
> > missing, both none and cephx are allowed because the configuration file
> > could still provide a key.
>
> This is a bit terse for readers who aren't familiar with the way things
> work now (or have since forgotten pretty much everything about it, like
> me).
>
> Perhaps spelling out how the compatibility code translates the old
> option parameter to BlockdevOptionsRbd would help.
>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >
> > This doesn't actually work correctly yet because the way that options
> > are passed through the block layer (QAPI -> QemuOpts -> QDict). Before
> > we fix or hack around this, let's make sure this is the schema that we
> > want.
>
> Makes sense.
>
> > The two known problems are:
> >
> > 1. QDict *options in qemu_rbd_open() may contain options either in their
> > proper QObject types (if passed from blockdev-add) or as strings
> > (-drive). Both forms can be mixed in the same options QDict.
>
> Remind me, how can such a mix happen?
The way I'm aware of is that you use blockdev-add, so you get the real
types for some options, and then the block layer adds strings for
default values.
> > rbd uses the keyval visitor to convert the options into a QAPI
> > object. This means that it requires all options to be strings. This
> > patch, however, introduces a bool property, so the visitor breaks
> > when it gets its input from blockdev-add.
> >
> > Options to hack around the problem:
> >
> > a. Do an extra conversion step or two and go through QemuOpts like
> > some other block drivers. When I offered something like this to
> > Markus a while ago in a similar case, he rejected the idea.
>
> Going through QemuOpts just to get rid of strings is a bit like going
> down Niagara Falls in a barrel just to get rid of sleepiness: it'll do
> that, sure, but it's probably not all it'll do.
Nice comparison. I tend to agree, though there is much more precedence
for this one than for all other options. Almost all block drivers use
QemuOpts to parse their driver-specific options from the QDict.
> > b. Introduce a qdict_stringify_entries() that just does what its name
> > says. It would be called before the running keyval visitor so that
> > only strings will be present in the QDict.
>
> Precedence: a bunch of other QDict operations that are used only by the
> block layer. One more won't make a difference, I guess.
>
> Aside: I'm tempted to move them out of qdict.h to reduce the temptation
> to use them outside the block layer.
Unrelated cleanup, but seems reasonable. block/qdict.h?
> > c. Do a local one-off hack that checks if the entry with the key
> > "auth-none" is a QBool, and if so, overwrite it with a string. The
> > problem will reappear with the next non-string option.
>
> Defensible only if we're fairly confident such options will remain rare.
>
> > (d. Get rid of the QDict detour and work only with QAPI objects
> > everywhere. Support rbd authentication only in QEMU 4.0.)
>
> This is of course the proper solution, but it's a big project that will
> take time. The occasional stop gaps are unavoidable. We just need to
> take care not to spend all of our cycles on stop gaps, and none on
> actual progress.
>
> e. Make the keyval visitor accept scalars other than strings.
>
> More efficient than b., doubt it matters. Complicates the visitor.
> Harder to back out than b. when we no longer need it.
>
> I'm leaning towards b.
Okay, that's what I was leaning towards, too.
> > 2. The proposed schema allows 'auth-cephx': {} as a valid option with
> > the meaning that the cephx authentication scheme is enabled, but no
> > key is given (e.g. it is taken from the config file).
>
> Apropos config file: we need to be careful to maintain the QAPI schema's
> promise that "Values in the configuration file will be overridden by
> options specified via QAPI."
Yes, though it's made a bit easier by the fact that most options are
implemented with a rados_conf_set() call. If you call the function, you
override the config, if not, you don't.
> > However, an empty dict cannot currently be represented by flattened
> > QDicts.
>
> Flattening a QDict moves nested scalars to the root (with dotted keys),
> and drops nested non-scalars, i.e. QDict, QList. A nested empty QDict
> (or QList) vanishes without trace.
>
> > We need to find a way to enable this. I think this will be
> > externally visible because it directly translates into the dotted
> > syntax of -blockdev, so we may want to be careful.
>
> Quote keyval.c:
>
> * Design flaw: there is no way to denote an empty array or non-root
> * object. While interpreting "key absent" as empty seems natural
> * (removing a key-val from the input string removes the member when
> * there are more, so why not when it's the last), it doesn't work:
> * "key absent" already means "optional object/array absent", which
> * isn't the same as "empty object/array present".
>
> Getting that syntax right was hairy (you'll probably remember the
> lengthy e-mail discussions). I'm reluctant to revisit it. We concluded
> back then that dotted key syntax capable of expressing arbitrary JSON
> wasn't in the cards, but that the cases it can't do are so exotic that
> users should just fall back to JSON. And that would be my advice here.
>
> Can we design a schema that avoids this case? Let's see below.
Can we design such a schema? Yes.
Will we like it enough to accept it as a compromise? Maybe.
> > Any thoughts on the proposed QAPI schema or the two implementation
> > problems are welcome.
> >
> > qapi/block-core.json | 22 +++++++++++
> > block/rbd.c | 102 ++++++++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 99 insertions(+), 25 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index c50517bff3..d5ce588add 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3170,6 +3170,19 @@
> >
> >
> > ##
> > +# @RbdAuthCephx:
> > +#
> > +# @key-secret: ID of a QCryptoSecret object providing a key for cephx
> > +# authentication. If not specified, a key from the
> > +# specified configuration file, or the system default
> > +# configuration is used, if present.
> > +#
> > +# Since: 2.13
> > +##
> > +{ 'struct': 'RbdAuthCephx',
> > + 'data': { '*key-secret': 'str' } }
> > +
> > +##
> > # @BlockdevOptionsRbd:
> > #
> > # @pool: Ceph pool name.
> > @@ -3184,6 +3197,13 @@
> > #
> > # @user: Ceph id name.
> > #
> > +# @auth-none: true if connecting to a server without authentication
> > +# should be allowed (default: false; since 2.13)
> > +#
> > +# @auth-cephx: Configuration for cephx authentication if specified. If
> > +# not specified, cephx authentication is not allowed.
> > +# (since 2.13)
> > +#
> > # @server: Monitor host address and port. This maps
> > # to the "mon_host" Ceph option.
> > #
> > @@ -3195,6 +3215,8 @@
> > '*conf': 'str',
> > '*snapshot': 'str',
> > '*user': 'str',
> > + '*auth-none': 'bool',
> > + '*auth-cephx': 'RbdAuthCephx',
> > '*server': ['InetSocketAddressBase'] } }
> >
> > ##
>
> Two new optional members @auth-none, @auth-cephx.
>
> For backwards compatibility, behavior needs to remain unchanged when
> both are absent. What is the behavior we need to preserve? Config
> file, then default?
Yes. (rados_conf_set("auth_client_required") is not called at all.)
> The schema permits four cases, which get translated to an auth client
> required setting by qemu_rbd_set_auth(), visible below:
>
> (1) just auth-none: we set auth_client_required to "none"
>
> (2) just auth-cephx: we set auth_client_required to "cephx"
>
> (3) both: we set auth_client_required to "none;cephx", which I can't
> find in the documentation right now, but according to my notes means
> "either method"
The state of the Ceph documentation indeed wasn't very helpful. On the
other hand, I'm afraid we're not in a position to complain about bad
documentation...
> (4) neither: rejected with "No authentication scheme allowed"
> Uh, why isn't this a compatibility break? Or am I confused?
It is, and that is one of the reasons why this patch is broken. I failed
to mention this in the commit message, but I replied to the patch the
next day to add this.
> When auth-cephx is present, we get subcases
>
> (2a) and (3a) key-secret present: key is in the QCryptoSecret named, and
> we set
>
> (2b) and (3b) key-secret absent: key must be provided some other way,
> say in a configuration file, default keyring, whatever, or else
> we'll fail somehow, I guess
>
> Related: existing BlockdevOptionsRbd member @user. As far as I know,
> it's meaningless with auth-none.
I don't know otherwise and suspect you are right, but Jeff and I
couldn't find any definite confirmation for this assumption, so we left
it where it is instead of moving it into RbdAuthCephx.
> Ways to avoid the troublesome auth-cephx: {}:
>
> (A) Make auth-cephx a bool, rely on the other ways to provide the key
>
> (B) Make auth-cephx a bool and add the @key-secret member next to it
>
> Like @user, the member is meaningless with auth-none.
>
> (C) Make auth-cephx.key-secret a mandatory StrOrNull
>
> Should take care of "vanishes on flattening" problem, but dotted key
> syntax is still screwed: since any value is a valid string, there's
> none left for null. My take would be that if you want to say
> auth-cephx.key-secret: {}, you get to say it in JSON.
>
> Aside: check_alternate() tries to catch such alternates, but we
> failed to update it when we added first class null. *sigh*
>
> Which one do you hate least?
What should happen with null when you stringify it? If we want to take
the options QDict, stringify all entries and run them through the keyval
visitor, I'm almost sure that null will be lost.
So (A) doesn't work unless we can specify what "other ways" are that are
acceptable for libvirt, and (C) probably doesn't play well with b. above
(the qdict_stringify_entries() for handling mixed type QDicts).
Looks like only (B) is left as viable, so that's automatically the one I
hate least of these. If we can fix the problems, I think I'd prefer (C),
though.
Kevin
On Wed, Apr 18, 2018 at 06:28:23PM +0200, Kevin Wolf wrote: > Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben: > > 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. 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 -- |: 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: > On Wed, Apr 18, 2018 at 06:28:23PM +0200, Kevin Wolf wrote: > > Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben: > > > > 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. > > 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. 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
On Wed, Apr 18, 2018 at 06:52:08PM +0200, Kevin Wolf wrote: > Am 18.04.2018 um 18:34 hat Daniel P. Berrangé geschrieben: > > On Wed, Apr 18, 2018 at 06:28:23PM +0200, Kevin Wolf wrote: > > > Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben: > > > > > > 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. > > > > 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. > > 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. 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 -- |: 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 :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Apr 18, 2018 at 06:52:08PM +0200, Kevin Wolf wrote: >> Am 18.04.2018 um 18:34 hat Daniel P. Berrangé geschrieben: >> > On Wed, Apr 18, 2018 at 06:28:23PM +0200, Kevin Wolf wrote: >> > > Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben: >> > >> > > > 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. Yes. >> > 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. >> > 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. >> > 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. > > 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. 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>
On Fri, Apr 20, 2018 at 03:34:26PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Wed, Apr 18, 2018 at 06:52:08PM +0200, Kevin Wolf wrote: > >> Am 18.04.2018 um 18:34 hat Daniel P. Berrangé geschrieben: > >> > On Wed, Apr 18, 2018 at 06:28:23PM +0200, Kevin Wolf wrote: > >> > > Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben: > >> > > >> > > > 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. > > Yes. > > >> > 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. > > >> > 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. > > >> > 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. > > > > 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. > > 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. > <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 -- |: 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 :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Apr 20, 2018 at 03:34:26PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > 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. >> >> 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. 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. [...]
On Fri, Apr 20, 2018 at 04:50:38PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Fri, Apr 20, 2018 at 03:34:26PM +0200, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> > >> > 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. > >> > >> 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. > > 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 -- |: 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 :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Apr 20, 2018 at 04:50:38PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Fri, Apr 20, 2018 at 03:34:26PM +0200, Markus Armbruster wrote: >> >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> >> >> > 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. >> >> >> >> 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. >> >> 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. I understand now, thanks.
Kevin Wolf <kwolf@redhat.com> writes:
> Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > The legacy command line syntax supports a "password-secret" option that
>> > allows to pass an authentication key to Ceph. This was not supported in
>> > QMP so far.
>>
>> We tried during development of v2.9.0 (commit 0a55679b4a5), but reverted
>> before the release:
>>
>> commit 464444fcc161284ac0e743b99251debc297d5236
>> Author: Markus Armbruster <armbru@redhat.com>
>> Date: Tue Mar 28 10:56:06 2017 +0200
>>
>> rbd: Revert -blockdev and -drive parameter auth-supported
>>
>> This reverts half of commit 0a55679. We're having second thoughts on
>> the QAPI schema (and thus the external interface), and haven't reached
>> consensus, yet. Issues include:
>>
>> * The implementation uses deprecated rados_conf_set() key
>> "auth_supported". No biggie.
>>
>> * The implementation makes -drive silently ignore invalid parameters
>> "auth" and "auth-supported.*.X" where X isn't "auth". Fixable (in
>> fact I'm going to fix similar bugs around parameter server), so
>> again no biggie.
>>
>> * BlockdevOptionsRbd member @password-secret applies only to
>> authentication method cephx. Should it be a variant member of
>> RbdAuthMethod?
>>
>> * BlockdevOptionsRbd member @user could apply to both methods cephx
>> and none, but I'm not sure it's actually used with none. If it
>> isn't, should it be a variant member of RbdAuthMethod?
>>
>> * The client offers a *set* of authentication methods, not a list.
>> Should the methods be optional members of BlockdevOptionsRbd instead
>> of members of list @auth-supported? The latter begs the question
>> what multiple entries for the same method mean. Trivial question
>> now that RbdAuthMethod contains nothing but @type, but less so when
>> RbdAuthMethod acquires other members, such the ones discussed above.
>>
>> * How BlockdevOptionsRbd member @auth-supported interacts with
>> settings from a configuration file specified with @conf is
>> undocumented. I suspect it's untested, too.
>>
>> Let's avoid painting ourselves into a corner now, and revert the
>> feature for 2.9.
>
> We tried to address all of these points in the schema of this RFC. Maybe
> things become easier if we compromise on some.
>
>> Note that users can still configure authentication methods with a
>> configuration file. They probably do that anyway if they use Ceph
>> outside QEMU as well.
>
> This solution that we originally intended to offer was dismissed by
> libvirt as unpractical: libvirt allows the user to specify both a config
> file and a key, and if it wanted to use a config file to pass the key,
> it would have to create a merged config file and keep it sync with the
> user config file at all times. Understandable that they want to avoid
> this.
>
>> Further note that this doesn't affect use of key "auth-supported" in
>> -drive file=rbd:...:key=value.
>>
>> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
>> which is silly. This will be cleaned up shortly.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Jeff Cody <jcody@redhat.com>
>> Message-id: 1490691368-32099-9-git-send-email-armbru@redhat.com
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>>
>> > This patch introduces authentication options in the QAPI schema, makes
>> > them do the corresponding rados_conf_set() calls and adds compatibility
>> > code that translates the old "password-secret" option both for opening
>>
>> Suggest 'the old "password-secret" command line option parameter'.
>>
>> > and creating images to the new set of options.
>> >
>> > Note that the old option didn't allow to explicitly specify the set of
>>
>> Likewise, 'the old option parameter'.
>>
>> > allowed authentication schemes. The compatibility code assumes that if
>> > "password-secret" is given, only the cephx scheme is allowed. If it's
>> > missing, both none and cephx are allowed because the configuration file
>> > could still provide a key.
>>
>> This is a bit terse for readers who aren't familiar with the way things
>> work now (or have since forgotten pretty much everything about it, like
>> me).
>>
>> Perhaps spelling out how the compatibility code translates the old
>> option parameter to BlockdevOptionsRbd would help.
>>
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >
>> > This doesn't actually work correctly yet because the way that options
>> > are passed through the block layer (QAPI -> QemuOpts -> QDict). Before
>> > we fix or hack around this, let's make sure this is the schema that we
>> > want.
>>
>> Makes sense.
>>
>> > The two known problems are:
>> >
>> > 1. QDict *options in qemu_rbd_open() may contain options either in their
>> > proper QObject types (if passed from blockdev-add) or as strings
>> > (-drive). Both forms can be mixed in the same options QDict.
>>
>> Remind me, how can such a mix happen?
>
> The way I'm aware of is that you use blockdev-add, so you get the real
> types for some options, and then the block layer adds strings for
> default values.
I see.
>> > rbd uses the keyval visitor to convert the options into a QAPI
>> > object. This means that it requires all options to be strings. This
>> > patch, however, introduces a bool property, so the visitor breaks
>> > when it gets its input from blockdev-add.
>> >
>> > Options to hack around the problem:
>> >
>> > a. Do an extra conversion step or two and go through QemuOpts like
>> > some other block drivers. When I offered something like this to
>> > Markus a while ago in a similar case, he rejected the idea.
>>
>> Going through QemuOpts just to get rid of strings is a bit like going
>> down Niagara Falls in a barrel just to get rid of sleepiness: it'll do
>> that, sure, but it's probably not all it'll do.
>
> Nice comparison. I tend to agree, though there is much more precedence
> for this one than for all other options. Almost all block drivers use
> QemuOpts to parse their driver-specific options from the QDict.
For historical reasons. If I remember correctly, QemuOpts got there
first, QDict was spliced in later.
>> > b. Introduce a qdict_stringify_entries() that just does what its name
>> > says. It would be called before the running keyval visitor so that
>> > only strings will be present in the QDict.
>>
>> Precedence: a bunch of other QDict operations that are used only by the
>> block layer. One more won't make a difference, I guess.
>>
>> Aside: I'm tempted to move them out of qdict.h to reduce the temptation
>> to use them outside the block layer.
>
> Unrelated cleanup, but seems reasonable. block/qdict.h?
Works for me. No rush.
>> > c. Do a local one-off hack that checks if the entry with the key
>> > "auth-none" is a QBool, and if so, overwrite it with a string. The
>> > problem will reappear with the next non-string option.
>>
>> Defensible only if we're fairly confident such options will remain rare.
>>
>> > (d. Get rid of the QDict detour and work only with QAPI objects
>> > everywhere. Support rbd authentication only in QEMU 4.0.)
>>
>> This is of course the proper solution, but it's a big project that will
>> take time. The occasional stop gaps are unavoidable. We just need to
>> take care not to spend all of our cycles on stop gaps, and none on
>> actual progress.
>>
>> e. Make the keyval visitor accept scalars other than strings.
>>
>> More efficient than b., doubt it matters. Complicates the visitor.
>> Harder to back out than b. when we no longer need it.
>>
>> I'm leaning towards b.
>
> Okay, that's what I was leaning towards, too.
Good.
>> > 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.
>> > However, an empty dict cannot currently be represented by flattened
>> > QDicts.
>>
>> Flattening a QDict moves nested scalars to the root (with dotted keys),
>> and drops nested non-scalars, i.e. QDict, QList. A nested empty QDict
>> (or QList) vanishes without trace.
>>
>> > We need to find a way to enable this. I think this will be
>> > externally visible because it directly translates into the dotted
>> > syntax of -blockdev, so we may want to be careful.
>>
>> Quote keyval.c:
>>
>> * Design flaw: there is no way to denote an empty array or non-root
>> * object. While interpreting "key absent" as empty seems natural
>> * (removing a key-val from the input string removes the member when
>> * there are more, so why not when it's the last), it doesn't work:
>> * "key absent" already means "optional object/array absent", which
>> * isn't the same as "empty object/array present".
>>
>> Getting that syntax right was hairy (you'll probably remember the
>> lengthy e-mail discussions). I'm reluctant to revisit it. We concluded
>> back then that dotted key syntax capable of expressing arbitrary JSON
>> wasn't in the cards, but that the cases it can't do are so exotic that
>> users should just fall back to JSON. And that would be my advice here.
>>
>> Can we design a schema that avoids this case? Let's see below.
>
> Can we design such a schema? Yes.
>
> Will we like it enough to accept it as a compromise? Maybe.
That's fine. We explore the design space, and pick what we hate least.
This RFC is a fine start.
>> > Any thoughts on the proposed QAPI schema or the two implementation
>> > problems are welcome.
>> >
>> > qapi/block-core.json | 22 +++++++++++
>> > block/rbd.c | 102 ++++++++++++++++++++++++++++++++++++++-------------
>> > 2 files changed, 99 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index c50517bff3..d5ce588add 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -3170,6 +3170,19 @@
>> >
>> >
>> > ##
>> > +# @RbdAuthCephx:
>> > +#
>> > +# @key-secret: ID of a QCryptoSecret object providing a key for cephx
>> > +# authentication. If not specified, a key from the
>> > +# specified configuration file, or the system default
>> > +# configuration is used, if present.
>> > +#
>> > +# Since: 2.13
>> > +##
>> > +{ 'struct': 'RbdAuthCephx',
>> > + 'data': { '*key-secret': 'str' } }
>> > +
>> > +##
>> > # @BlockdevOptionsRbd:
>> > #
>> > # @pool: Ceph pool name.
>> > @@ -3184,6 +3197,13 @@
>> > #
>> > # @user: Ceph id name.
>> > #
>> > +# @auth-none: true if connecting to a server without authentication
>> > +# should be allowed (default: false; since 2.13)
>> > +#
>> > +# @auth-cephx: Configuration for cephx authentication if specified. If
>> > +# not specified, cephx authentication is not allowed.
>> > +# (since 2.13)
>> > +#
>> > # @server: Monitor host address and port. This maps
>> > # to the "mon_host" Ceph option.
>> > #
>> > @@ -3195,6 +3215,8 @@
>> > '*conf': 'str',
>> > '*snapshot': 'str',
>> > '*user': 'str',
>> > + '*auth-none': 'bool',
>> > + '*auth-cephx': 'RbdAuthCephx',
>> > '*server': ['InetSocketAddressBase'] } }
>> >
>> > ##
>>
>> Two new optional members @auth-none, @auth-cephx.
>>
>> For backwards compatibility, behavior needs to remain unchanged when
>> both are absent. What is the behavior we need to preserve? Config
>> file, then default?
>
> Yes. (rados_conf_set("auth_client_required") is not called at all.)
>
>> The schema permits four cases, which get translated to an auth client
>> required setting by qemu_rbd_set_auth(), visible below:
>>
>> (1) just auth-none: we set auth_client_required to "none"
>>
>> (2) just auth-cephx: we set auth_client_required to "cephx"
>>
>> (3) both: we set auth_client_required to "none;cephx", which I can't
>> find in the documentation right now, but according to my notes means
>> "either method"
>
> The state of the Ceph documentation indeed wasn't very helpful. On the
> other hand, I'm afraid we're not in a position to complain about bad
> documentation...
Right on both counts.
>> (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?!?
>> When auth-cephx is present, we get subcases
>>
>> (2a) and (3a) key-secret present: key is in the QCryptoSecret named, and
>> we set
>>
>> (2b) and (3b) key-secret absent: key must be provided some other way,
>> say in a configuration file, default keyring, whatever, or else
>> we'll fail somehow, I guess
>>
>> Related: existing BlockdevOptionsRbd member @user. As far as I know,
>> it's meaningless with auth-none.
>
> I don't know otherwise and suspect you are right, but Jeff and I
> couldn't find any definite confirmation for this assumption, so we left
> it where it is instead of moving it into RbdAuthCephx.
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.
>> Ways to avoid the troublesome auth-cephx: {}:
>>
>> (A) Make auth-cephx a bool, rely on the other ways to provide the key
>>
>> (B) Make auth-cephx a bool and add the @key-secret member next to it
>>
>> Like @user, the member is meaningless with auth-none.
>>
>> (C) Make auth-cephx.key-secret a mandatory StrOrNull
>>
>> Should take care of "vanishes on flattening" problem, but dotted key
>> syntax is still screwed: since any value is a valid string, there's
>> none left for null. My take would be that if you want to say
>> auth-cephx.key-secret: {}, you get to say it in JSON.
>>
>> Aside: check_alternate() tries to catch such alternates, but we
>> failed to update it when we added first class null. *sigh*
>>
>> Which one do you hate least?
>
> What should happen with null when you stringify it? If we want to take
> the options QDict, stringify all entries and run them through the keyval
> visitor, I'm almost sure that null will be lost.
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.
> So (A) doesn't work unless we can specify what "other ways" are that are
> acceptable for libvirt,
Yes.
> and (C) probably doesn't play well with b. above
> (the qdict_stringify_entries() for handling mixed type QDicts).
I think it could be done, and tried to sketch how.
> 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?
On Fri, Apr 20, 2018 at 04:39:02PM +0200, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 18.04.2018 um 17:06 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >> > The legacy command line syntax supports a "password-secret" option that
> >> > allows to pass an authentication key to Ceph. This was not supported in
> >> > QMP so far.
> >>
> >> We tried during development of v2.9.0 (commit 0a55679b4a5), but reverted
> >> before the release:
> >>
> >> commit 464444fcc161284ac0e743b99251debc297d5236
> >> Author: Markus Armbruster <armbru@redhat.com>
> >> Date: Tue Mar 28 10:56:06 2017 +0200
> >>
> >> rbd: Revert -blockdev and -drive parameter auth-supported
> >>
> >> This reverts half of commit 0a55679. We're having second thoughts on
> >> the QAPI schema (and thus the external interface), and haven't reached
> >> consensus, yet. Issues include:
> >>
> >> * The implementation uses deprecated rados_conf_set() key
> >> "auth_supported". No biggie.
> >>
> >> * The implementation makes -drive silently ignore invalid parameters
> >> "auth" and "auth-supported.*.X" where X isn't "auth". Fixable (in
> >> fact I'm going to fix similar bugs around parameter server), so
> >> again no biggie.
> >>
> >> * BlockdevOptionsRbd member @password-secret applies only to
> >> authentication method cephx. Should it be a variant member of
> >> RbdAuthMethod?
> >>
> >> * BlockdevOptionsRbd member @user could apply to both methods cephx
> >> and none, but I'm not sure it's actually used with none. If it
> >> isn't, should it be a variant member of RbdAuthMethod?
> >>
> >> * The client offers a *set* of authentication methods, not a list.
> >> Should the methods be optional members of BlockdevOptionsRbd instead
> >> of members of list @auth-supported? The latter begs the question
> >> what multiple entries for the same method mean. Trivial question
> >> now that RbdAuthMethod contains nothing but @type, but less so when
> >> RbdAuthMethod acquires other members, such the ones discussed above.
> >>
> >> * How BlockdevOptionsRbd member @auth-supported interacts with
> >> settings from a configuration file specified with @conf is
> >> undocumented. I suspect it's untested, too.
> >>
> >> Let's avoid painting ourselves into a corner now, and revert the
> >> feature for 2.9.
> >
> > We tried to address all of these points in the schema of this RFC. Maybe
> > things become easier if we compromise on some.
> >
> >> Note that users can still configure authentication methods with a
> >> configuration file. They probably do that anyway if they use Ceph
> >> outside QEMU as well.
> >
> > This solution that we originally intended to offer was dismissed by
> > libvirt as unpractical: libvirt allows the user to specify both a config
> > file and a key, and if it wanted to use a config file to pass the key,
> > it would have to create a merged config file and keep it sync with the
> > user config file at all times. Understandable that they want to avoid
> > this.
> >
> >> Further note that this doesn't affect use of key "auth-supported" in
> >> -drive file=rbd:...:key=value.
> >>
> >> qemu_rbd_array_opts()'s parameter @type now must be RBD_MON_HOST,
> >> which is silly. This will be cleaned up shortly.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Reviewed-by: Jeff Cody <jcody@redhat.com>
> >> Message-id: 1490691368-32099-9-git-send-email-armbru@redhat.com
> >> Signed-off-by: Jeff Cody <jcody@redhat.com>
> >>
> >> > This patch introduces authentication options in the QAPI schema, makes
> >> > them do the corresponding rados_conf_set() calls and adds compatibility
> >> > code that translates the old "password-secret" option both for opening
> >>
> >> Suggest 'the old "password-secret" command line option parameter'.
> >>
> >> > and creating images to the new set of options.
> >> >
> >> > Note that the old option didn't allow to explicitly specify the set of
> >>
> >> Likewise, 'the old option parameter'.
> >>
> >> > allowed authentication schemes. The compatibility code assumes that if
> >> > "password-secret" is given, only the cephx scheme is allowed. If it's
> >> > missing, both none and cephx are allowed because the configuration file
> >> > could still provide a key.
> >>
> >> This is a bit terse for readers who aren't familiar with the way things
> >> work now (or have since forgotten pretty much everything about it, like
> >> me).
> >>
> >> Perhaps spelling out how the compatibility code translates the old
> >> option parameter to BlockdevOptionsRbd would help.
> >>
> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> > ---
> >> >
> >> > This doesn't actually work correctly yet because the way that options
> >> > are passed through the block layer (QAPI -> QemuOpts -> QDict). Before
> >> > we fix or hack around this, let's make sure this is the schema that we
> >> > want.
> >>
> >> Makes sense.
> >>
> >> > The two known problems are:
> >> >
> >> > 1. QDict *options in qemu_rbd_open() may contain options either in their
> >> > proper QObject types (if passed from blockdev-add) or as strings
> >> > (-drive). Both forms can be mixed in the same options QDict.
> >>
> >> Remind me, how can such a mix happen?
> >
> > The way I'm aware of is that you use blockdev-add, so you get the real
> > types for some options, and then the block layer adds strings for
> > default values.
>
> I see.
>
> >> > rbd uses the keyval visitor to convert the options into a QAPI
> >> > object. This means that it requires all options to be strings. This
> >> > patch, however, introduces a bool property, so the visitor breaks
> >> > when it gets its input from blockdev-add.
> >> >
> >> > Options to hack around the problem:
> >> >
> >> > a. Do an extra conversion step or two and go through QemuOpts like
> >> > some other block drivers. When I offered something like this to
> >> > Markus a while ago in a similar case, he rejected the idea.
> >>
> >> Going through QemuOpts just to get rid of strings is a bit like going
> >> down Niagara Falls in a barrel just to get rid of sleepiness: it'll do
> >> that, sure, but it's probably not all it'll do.
> >
> > Nice comparison. I tend to agree, though there is much more precedence
> > for this one than for all other options. Almost all block drivers use
> > QemuOpts to parse their driver-specific options from the QDict.
>
> For historical reasons. If I remember correctly, QemuOpts got there
> first, QDict was spliced in later.
>
> >> > b. Introduce a qdict_stringify_entries() that just does what its name
> >> > says. It would be called before the running keyval visitor so that
> >> > only strings will be present in the QDict.
> >>
> >> Precedence: a bunch of other QDict operations that are used only by the
> >> block layer. One more won't make a difference, I guess.
> >>
> >> Aside: I'm tempted to move them out of qdict.h to reduce the temptation
> >> to use them outside the block layer.
> >
> > Unrelated cleanup, but seems reasonable. block/qdict.h?
>
> Works for me. No rush.
>
> >> > c. Do a local one-off hack that checks if the entry with the key
> >> > "auth-none" is a QBool, and if so, overwrite it with a string. The
> >> > problem will reappear with the next non-string option.
> >>
> >> Defensible only if we're fairly confident such options will remain rare.
> >>
> >> > (d. Get rid of the QDict detour and work only with QAPI objects
> >> > everywhere. Support rbd authentication only in QEMU 4.0.)
> >>
> >> This is of course the proper solution, but it's a big project that will
> >> take time. The occasional stop gaps are unavoidable. We just need to
> >> take care not to spend all of our cycles on stop gaps, and none on
> >> actual progress.
> >>
> >> e. Make the keyval visitor accept scalars other than strings.
> >>
> >> More efficient than b., doubt it matters. Complicates the visitor.
> >> Harder to back out than b. when we no longer need it.
> >>
> >> I'm leaning towards b.
> >
> > Okay, that's what I was leaning towards, too.
>
> Good.
>
> >> > 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.
>
> >> > However, an empty dict cannot currently be represented by flattened
> >> > QDicts.
> >>
> >> Flattening a QDict moves nested scalars to the root (with dotted keys),
> >> and drops nested non-scalars, i.e. QDict, QList. A nested empty QDict
> >> (or QList) vanishes without trace.
> >>
> >> > We need to find a way to enable this. I think this will be
> >> > externally visible because it directly translates into the dotted
> >> > syntax of -blockdev, so we may want to be careful.
> >>
> >> Quote keyval.c:
> >>
> >> * Design flaw: there is no way to denote an empty array or non-root
> >> * object. While interpreting "key absent" as empty seems natural
> >> * (removing a key-val from the input string removes the member when
> >> * there are more, so why not when it's the last), it doesn't work:
> >> * "key absent" already means "optional object/array absent", which
> >> * isn't the same as "empty object/array present".
> >>
> >> Getting that syntax right was hairy (you'll probably remember the
> >> lengthy e-mail discussions). I'm reluctant to revisit it. We concluded
> >> back then that dotted key syntax capable of expressing arbitrary JSON
> >> wasn't in the cards, but that the cases it can't do are so exotic that
> >> users should just fall back to JSON. And that would be my advice here.
> >>
> >> Can we design a schema that avoids this case? Let's see below.
> >
> > Can we design such a schema? Yes.
> >
> > Will we like it enough to accept it as a compromise? Maybe.
>
> That's fine. We explore the design space, and pick what we hate least.
> This RFC is a fine start.
>
> >> > Any thoughts on the proposed QAPI schema or the two implementation
> >> > problems are welcome.
> >> >
> >> > qapi/block-core.json | 22 +++++++++++
> >> > block/rbd.c | 102 ++++++++++++++++++++++++++++++++++++++-------------
> >> > 2 files changed, 99 insertions(+), 25 deletions(-)
> >> >
> >> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> > index c50517bff3..d5ce588add 100644
> >> > --- a/qapi/block-core.json
> >> > +++ b/qapi/block-core.json
> >> > @@ -3170,6 +3170,19 @@
> >> >
> >> >
> >> > ##
> >> > +# @RbdAuthCephx:
> >> > +#
> >> > +# @key-secret: ID of a QCryptoSecret object providing a key for cephx
> >> > +# authentication. If not specified, a key from the
> >> > +# specified configuration file, or the system default
> >> > +# configuration is used, if present.
> >> > +#
> >> > +# Since: 2.13
> >> > +##
> >> > +{ 'struct': 'RbdAuthCephx',
> >> > + 'data': { '*key-secret': 'str' } }
> >> > +
> >> > +##
> >> > # @BlockdevOptionsRbd:
> >> > #
> >> > # @pool: Ceph pool name.
> >> > @@ -3184,6 +3197,13 @@
> >> > #
> >> > # @user: Ceph id name.
> >> > #
> >> > +# @auth-none: true if connecting to a server without authentication
> >> > +# should be allowed (default: false; since 2.13)
> >> > +#
> >> > +# @auth-cephx: Configuration for cephx authentication if specified. If
> >> > +# not specified, cephx authentication is not allowed.
> >> > +# (since 2.13)
> >> > +#
> >> > # @server: Monitor host address and port. This maps
> >> > # to the "mon_host" Ceph option.
> >> > #
> >> > @@ -3195,6 +3215,8 @@
> >> > '*conf': 'str',
> >> > '*snapshot': 'str',
> >> > '*user': 'str',
> >> > + '*auth-none': 'bool',
> >> > + '*auth-cephx': 'RbdAuthCephx',
> >> > '*server': ['InetSocketAddressBase'] } }
> >> >
> >> > ##
> >>
> >> Two new optional members @auth-none, @auth-cephx.
> >>
> >> For backwards compatibility, behavior needs to remain unchanged when
> >> both are absent. What is the behavior we need to preserve? Config
> >> file, then default?
> >
> > Yes. (rados_conf_set("auth_client_required") is not called at all.)
> >
> >> The schema permits four cases, which get translated to an auth client
> >> required setting by qemu_rbd_set_auth(), visible below:
> >>
> >> (1) just auth-none: we set auth_client_required to "none"
> >>
> >> (2) just auth-cephx: we set auth_client_required to "cephx"
> >>
> >> (3) both: we set auth_client_required to "none;cephx", which I can't
> >> find in the documentation right now, but according to my notes means
> >> "either method"
> >
> > The state of the Ceph documentation indeed wasn't very helpful. On the
> > other hand, I'm afraid we're not in a position to complain about bad
> > documentation...
>
> Right on both counts.
>
> >> (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?!?
>
> >> When auth-cephx is present, we get subcases
> >>
> >> (2a) and (3a) key-secret present: key is in the QCryptoSecret named, and
> >> we set
> >>
> >> (2b) and (3b) key-secret absent: key must be provided some other way,
> >> say in a configuration file, default keyring, whatever, or else
> >> we'll fail somehow, I guess
> >>
> >> Related: existing BlockdevOptionsRbd member @user. As far as I know,
> >> it's meaningless with auth-none.
> >
> > I don't know otherwise and suspect you are right, but Jeff and I
> > couldn't find any definite confirmation for this assumption, so we left
> > it where it is instead of moving it into RbdAuthCephx.
>
> 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.
>
> >> Ways to avoid the troublesome auth-cephx: {}:
> >>
> >> (A) Make auth-cephx a bool, rely on the other ways to provide the key
> >>
> >> (B) Make auth-cephx a bool and add the @key-secret member next to it
> >>
> >> Like @user, the member is meaningless with auth-none.
> >>
> >> (C) Make auth-cephx.key-secret a mandatory StrOrNull
> >>
> >> Should take care of "vanishes on flattening" problem, but dotted key
> >> syntax is still screwed: since any value is a valid string, there's
> >> none left for null. My take would be that if you want to say
> >> auth-cephx.key-secret: {}, you get to say it in JSON.
> >>
> >> Aside: check_alternate() tries to catch such alternates, but we
> >> failed to update it when we added first class null. *sigh*
> >>
> >> Which one do you hate least?
> >
> > What should happen with null when you stringify it? If we want to take
> > the options QDict, stringify all entries and run them through the keyval
> > visitor, I'm almost sure that null will be lost.
>
> 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.
>
> > So (A) doesn't work unless we can specify what "other ways" are that are
> > acceptable for libvirt,
>
> Yes.
>
> > and (C) probably doesn't play well with b. above
> > (the qdict_stringify_entries() for handling mixed type QDicts).
>
> I think it could be done, and tried to sketch how.
>
> > 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?
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:
> On Fri, Apr 20, 2018 at 04:39:02PM +0200, Markus Armbruster wrote:
> > >> Ways to avoid the troublesome auth-cephx: {}:
> > >>
> > >> (A) Make auth-cephx a bool, rely on the other ways to provide the key
> > >>
> > >> (B) Make auth-cephx a bool and add the @key-secret member next to it
> > >>
> > >> Like @user, the member is meaningless with auth-none.
> > >>
> > >> (C) Make auth-cephx.key-secret a mandatory StrOrNull
> > >>
> > >> Should take care of "vanishes on flattening" problem, but dotted key
> > >> syntax is still screwed: since any value is a valid string, there's
> > >> none left for null. My take would be that if you want to say
> > >> auth-cephx.key-secret: {}, you get to say it in JSON.
> > >>
> > >> Aside: check_alternate() tries to catch such alternates, but we
> > >> failed to update it when we added first class null. *sigh*
> > >>
> > >> Which one do you hate least?
> > >
> > > What should happen with null when you stringify it? If we want to take
> > > the options QDict, stringify all entries and run them through the keyval
> > > visitor, I'm almost sure that null will be lost.
> >
> > 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.
> > 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.
> >
> > > So (A) doesn't work unless we can specify what "other ways" are that are
> > > acceptable for libvirt,
> >
> > Yes.
> >
> > > and (C) probably doesn't play well with b. above
> > > (the qdict_stringify_entries() for handling mixed type QDicts).
> >
> > I think it could be done, and tried to sketch how.
> >
> > > 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.
> > 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).
> How about I put together a patch with (B) for an RFC v2?
How about doing the same with (C) and moving @user? :-)
Kevin
On Wed, Apr 25, 2018 at 09:50:19AM +0200, Kevin Wolf wrote:
> Am 24.04.2018 um 20:26 hat Jeff Cody geschrieben:
> > On Fri, Apr 20, 2018 at 04:39:02PM +0200, Markus Armbruster wrote:
> > > >> Ways to avoid the troublesome auth-cephx: {}:
> > > >>
> > > >> (A) Make auth-cephx a bool, rely on the other ways to provide the key
> > > >>
> > > >> (B) Make auth-cephx a bool and add the @key-secret member next to it
> > > >>
> > > >> Like @user, the member is meaningless with auth-none.
> > > >>
> > > >> (C) Make auth-cephx.key-secret a mandatory StrOrNull
> > > >>
> > > >> Should take care of "vanishes on flattening" problem, but dotted key
> > > >> syntax is still screwed: since any value is a valid string, there's
> > > >> none left for null. My take would be that if you want to say
> > > >> auth-cephx.key-secret: {}, you get to say it in JSON.
> > > >>
> > > >> Aside: check_alternate() tries to catch such alternates, but we
> > > >> failed to update it when we added first class null. *sigh*
> > > >>
> > > >> Which one do you hate least?
> > > >
> > > > What should happen with null when you stringify it? If we want to take
> > > > the options QDict, stringify all entries and run them through the keyval
> > > > visitor, I'm almost sure that null will be lost.
> > >
> > > 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.
>
> > > 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.
> > >
> > > > So (A) doesn't work unless we can specify what "other ways" are that are
> > > > acceptable for libvirt,
> > >
> > > Yes.
> > >
> > > > and (C) probably doesn't play well with b. above
> > > > (the qdict_stringify_entries() for handling mixed type QDicts).
> > >
> > > I think it could be done, and tried to sketch how.
> > >
> > > > 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.
>
> > > 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.
>
Good point on the default case, that essentially negates my concerns.
> I prefer slightly unwieldy command line syntax to getting the internally
> used data types wrong (= hard to use correctly).
>
Fair enough.
> > 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
© 2016 - 2024 Red Hat, Inc.