qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 09/11] authz: add QAuthZListFile object type


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-devel] [PATCH v5 09/11] authz: add QAuthZListFile object type for a file access control list
Date: Fri, 19 Oct 2018 14:57:20 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

On 19/10/2018 14:53, Daniel P. Berrangé wrote:
> 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.

I thought so. Just add a simple /* comment */ about it :)

> 
>>> +            return;
>>> +        }
>>> +    }
>>> +}
> 
> Regards,
> Daniel
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]