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

[Daniel P. Berrange]

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,
> > +                                           &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.

Yep

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

Historically we have the qemu/acl.c code which uses fnmatch and falls
back to plain strcmp if not available. Since this was intended to be an
exact drop-in replacement, I tried to implement the same logic here.

We could do something slightly different though - define a property
against the QAuthzSimple object which enumerates the type of matching
scheme - exact, glob or regex. The acl.c compatibility layer could
then simply set  matchtype=exact when !CONFIG_FNMATCH, which would
in turn allow the QAuthzSimple impl to raise a fatall error for use
of globbing when fnmatch is missing.

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

Yep, allowing style to be set per rule is another option - I wonder
if it is better todo it per-rule or per-ACL applying to all rules.

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

Yep, as mentioned above this is just the difference between QAPI
and QOM user creatable objects that needs resolving somehow.

> > +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?

I don't think it really matters much - if you add 2 rules with the
same string, you still need 2 calls to delete it. The order of
deletion can be left undefined I think.

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