From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56869) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aii8A-0006b4-IQ for qemu-devel@nongnu.org; Wed, 23 Mar 2016 08:43:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aii87-0005xy-9n for qemu-devel@nongnu.org; Wed, 23 Mar 2016 08:43:22 -0400 Date: Wed, 23 Mar 2016 12:43:03 +0000 From: "Daniel P. Berrange" Message-ID: <20160323124303.GP19338@redhat.com> References: <1457635927-23045-1-git-send-email-berrange@redhat.com> <1457636396-24983-1-git-send-email-berrange@redhat.com> <1457636396-24983-10-git-send-email-berrange@redhat.com> <56F1BB46.3090407@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56F1BB46.3090407@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 10/10] vnc: allow specifying a custom ACL object name Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-block@nongnu.org, Markus Armbruster , qemu-devel@nongnu.org, Paolo Bonzini , Max Reitz , Andreas =?utf-8?Q?F=C3=A4rber?= On Tue, Mar 22, 2016 at 03:38:14PM -0600, Eric Blake wrote: > On 03/10/2016 11:59 AM, Daniel P. Berrange wrote: > > The VNC server has historically had support for ACLs to check > > both the SASL username and the TLS x509 distinguished name. > > The VNC server was responsible for creating the initial ACL, > > and the client app was then responsible for populating it with > > rules using the HMP 'acl_add' command. > > > > This is not satisfactory for a variety of reasons. There is > > no way to populate the ACLs from the command line, users are > > forced to use the HMP. With multiple network services all > > supporting TLS and ACLs now, it is desirable to be able to > > define a single ACL that is referenced by all services. > > > > To address these limitations, two new options are added to the > > VNC server CLI. The 'tls-acl' option takes the ID of a QAuthZ > > object to use for checking TLS x509 distinguished names, and > > the 'sasl-acl' option takes the ID of another object to use for > > checking SASL usernames. > > > > In this example, we setup two ACLs. The first allows any client > > with a certificate issued by the 'RedHat' organization in the > > 'London' locality. The second ACL allows clients with either > > the 'joe@REDHAT.COM' or 'fred@REDHAT.COM' kerberos usernames. > > Both ACLs must pass for the user to be allowed. > > > > $QEMU -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\ > > endpoint=server,verify-peer=yes \ > > -object authz-simple,id=acl0,policy=deny,\ > > rules.0.match=O=RedHat,,L=London,rules.0.policy=allow \ > > -object authz-simple,id=acl0,policy=deny,\ > > Umm, you can't reuse 'acl0' as the id. > > > rules.0.match=fred@REDHAT.COM,rules.0.policy=allow \ > > rules.0.match=joe@REDHAT.COM,rules.0.policy=allow \ > > -vnc 0.0.0.0:1,tls-creds=tls0,tls-acl=tlsacl0, > > sasl,sasl-acl=saslacl0 \ > > And this fails because the ids don't exist. I think you meant > authz-simple,id=tlsacl0 in the first instance, and > authz-simple,id=saslacl0 in the second instance. Heh, yeah, I really ought to try the examples I put in the commit message tomake sure they work :-) > > Signed-off-by: Daniel P. Berrange > > --- > > ui/vnc.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++------------ > > 1 file changed, 60 insertions(+), 13 deletions(-) > > > > @@ -3670,6 +3680,21 @@ void vnc_display_open(const char *id, Error **errp) > > } > > } > > acl = qemu_opt_get_bool(opts, "acl", false); > > + tlsacl = qemu_opt_get(opts, "tls-acl"); > > + if (acl && tlsacl) { > > + error_setg(errp, "'acl' option is mutually exclusive with the " > > + "'tls-acl' options"); > > + goto fail; > > + } > > + > > +#ifdef CONFIG_VNC_SASL > > + saslacl = qemu_opt_get(opts, "sasl-acl"); > > + if (acl && saslacl) { > > + error_setg(errp, "'acl' option is mutually exclusive with the " > > + "'sasl-acl' options"); > > + goto fail; > > + } > > +#endif > > Do we explicitly fail if sasl-acl was provided but CONFIG_VNC_SASL is > not defined? It looks here like you silently ignore it, which would not > be good. Yes, we should really raise the error unconditionally. > > @@ -3710,19 +3737,39 @@ void vnc_display_open(const char *id, Error **errp) > > &error_abort); > > } > > #ifdef CONFIG_VNC_SASL > > - if (acl && sasl) { > > - char *aclname; > > + if (sasl) { > > + if (saslacl) { > > + Object *container, *acl; > > + container = object_get_objects_root(); > > + acl = object_resolve_path_component(container, saslacl); > > + if (!acl) { > > + error_setg(errp, "Cannot find ACL %s", saslacl); > > + goto fail; > > + } > > > > - if (strcmp(vs->id, "default") == 0) { > > - aclname = g_strdup("vnc.username"); > > - } else { > > - aclname = g_strdup_printf("vnc.%s.username", vs->id); > > - } > > - vs->sasl.acl = > > - QAUTHZ(qauthz_simple_new(aclname, > > - QAUTHZ_SIMPLE_POLICY_DENY, > > - &error_abort)); > > - g_free(aclname); > > + if (!object_dynamic_cast(acl, TYPE_QAUTHZ)) { > > + error_setg(errp, "Object '%s' is not a QAuthZ subclass", > > + saslacl); > > + goto fail; > > + } > > + vs->sasl.acl = QAUTHZ(acl); > > + } else if (acl) { > > + char *aclname; > > + > > + if (strcmp(vs->id, "default") == 0) { > > + aclname = g_strdup("vnc.username"); > > + } else { > > + aclname = g_strdup_printf("vnc.%s.username", vs->id); > > + } > > + vs->sasl.acl = > > + QAUTHZ(qauthz_simple_new(aclname, > > + QAUTHZ_SIMPLE_POLICY_DENY, > > + &error_abort)); > > + g_free(aclname); > > + } > > + } else if (saslacl) { > > + error_setg(errp, "SASL ACL provided when SASL is disabled"); > > + goto fail; > > } > > #endif > > > > Again, the saslacl check is only mentioned inside the #if; what happens > when the #if is not compiled? Yeah, I should fix that. 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 :|