Re: [Qemu-devel] [PATCH v6 08/11] authz: add QAuthZList object type for an access control list

[Daniel P . Berrangé]

On Thu, Nov 08, 2018 at 02:23:43AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Oct 19, 2018 at 5:45 PM Daniel P. Berrangé <address@hidden> wrote
> > ---
> >  Makefile                |   7 +-
> >  Makefile.objs           |   4 +
> >  qapi/authz.json         |  58 ++++++++
> >  qapi/qapi-schema.json   |   1 +
> >  include/authz/list.h    | 106 ++++++++++++++
> >  authz/list.c            | 309 ++++++++++++++++++++++++++++++++++++++++
> >  tests/test-authz-list.c | 171 ++++++++++++++++++++++
> >  .gitignore              |   4 +
> >  MAINTAINERS             |   1 +
> >  authz/Makefile.objs     |   1 +
> >  authz/trace-events      |   4 +
> >  tests/Makefile.include  |   4 +
> >  12 files changed, 669 insertions(+), 1 deletion(-)
> >  create mode 100644 qapi/authz.json
> >  create mode 100644 include/authz/list.h
> >  create mode 100644 authz/list.c
> >  create mode 100644 tests/test-authz-list.c
> >

> > diff --git a/qapi/authz.json b/qapi/authz.json
> > new file mode 100644
> > index 0000000000..607839c627
> > --- /dev/null
> > +++ b/qapi/authz.json
> > @@ -0,0 +1,58 @@
> > +# -*- Mode: Python -*-
> > +#
> > +# QAPI authz definitions
> > +
> > +##
> > +# @QAuthZListPolicy:
> > +#
> > +# The authorization policy result
> > +#
> > +# @deny: deny access
> > +# @allow: allow access
> > +#
> > +# Since: 3.0
> 
> obviously, you'll have to update to 3.2

4.0 in fact since we bump at the start of each year


> > +##
> > +# @QAuthZListRuleListHack:
> > +#
> > +# Not exposed via QMP; hack to generate QAuthZListRuleList
> > +# for use internally by the code.
> 
> Well, this will probably end in the documentation (it's already in the
> .json, which is one source of documentation ;).
> 
> What about adding a 'gen-list' field, or a 'pragma' listing the
> structs that should have list code generated?

I'll leave that for future motivated contributors, as I'm just
following pre-existing best practice here.


> > +static bool qauthz_list_is_allowed(QAuthZ *authz,
> > +                                   const char *identity,
> > +                                   Error **errp)
> > +{
> > +    QAuthZList *lauthz = QAUTHZ_LIST(authz);
> > +    QAuthZListRuleList *rules = lauthz->rules;
> > +
> > +    while (rules) {
> > +        QAuthZListRule *rule = rules->value;
> > +        QAuthZListFormat format = rule->has_format ? rule->format :
> > +            QAUTHZ_LIST_FORMAT_EXACT;
> > +
> > +        trace_qauthz_list_check_rule(authz, rule->match, identity,
> > +                                     format, rule->policy);
> > +        switch (format) {
> > +        case QAUTHZ_LIST_FORMAT_EXACT:
> > +            if (strcmp(rule->match, identity) == 0) {
> 
> g_str_equal() ?

Yes.

> 
> > +                return rule->policy == QAUTHZ_LIST_POLICY_ALLOW;
> > +            }
> > +            break;
> > +#ifdef CONFIG_FNMATCH
> > +        case QAUTHZ_LIST_FORMAT_GLOB:
> > +            if (fnmatch(rule->match, identity, 0) == 0) {
> 
> Would GPatternSpec be a good alternative?

Excellent, I didn't know about it

> > +                return rule->policy == QAUTHZ_LIST_POLICY_ALLOW;
> > +            }
> > +            break;
> > +#else
> > +            return false;
> > +#endif
> > +        default:
> 
> 
> No g_warn_if_reached() ?Then perhaps add a comment why.

I guess we could.

> > +ssize_t qauthz_list_delete_rule(QAuthZList *auth, const char *match)
> > +{
> > +    QAuthZListRule *rule;
> > +    QAuthZListRuleList *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_QAuthZListRuleList(rules);
> > +            return i;
> 
> What's the point in returning the old index? Maybe true/false along
> with an Error would be more convenient?

It is required for the conversion of the existing acl_remove API
in the HMP monitor.


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 :|