On Tue, Mar 22, 2016 at 11:38:53AM -0600, Eric Blake wrote: > On 03/10/2016 11:59 AM, Daniel P. Berrange wrote: > > Add a QAuthZSimple object type that implements the QAuthZ > > interface. This simple 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. > > > > To create an instance of this object via the QMP monitor, > > the syntax used would be > > > > { > > "execute": "object-add", > > "arguments": { > > "qom-type": "authz-simple", > > "id": "auth0", > > "parameters": { > > "rules": [ > > { "match": "fred", "policy": "allow" }, > > { "match": "bob", "policy": "allow" }, > > { "match": "danb", "policy": "deny" }, > > { "match": "dan*", "policy": "allow" } > > ], > > "policy": "deny" > > } > > } > > } > > So "match" appears to be a glob (as opposed to a regex). And order is > important (the first rule matched ends the lookup), so you correctly > used an array for the list of rules (a dict does not have to maintain > order).
Yes, its a glob (as defined by fnmatch) > > > > Or via the -object command line > > > > $QEMU \ > > -object authz-simple,id=acl0,policy=deny,\ > > match.0.name=fred,match.0.policy=allow, \ > > match.1.name=bob,match.1.policy=allow, \ > > match.2.name=danb,match.2.policy=deny, \ > > match.3.name=dan*,match.3.policy=allow > > The '*' in the command line will require shell quoting. Yeah, CLI syntax escaping from shell is horrible, but fortunately libvirt doesn't use shell :-) None the less I'll update the example. > > +## > > +# QAuthZSimplePolicy: > > +# > > +# The authorization policy result > > +# > > +# @deny: deny access > > +# @allow: allow access > > +# > > +# Since: 2.6 > > +## > > +{ 'enum': 'QAuthZSimplePolicy', > > + 'prefix': 'QAUTHZ_SIMPLE_POLICY', > > + 'data': ['deny', 'allow']} > > I know Markus isn't a big fan of 'prefix', but I don't mind it. It doesn't affect public API anyway, so we can change it at will later if desired. > We're awfully late in the 2.6 cycle; this feels like a feature addition > rather than a bug fix, so should it be 2.7? It is definitely a feature addition. I was hoping to get it into 2.6 since it is logically associated with the TLS enhancements I've been doing. It isn't the end of the world if it waits until 2.7 though, so I'm open minded either way. Basically TLS provides encryption, x509 certs provide authentication, and this ACL stuff provides the authorization piece of the puzzel. If we miss the authorization piece in 2.6 we're still in a far better position than we were historically because we at least have encryption and authentication still :-) You can mitigate the lack of authorization by having a dedicated CA certificate just for QEMU services, so that you limit who has access to client certs. This is a crude authorization setup that is none the less acceptable in many scenarios. > > +## > > +# QAuthZSimpleRule: > > +# > > +# A single authorization rule. > > +# > > +# @match: a glob to match against a user identity > > +# @policy: the result to return if @match evaluates to true > > +# > > +# Since: 2.6 > > +## > > +{ 'struct': 'QAuthZSimpleRule', > > + 'data': {'match': 'str', > > + 'policy': 'QAuthZSimplePolicy'}} > > Hmm. I was expecting something like: > > { 'struct': 'QAuthZSimple', > 'data': { 'rules': [ 'QAuthZSimpleRule' ], > 'policy': 'QAuthZSimplePolicy' } } > > but I guess that's one of our short-comings of QOM: the top-level > structure does not have to be specified anywhere in QAPI. I'm not sure I'd call it a short-coming but rather its just a difference in approach. For anything that is a user-defined object, the QAPI schema is defined implicitly by the QOM object or class definition. With the ability to define QOM properties against the class instad of object, we are actally in a position where we could auto-generate a QAPI schema to represent each user-defined object type if desired. Alternatively we could equally extend QAPI to have an 'object' type (which is a specialization of the 'struct' type) and auto-generate the basic boilerplate code to define a QOM object class from it. > > +static void test_authz_default_deny(void) > > +{ > > + QAuthZSimple *auth = qauthz_simple_new("auth0", > > + QAUTHZ_SIMPLE_POLICY_DENY, > > + &error_abort); > > + > > + g_assert(!qauthz_is_allowed(QAUTHZ(auth), "fred", &error_abort)); > > + > > Okay, so you definitely intend to return false without setting errp, so > it is a ternary result. Yep > > +#ifdef CONFIG_FNMATCH > > +static void test_authz_complex(void) > > +{ > > Wait - the behavior depends on whether fnmatch() is available? That is, > a name is a literal match if fnmatch() is not present, and glob if > present? I'd argue that if fnmatch() is not present, we must explicitly > reject any "match" with glob metacharacters, rather than accidentally > matching only a literal "*" when a glob was intended. Historically we have the qemu/acl.c code which uses fnmatch and falls back to plain strcmp if not available. Since this was intended to be an exact drop-in replacement, I tried to implement the same logic here. We could do something slightly different though - define a property against the QAuthzSimple object which enumerates the type of matching scheme - exact, glob or regex. The acl.c compatibility layer could then simply set matchtype=exact when !CONFIG_FNMATCH, which would in turn allow the QAuthzSimple impl to raise a fatall error for use of globbing when fnmatch is missing. > # How to interpret 'match', as a literal string or a glob > { 'enum': 'QAuthZSimpleStyle', 'data': [ 'literal', 'glob' ] } > # @style: #optional, default to 'literal' > { 'struct': 'QAuthZSimpleRule', > 'data': { 'match': 'str', '*style': 'QAuthZSimpleStyle', > 'policy': 'QAuthZSimplePolicy' } } > > and used as: > > "rules": [ > { "match": "fred", "policy": "allow" }, > { "match": "bob", "policy": "allow" }, > { "match": "danb", "policy": "deny" }, > { "match": "dan*", "policy": "allow", "style": "glob" } > > where you can then gracefully error out for ALL attempts to use > "style":"glob" if fnmatch() is not present, and use strcmp() even when > fnmatch() is present for rules that have the (default) "style":"literal". Yep, allowing style to be set per rule is another option - I wonder if it is better todo it per-rule or per-ACL applying to all rules. > > +static void > > +qauthz_simple_class_init(ObjectClass *oc, void *data) > > +{ > > + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc); > > + QAuthZClass *authz = QAUTHZ_CLASS(oc); > > + > > + ucc->complete = qauthz_simple_complete; > > + authz->is_allowed = qauthz_simple_is_allowed; > > + > > + object_class_property_add_enum(oc, "policy", > > + "QAuthZSimplePolicy", > > + QAuthZSimplePolicy_lookup, > > + qauthz_simple_prop_get_policy, > > + qauthz_simple_prop_set_policy, > > + NULL); > > + > > + object_class_property_add(oc, "rules", "QAuthZSimpleRule", > > + qauthz_simple_prop_get_rules, > > + qauthz_simple_prop_set_rules, > > + NULL, NULL, NULL); > > +} > > Not for this patch, but it would be cool if we had enough framework to > just tell QOM to instantiate an object with callbacks by merely pointing > to the name of a QAPI type that the object implements (referring back to > my comment that I'm a bit surprised we didn't need a QAPI type for the > overall QAuthZSimple). Yep, as mentioned above this is just the difference between QAPI and QOM user creatable objects that needs resolving somehow. > > +size_t qauthz_simple_append_rule(QAuthZSimple *auth, const char *match, > > + QAuthZSimplePolicy policy) > > +{ > > + QAuthZSimpleRule *rule; > > + QAuthZSimpleRuleList *rules, *tmp; > > + size_t i = 0; > > + > > + rule = g_new0(QAuthZSimpleRule, 1); > > + rule->policy = policy; > > + rule->match = g_strdup(match); > > + > > + tmp = g_new0(QAuthZSimpleRuleList, 1); > > + tmp->value = rule; > > + > > + rules = auth->rules; > > + if (rules) { > > + while (rules->next) { > > + i++; > > + rules = rules->next; > > + } > > + rules->next = tmp; > > + return i + 1; > > No checks whether 'match' is already an existing rule... > > > > +ssize_t qauthz_simple_delete_rule(QAuthZSimple *auth, const char *match) > > +{ > > + QAuthZSimpleRule *rule; > > + QAuthZSimpleRuleList *rules, *prev; > > + size_t i = 0; > > + > > + prev = NULL; > > + rules = auth->rules; > > + while (rules) { > > + rule = rules->value; > > + if (g_str_equal(rule->match, match)) { > > + if (prev) { > > + prev->next = rules->next; > > + } else { > > + auth->rules = rules->next; > > + } > > + rules->next = NULL; > > + qapi_free_QAuthZSimpleRuleList(rules); > > + return i; > > ...which means a rule can be inserted more than once, and this only > removes the first instance of the rule. Do we care enough to add in > additional checking that we aren't permitting duplicate 'match' strings > in the list of rules? I don't think it really matters much - if you add 2 rules with the same string, you still need 2 calls to delete it. The order of deletion can be left undefined I think. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|