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 'j...@redhat.com' or 'f...@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=f...@redhat.com,rules.0.policy=allow \
> > rules.0.match=j...@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 <berra...@redhat.com>
> > ---
> > 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 :|
