Re: [Qemu-block] [PATCH v3 05/10] util: add QAuthZSimple object type for a simple access control list

[Eric Blake]

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 <address@hidden>
> ---

> +/**
> + * 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,
> +                                           &error_abort);
> +
> +    g_assert(!qauthz_is_allowed(QAUTHZ(auth), "fred", &error_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 whether fnmatch() is available?  That is,
a name is a literal match if fnmatch() is not present, and glob if
present?  I'd argue that if fnmatch() is not present, we must explicitly
reject any "match" with glob metacharacters, rather than accidentally
matching only a literal "*" when a glob was intended.

> +++ b/util/authz-simple.c

> +
> +/* If no fnmatch, fallback to exact string matching
> + * instead of allowing wildcards */
> +#ifdef CONFIG_FNMATCH
> +#include <fnmatch.h>
> +#define qauthz_match(pattern, string) fnmatch(pattern, string, 0)
> +#else
> +#define qauthz_match(pattern, string) strcmp(pattern, string)
> +#endif

As mentioned above, I'd be more comfortable if you also explicitly
reject any attempt to add a pattern that resembles a glob when globbing
is not enabled.  Or maybe it's worth a more complex QAPI definition:

# How to interpret 'match', as a literal string or a glob
{ 'enum': 'QAuthZSimpleStyle', 'data': [ 'literal', 'glob' ] }
# @style: #optional, default to 'literal'
{ 'struct': 'QAuthZSimpleRule',
  'data': { 'match': 'str', '*style': 'QAuthZSimpleStyle',
            'policy': 'QAuthZSimplePolicy' } }

and used as:

         "rules": [
            { "match": "fred", "policy": "allow" },
            { "match": "bob", "policy": "allow" },
            { "match": "danb", "policy": "deny" },
            { "match": "dan*", "policy": "allow", "style": "glob" }

where you can then gracefully error out for ALL attempts to use
"style":"glob" if fnmatch() is not present, and use strcmp() even when
fnmatch() is present for rules that have the (default) "style":"literal".


> +static void
> +qauthz_simple_class_init(ObjectClass *oc, void *data)
> +{
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +    QAuthZClass *authz = QAUTHZ_CLASS(oc);
> +
> +    ucc->complete = qauthz_simple_complete;
> +    authz->is_allowed = qauthz_simple_is_allowed;
> +
> +    object_class_property_add_enum(oc, "policy",
> +                                   "QAuthZSimplePolicy",
> +                                   QAuthZSimplePolicy_lookup,
> +                                   qauthz_simple_prop_get_policy,
> +                                   qauthz_simple_prop_set_policy,
> +                                   NULL);
> +
> +    object_class_property_add(oc, "rules", "QAuthZSimpleRule",
> +                              qauthz_simple_prop_get_rules,
> +                              qauthz_simple_prop_set_rules,
> +                              NULL, NULL, NULL);
> +}

Not for this patch, but it would be cool if we had enough framework to
just tell QOM to instantiate an object with callbacks by merely pointing
to the name of a QAPI type that the object implements (referring back to
my comment that I'm a bit surprised we didn't need a QAPI type for the
overall QAuthZSimple).

> +
> +size_t qauthz_simple_append_rule(QAuthZSimple *auth, const char *match,
> +                                 QAuthZSimplePolicy policy)
> +{
> +    QAuthZSimpleRule *rule;
> +    QAuthZSimpleRuleList *rules, *tmp;
> +    size_t i = 0;
> +
> +    rule = g_new0(QAuthZSimpleRule, 1);
> +    rule->policy = policy;
> +    rule->match = g_strdup(match);
> +
> +    tmp = g_new0(QAuthZSimpleRuleList, 1);
> +    tmp->value = rule;
> +
> +    rules = auth->rules;
> +    if (rules) {
> +        while (rules->next) {
> +            i++;
> +            rules = rules->next;
> +        }
> +        rules->next = tmp;
> +        return i + 1;

No checks whether 'match' is already an existing rule...


> +ssize_t qauthz_simple_delete_rule(QAuthZSimple *auth, const char *match)
> +{
> +    QAuthZSimpleRule *rule;
> +    QAuthZSimpleRuleList *rules, *prev;
> +    size_t i = 0;
> +
> +    prev = NULL;
> +    rules = auth->rules;
> +    while (rules) {
> +        rule = rules->value;
> +        if (g_str_equal(rule->match, match)) {
> +            if (prev) {
> +                prev->next = rules->next;
> +            } else {
> +                auth->rules = rules->next;
> +            }
> +            rules->next = NULL;
> +            qapi_free_QAuthZSimpleRuleList(rules);
> +            return i;

...which means a rule can be inserted more than once, and this only
removes the first instance of the rule.  Do we care enough to add in
additional checking that we aren't permitting duplicate 'match' strings
in the list of rules?

If you add the style=literal/glob to a rule, would you also want to be
able to constrain deletion to a particular style (delete the glob 'dan*'
but leave the literal 'dan*' intact)?

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

signature.asc
Description: OpenPGP digital signature