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é <berra...@redhat.com>
>>> ---
>>> 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
>
