Re: [Qemu-devel] [PATCH v5 09/11] authz: add QAuthZListFile object type for a file access control list

[Daniel P . Berrangé]

On Fri, Oct 19, 2018 at 11:41:45AM +0200, Philippe Mathieu-Daudé wrote:
> On 09/10/2018 15:04, Daniel P. Berrangé wrote:
> > Add a QAuthZListFile object type that implements the QAuthZ interface. This
> > built-in implementation is a proxy around the QAtuhZList object type,
> > initializing it from an external file, and optionally, automatically
> > reloading it whenever it changes.
> > 
> > To create an instance of this object via the QMP monitor, the syntax
> > used would be:
> > 
> >       {
> >         "execute": "object-add",
> >         "arguments": {
> >           "qom-type": "authz-list-file",
> >           "id": "authz0",
> >           "parameters": {
> >             "filename": "/etc/qemu/vnc.acl",
> >         "refresh": "yes"
> >           }
> >         }
> >       }
> > 
> > If "refresh" is "yes", inotify is used to monitor the file,
> > automatically reloading changes. If an error occurs during reloading,
> > all authorizations will fail until the file is next successfully
> > loaded.
> > 
> > The /etc/qemu/vnc.acl file would contain a JSON representation of a
> > QAuthZList object
> > 
> >     {
> >       "rules": [
> >          { "match": "fred", "policy": "allow", "format": "exact" },
> >          { "match": "bob", "policy": "allow", "format": "exact" },
> >          { "match": "danb", "policy": "deny", "format": "glob" },
> >          { "match": "dan*", "policy": "allow", "format": "exact" },
> >       ],
> >       "policy": "deny"
> >     }
> > 
> > This sets up an authorization rule that allows 'fred', 'bob' and anyone
> > whose name starts with 'dan', except for 'danb'. Everyone unmatched is
> > denied.
> > 
> > The object can be loaded on the comand line using
> > 
> >    -object authz-list-file,id=authz0,filename=/etc/qemu/vnc.acl,refresh=yes
> > 
> > Signed-off-by: Daniel P. Berrangé <address@hidden>
> > ---
> >  authz/Makefile.objs      |   1 +
> >  authz/listfile.c         | 284 +++++++++++++++++++++++++++++++++++++++
> >  authz/trace-events       |   4 +
> >  include/authz/listfile.h | 110 +++++++++++++++
> >  qemu-options.hx          |  47 +++++++
> >  5 files changed, 446 insertions(+)
> >  create mode 100644 authz/listfile.c
> >  create mode 100644 include/authz/listfile.h
> > 

> > +static void
> > +qauthz_list_file_event(int wd G_GNUC_UNUSED,
> > +                       QFileMonitorEvent ev G_GNUC_UNUSED,
> > +                       const char *name G_GNUC_UNUSED,
> > +                       void *opaque)
> > +{
> > +    QAuthZListFile *fauthz = opaque;
> > +    Error *err = NULL;
> > +
> > +    if (ev != QFILE_MONITOR_EVENT_MODIFIED &&
> > +        ev != QFILE_MONITOR_EVENT_CREATED)
> 
> You missed:
> 
>                                               {
> 
> > +        return;
> 
>        }

Opps, yes.


> > +static void
> > +qauthz_list_file_complete(UserCreatable *uc, Error **errp)
> > +{
> > +    QAuthZListFile *fauthz = QAUTHZ_LIST_FILE(uc);
> > +
> > +    fauthz->list = qauthz_list_file_load(fauthz, errp);
> > +
> > +    if (fauthz->refresh) {
> 
> Can we invert this condition?

Yes, will do

> 
> > +        gchar *dir, *file;
> > +        fauthz->file_monitor = qemu_file_monitor_get_instance(errp);
> > +        if (!fauthz->file_monitor) {
> > +            return;
> > +        }
> > +
> > +        dir = g_path_get_dirname(fauthz->filename);
> > +        if (g_str_equal(dir, ".")) {
> > +            error_setg(errp, "Filename must be an absolute path");
> 
> What about:
> 
>                goto cleanup;

Yep.

> 
> > +            g_free(dir);
> > +            return;
> > +        }
> > +        file = g_path_get_basename(fauthz->filename);
> > +        if (g_str_equal(file, ".")) {
> > +            error_setg(errp, "Path has no trailing filename component");
> 
>                goto cleanup;
> 
> > +            g_free(file);
> > +            g_free(dir);
> > +            return;
> > +        }
> > +
> > +        fauthz->file_watch = qemu_file_monitor_add_watch(
> > +            fauthz->file_monitor, dir, file,
> > +            qauthz_list_file_event, fauthz, errp);
> > +        g_free(file);
> > +        g_free(dir);
> > +        if (fauthz->file_watch < 0) {
> 
> Is this really useful? Do you plan to add more code here?

I just want to make it clear to anyone who changes the code
in future that there's an expected failure condition here.

> > +            return;
> > +        }
> > +    }
> > +}

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