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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Feb 25, 2019 at 08:36:33AM -0600, Eric Blake wrote:
> I missed reviewing this before the pull request, so comments here are
> best for a followup patch:
> 
> On 2/25/19 6:31 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > Add a QAuthZList object type that implements the QAuthZ interface. This
> > 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.
> > 
> 
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> 
> > +++ b/qapi/Makefile.objs
> > @@ -7,7 +7,7 @@ util-obj-y += qapi-util.o
> >  
> >  QAPI_COMMON_MODULES = block-core block char common crypto introspect
> >  QAPI_COMMON_MODULES += job migration misc net rdma rocker run-state
> > -QAPI_COMMON_MODULES += sockets tpm trace transaction ui
> > +QAPI_COMMON_MODULES += sockets tpm trace transaction ui authz
> 
> Let's keep this list alphabetically sorted (authz before block-core).
> 
> > +++ 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: 4.0
> > +##
> > +{ 'enum': 'QAuthZListPolicy',
> > +  'prefix': 'QAUTHZ_LIST_POLICY',
> > +  'data': ['deny', 'allow']}
> > +
> > +##
> > +# @QAuthZListFormat:
> > +#
> > +# The authorization policy result
> > +#
> > +# @exact: an exact string match
> > +# @glob: string with ? and * shell wildcard support
> 
> Does it actually use glob() (in which case it also has [] glob support?)

No, it uses g_pattern_match as glob() isn't cross-platform, and
this only supports ? and *

https://developer.gnome.org/glib/stable/glib-Glob-style-pattern-matching.html

  "The g_pattern_match* functions match a string against a 
   pattern containing '*' and '?' wildcards with similar 
   semantics as the standard glob() function: '*' matches 
   an arbitrary, possibly empty, string, '?' matches an 
   arbitrary character.

   Note that in contrast to glob(), the '/' character can 
   be matched by the wildcards, there are no '[...]' 
   character ranges and '*' and '?' can not be escaped to
   include them literally in a pattern."

> > +##
> > +# @QAuthZListRule:
> > +#
> > +# A single authorization rule.
> > +#
> > +# @match: a glob to match against a user identity
> 
> Should this read 'a string or glob to match...' since...
> 
> > +# @policy: the result to return if @match evaluates to true
> > +# @format: (optional) the format of the @match rule (default 'exact')
> 
> ...format controls which of the two styles it is interpreted as?  The
> use of '(optional)' is not required in the current QAPI doc generator,
> and in fact results in redundant output.

This shows how long this pach series has been waiting to merge....

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