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

[Eric Blake]

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" <address@hidden>
> 
> 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 <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

> +++ 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?)

> +#
> +# Since: 4.0
> +##
> +{ 'enum': 'QAuthZListFormat',
> +  'prefix': 'QAUTHZ_LIST_FORMAT',
> +  'data': ['exact', 'glob']}
> +
> +##
> +# @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.

> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'QAuthZListRule',
> +  'data': {'match': 'str',
> +           'policy': 'QAuthZListPolicy',
> +           '*format': 'QAuthZListFormat'}}
> +
> +##
> +# @QAuthZListRuleListHack:
> +#
> +# Not exposed via QMP; hack to generate QAuthZListRuleList
> +# for use internally by the code.
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'QAuthZListRuleListHack',
> +  'data': { 'unused': ['QAuthZListRule'] } }

We keep on encountering these hacks; someday it would be nice to teach
the QAPI generator a nicer way to do this. But not your problem.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org