Re: [Qemu-devel] [PATCH v3 05/10] util: add QAuthZSimple object type for a simple access control list
On Tue, Mar 22, 2016 at 11:38:53AM -0600, Eric Blake wrote:
> On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
> > Add a QAuthZSimple object type that implements the QAuthZ
> > interface. This simple built-in implementation maintains
> > a trivial access control list with a sequence of match
> > rules and a final default policy. This replicates the
> > functionality currently provided by the qemu_acl module.
> >
> > To create an instance of this object via the QMP monitor,
> > the syntax used would be
> >
> > {
> > "execute": "object-add",
> > "arguments": {
> > "qom-type": "authz-simple",
> > "id": "auth0",
> > "parameters": {
> > "rules": [
> >{ "match": "fred", "policy": "allow" },
> >{ "match": "bob", "policy": "allow" },
> >{ "match": "danb", "policy": "deny" },
> >{ "match": "dan*", "policy": "allow" }
> > ],
> > "policy": "deny"
> > }
> > }
> > }
>
> So "match" appears to be a glob (as opposed to a regex). And order is
> important (the first rule matched ends the lookup), so you correctly
> used an array for the list of rules (a dict does not have to maintain
> order).
Yes, its a glob (as defined by fnmatch)
> >
> > Or via the -object command line
> >
> > $QEMU \
> > -object authz-simple,id=acl0,policy=deny,\
> > match.0.name=fred,match.0.policy=allow, \
> > match.1.name=bob,match.1.policy=allow, \
> > match.2.name=danb,match.2.policy=deny, \
> > match.3.name=dan*,match.3.policy=allow
>
> The '*' in the command line will require shell quoting.
Yeah, CLI syntax escaping from shell is horrible, but fortunately
libvirt doesn't use shell :-) None the less I'll update the
example.
> > +##
> > +# QAuthZSimplePolicy:
> > +#
> > +# The authorization policy result
> > +#
> > +# @deny: deny access
> > +# @allow: allow access
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'enum': 'QAuthZSimplePolicy',
> > + 'prefix': 'QAUTHZ_SIMPLE_POLICY',
> > + 'data': ['deny', 'allow']}
>
> I know Markus isn't a big fan of 'prefix', but I don't mind it.
It doesn't affect public API anyway, so we can change it at will
later if desired.
> We're awfully late in the 2.6 cycle; this feels like a feature addition
> rather than a bug fix, so should it be 2.7?
It is definitely a feature addition. I was hoping to get it into 2.6
since it is logically associated with the TLS enhancements I've been
doing. It isn't the end of the world if it waits until 2.7 though,
so I'm open minded either way.
Basically TLS provides encryption, x509 certs provide authentication,
and this ACL stuff provides the authorization piece of the puzzel.
If we miss the authorization piece in 2.6 we're still in a far better
position than we were historically because we at least have encryption
and authentication still :-) You can mitigate the lack of authorization
by having a dedicated CA certificate just for QEMU services, so that
you limit who has access to client certs. This is a crude authorization
setup that is none the less acceptable in many scenarios.
> > +##
> > +# QAuthZSimpleRule:
> > +#
> > +# A single authorization rule.
> > +#
> > +# @match: a glob to match against a user identity
> > +# @policy: the result to return if @match evaluates to true
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'struct': 'QAuthZSimpleRule',
> > + 'data': {'match': 'str',
> > + 'policy': 'QAuthZSimplePolicy'}}
>
> Hmm. I was expecting something like:
>
> { 'struct': 'QAuthZSimple',
> 'data': { 'rules': [ 'QAuthZSimpleRule' ],
> 'policy': 'QAuthZSimplePolicy' } }
>
> but I guess that's one of our short-comings of QOM: the top-level
> structure does not have to be specified anywhere in QAPI.
I'm not sure I'd call it a short-coming but rather its just a difference
in approach. For anything that is a user-defined object, the QAPI schema
is defined implicitly by the QOM object or class definition. With the
ability to define QOM properties against the class instad of object,
we are actally in a position where we could auto-generate a QAPI schema
to represent each user-defined object type if desired. Alternatively
we could equally extend QAPI to have an 'object' type (which is a
specialization of the 'struct' type) and auto-generate the basic
boilerplate code to define a QOM object class from it.
> > +static void test_authz_default_deny(void)
> > +{
> > +QAuthZSimple *auth = qauthz_simple_new("auth0",
> > + QAUTHZ_SIMPLE_POLICY_DENY,
> > + _abort);
> > +
> > +g_assert(!qauthz_is_allowed(QAUTHZ(auth), "fred", _abort));
> > +
>
> Okay, so you definitely intend to return false without setting errp, so
> it is a ternary result.
Yep
> > +#ifdef CONFIG_FNMATCH
> > +static void test_authz_complex(void)
> > +{
>
> Wait - the behavior depends on whether fnmatch() is available? That
Re: [Qemu-devel] [PATCH v3 05/10] util: add QAuthZSimple object type for a simple access control list
On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
> Add a QAuthZSimple object type that implements the QAuthZ
> interface. This simple built-in implementation maintains
> a trivial access control list with a sequence of match
> rules and a final default policy. This replicates the
> functionality currently provided by the qemu_acl module.
>
> To create an instance of this object via the QMP monitor,
> the syntax used would be
>
> {
> "execute": "object-add",
> "arguments": {
> "qom-type": "authz-simple",
> "id": "auth0",
> "parameters": {
> "rules": [
>{ "match": "fred", "policy": "allow" },
>{ "match": "bob", "policy": "allow" },
>{ "match": "danb", "policy": "deny" },
>{ "match": "dan*", "policy": "allow" }
> ],
> "policy": "deny"
> }
> }
> }
So "match" appears to be a glob (as opposed to a regex). And order is
important (the first rule matched ends the lookup), so you correctly
used an array for the list of rules (a dict does not have to maintain
order).
>
> Or via the -object command line
>
> $QEMU \
> -object authz-simple,id=acl0,policy=deny,\
> match.0.name=fred,match.0.policy=allow, \
> match.1.name=bob,match.1.policy=allow, \
> match.2.name=danb,match.2.policy=deny, \
> match.3.name=dan*,match.3.policy=allow
The '*' in the command line will require shell quoting.
>
> This sets up an authorization rule that allows 'fred',
> 'bob' and anyone whose name starts with 'dan', except
> for 'danb'. Everyone unmatched is denied.
>
> Signed-off-by: Daniel P. Berrange
> ---
> +/**
> + * QAuthZSimple:
> + *
> + * This authorization driver provides a simple mechanism
> + * for granting access by matching user names against a
> + * list of globs. Each match rule has an associated policy
> + * and a catch all policy applies if no rule matches
> + *
> + * To create an instace of this class via QMP:
s/instace/instance/
> + *
> + * Or via the CLI:
> + *
> + * $QEMU \
> + *-object authz-simple,id=acl0,policy=deny, \
> + *match.0.name=fred,match.0.policy=allow, \
> + *match.1.name=bob,match.1.policy=allow,\
> + *match.2.name=danb,match.2.policy=deny,\
> + *match.3.name=dan*,match.3.policy=allow
Again, quoting the "*" is important, and maybe a comment that the
whitespace is for display purposes but must be omitted on the command line.
> +++ b/qapi-schema.json
> @@ -5,6 +5,9 @@
> # QAPI common definitions
> { 'include': 'qapi/common.json' }
>
> +# QAPI util definitions
> +{ 'include': 'qapi/util.json' }
> +
> # QAPI crypto definitions
> { 'include': 'qapi/crypto.json' }
>
> @@ -3652,7 +3655,8 @@
> # Since 2.5
> ##
> { 'struct': 'DummyForceArrays',
> - 'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
> + 'data': { 'unused': ['X86CPUFeatureWordInfo'],
> +'iamalsounused': ['QAuthZSimpleRule'] } }
Cute. I might have renamed things and gone 'unused1' and 'unused2'.
> +++ b/qapi/util.json
> @@ -0,0 +1,31 @@
> +# -*- Mode: Python -*-
> +#
> +# QAPI util definitions
> +
> +##
> +# QAuthZSimplePolicy:
> +#
> +# The authorization policy result
> +#
> +# @deny: deny access
> +# @allow: allow access
> +#
> +# Since: 2.6
> +##
> +{ 'enum': 'QAuthZSimplePolicy',
> + 'prefix': 'QAUTHZ_SIMPLE_POLICY',
> + 'data': ['deny', 'allow']}
I know Markus isn't a big fan of 'prefix', but I don't mind it.
We're awfully late in the 2.6 cycle; this feels like a feature addition
rather than a bug fix, so should it be 2.7?
> +
> +##
> +# QAuthZSimpleRule:
> +#
> +# A single authorization rule.
> +#
> +# @match: a glob to match against a user identity
> +# @policy: the result to return if @match evaluates to true
> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'QAuthZSimpleRule',
> + 'data': {'match': 'str',
> + 'policy': 'QAuthZSimplePolicy'}}
Hmm. I was expecting something like:
{ 'struct': 'QAuthZSimple',
'data': { 'rules': [ 'QAuthZSimpleRule' ],
'policy': 'QAuthZSimplePolicy' } }
but I guess that's one of our short-comings of QOM: the top-level
structure does not have to be specified anywhere in QAPI.
> +++ b/tests/test-authz-simple.c
> @@ -0,0 +1,156 @@
> +/*
> + * QEMU simple authorization object
> + *
> + * Copyright (c) 2016 Red Hat, Inc.
> + *
> +static void test_authz_default_deny(void)
> +{
> +QAuthZSimple *auth = qauthz_simple_new("auth0",
> + QAUTHZ_SIMPLE_POLICY_DENY,
> + _abort);
> +
> +g_assert(!qauthz_is_allowed(QAUTHZ(auth), "fred", _abort));
> +
Okay, so you definitely intend to return false without setting errp, so
it is a ternary result.
> +
> +#ifdef CONFIG_FNMATCH
> +static void test_authz_complex(void)
> +{
Wait - the behavior depends on
