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

[Arnaud Patard]

Jamie Strandboge <jamie at canonical.com> writes:

> 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);
>

Ok, will update my patch to use this.

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

In the case of host certificates, the database parameter corresponds to
a NSS db directory, for instance:

$ ls <database directory>/
cert9.db  key4.db  pkcs11.txt

The certificate are in the db files and the name given in the xml are
the names of the certificates in the db:

$ certutil -L -d sql:<database directory>

Certificate Nickname                                         Trust
Attributes
                                                             SSL,S/MIME,JAR/XPI

cert1                                                        CTu,Cu,Cu
cert2                                                        CTu,Cu,Cu
cert3                                                        CTu,Cu,Cu

This means only the database directory is interesting.

>> > +            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.

ok, will add a comment about not being able to test theses cases.

Arnaud.