[libvirt] [patch 1/1] virt-aa-helper: Add support for smartcard host-certificates

[Jamie Strandboge]

On Thu, 14 Nov 2019, Cole Robinson wrote:

> On 10/24/19 4:57 AM, Arnaud Patard wrote:
> > When emulating smartcard with host certificates, qemu needs to
> > be able to read the certificates files, which is denied by apparmor.
> > Add necessary code to add the smartcard certificates related directory
> > to the apparmor profile.
> > 
> > This code supports only this case smartcard 'host' and 'passthrough'
> > settings are not supported, as I can't test them.
> > 
> > Signed-off-by: Arnaud Patard <apatard at hupstream.com>
> > Index: libvirt-5.0.0/src/security/virt-aa-helper.c
> > ===================================================================
> > --- libvirt-5.0.0.orig/src/security/virt-aa-helper.c
> > +++ libvirt-5.0.0/src/security/virt-aa-helper.c
> > @@ -1251,6 +1251,26 @@ get_files(vahControl * ctl)
> >          }
> >      }
> >  
> > +    for (i = 0; i < ctl->def->nsmartcards; i++) {
> > +        virDomainSmartcardDefPtr sc = ctl->def->smartcards[i];
> > +        virDomainSmartcardType sc_type = sc->type;
> > +        char *sc_db = (char *)VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
> > +        if (sc->data.cert.database)
> > +            sc_db = sc->data.cert.database;
> > +        switch(sc_type) {
> 
> Add a space after 'switch'. 'make syntax-check' will catch this. libvirt
> style is typically to not indent the 'case' keyword either, but this
> file is inconsistent on that front. With those fixed:
> 
> Reviewed-by: Cole Robinson <crobinso at redhat.com>
> 
> This matches what is done for the selinux driver AFAICT
> 
> CCing apparmor maintainers, I'll defer to them to commit
> 
> - Cole
> 
> > +            case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
> > +                break;
> > +            case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
> > +                virBufferAsprintf(&buf, "  \"%s/\" rk,\n", sc_db);
> > +                virBufferAsprintf(&buf, "  \"%s/*\" rk,\n", sc_db);
> > +                break;

I'll let other comment on the code changes, but these rules look ok. I
might suggest using this one line instead:

  virBufferAsprintf(&buf, "  \"%s/{,*}" rk,\n", sc_db);

Is it possible that the certificates might be in a lower directory? Ie,
is '**' warranted?

> > +            case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> > +                break;
> > +            case VIR_DOMAIN_SMARTCARD_TYPE_LAST:
> > +                break;
> > +        }
> > +    }

It probably makes sense to add a code comment on why
VIR_DOMAIN_SMARTCARD_TYPE_HOST, VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH
and VIR_DOMAIN_SMARTCARD_TYPE_LAST aren't supported and ignored.

-- 
Jamie Strandboge             | http://www.canonical.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191119/068997ba/attachment-0001.sig>