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

Christian Ehrhardt christian.ehrhardt at canonical.com
Wed Feb 12 10:12:25 UTC 2020


On Mon, Feb 10, 2020 at 7:36 AM Christian Ehrhardt <
christian.ehrhardt at canonical.com> wrote:

>
>
> On Fri, Jan 24, 2020 at 2:15 PM Arnaud Patard <apatard at hupstream.com>
> wrote:
>
>> Christian Ehrhardt <christian.ehrhardt at canonical.com> writes:
>>
>> > On Thu, Dec 5, 2019 at 6:25 PM Arnaud Patard <apatard at hupstream.com>
>> wrote:
>> >
>> >> When emulating smartcard with host certificates, qemu needs to
>> >> be able to read the certificates files. Add necessary code to
>> >> add the smartcard certificates file path to the apparmor profile.
>> >>
>> >> Passthrough support has been tested with spicevmc and remote-viewer.
>> >>
>> >> v2:
>> >> - Fix CodingStyle
>> >> - Add support for 'host' case.
>> >> - Add a comment to mention that the passthrough case doesn't need
>> >>   some configuration
>> >> - Use one rule with '{,*}' instead of two rules.
>> >>
>> >> Signed-off-by: Arnaud Patard <apatard at hupstream.com>
>> >> Index: libvirt/src/security/virt-aa-helper.c
>> >> ===================================================================
>> >> --- libvirt.orig/src/security/virt-aa-helper.c
>> >> +++ libvirt/src/security/virt-aa-helper.c
>> >> @@ -1271,6 +1271,39 @@ 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) {
>> >> +            /*
>> >> +             * Note: At time of writing, to get this working, qemu
>> >> seccomp sandbox has
>> >> +             * to be disabled or the host must be running QEMU with
>> commit
>> >> +             * 9a1565a03b79d80b236bc7cc2dbce52a2ef3a1b8.
>> >> +             * It's possibly due to
>> >> libcacard:vcard_emul_new_event_thread(), which calls
>> >> +             * PR_CreateThread(), which calls {g,s}etpriority(). And
>> >> resourcecontrol seccomp
>> >> +             * filter forbids it (cf src/qemu/qemu_command.c which
>> seems
>> >> to always use
>> >> +             * resourcecontrol=deny).
>> >> +             */
>> >> +            case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
>> >> +                virBufferAddLit(&buf, "  \"/etc/pki/nssdb/{,*}\"
>> rk,\n");
>> >>
>> >
>> > That path matches the examples in libvirt/qemu and also the fedora nss
>> > package
>> > [root at fedora~]# rpm -qf /etc/pki/nssdb/
>> > nss-3.47.0-2.fc29.x86_64
>> > [root at fedora ~]# ll /etc/pki/nssdb/
>> > total 8
>> > -rw-r--r-- 1 root root 65536 Jan  6  2017 cert8.db
>> > -rw-r--r-- 1 root root  9216 Jan  6  2017 cert9.db
>> > -rw-r--r-- 1 root root 16384 Jan  6  2017 key3.db
>> > -rw-r--r-- 1 root root 11264 Jan  6  2017 key4.db
>> > -rw-r--r-- 1 root root   451 Oct 23 11:23 pkcs11.txt
>> > -rw-r--r-- 1 root root 16384 Jan  6  2017 secmod.db
>> >
>> > But on Debian/Ubuntu the paths are slightly different.
>> > root at x:~# dpkg -L libnss3-nssdb
>> > [...]
>> > /var/lib/nssdb/key4.db
>> > /var/lib/nssdb/cert9.db
>> > /var/lib/nssdb/pkcs11.txt
>> > /var/lib/nssdb/secmod.db
>> >
>> > Therefore I'd ask you to add that path as well here.
>> >
>>
>> I don't remember seeing this /var/lib/nssdb path on my Debian. I don't
>> even find the libnss3-nssdb package. Is it Ubuntu-specific and
>> documented somewhere ?
>>
>
> Hmm, I was just checking paths on a multitude of containers and which
> package it belongs.
> After you wondered I checked more in detail - the package and the /var...
> path are only in older releases.
> So please feel free to ignore that part of my question.
>
>
>> I can add this path, I've no problem with that. Nevertheless, I'm
>> wondering that if it's really distribution specific (like done only on
>> Ubuntu), wouldn't it be better to have the distribution patch libvirt to
>> add this path ?
>>
>> >
>> > +                break;
>> >> +            case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
>> >> +                virBufferAsprintf(&buf, "  \"%s/{,*}\" rk,\n", sc_db);
>> >> +                break;
>> >>
>> >
>> >>From https://libvirt.org/formatdomain.html#elementsSmartcard
>> > "An additional sub-element <database> can specify the absolute path to
>> an
>> > alternate directory ... if not present, it defaults to /etc/pki/nssdb."
>> > That is in "VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE".
>> > Have you tested if sc_db is actually set to
>> > "VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE"
>> > in that case?
>> > If it is e.g. undefined we need to check for that and add
>> > "VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE"
>> > instead.
>>
>> I remember testing the case with certificates inside /etc/pki/nssdb and
>> not specifying the <database/> element at the early version of the
>> patch.
>>
>> >
>> > Furthermore actually this lets us define:
>> >   <smartcard mode='host-certificates'>
>> >     <certificate>cert1</certificate>
>> >     <certificate>cert2</certificate>
>> >     <certificate>cert3</certificate>
>> >     <database>/etc/pki/nssdb/</database>
>> >   </smartcard>
>> >
>> > There could be two guests using rather different cert[1-3] and there is
>> no
>> > need letting them cross read right?
>> > So instead of
>> >   virBufferAsprintf(&buf, "  \"%s/{,*}\" rk,\n", sc_db);
>> > maybe something like this is safer:
>> > iterate on certs-that-are-defined => cert
>> >     virBufferAsprintf(&buf, "  \"%s/%s\" rk,\n", sc_db, cert);
>>
>> Not sure to understand. The certificates are inside a SQLite database,
>> for instance:
>> $ ls /etc/pki/nssdb
>> cert9.db  key4.db  pkcs11.txt
>
> How your proposed change would be valid, since there's no specific file
>> for the certificate (for instance /etc/pki/nssdb/cert1). Is apparmor
>> able to filter the access of the content of a SQLite database ?
>>
>
> Hmm - yeah I thought that would be files, but you are right "...provide
> three NSS certificate names residing in a database..."
>
> The reason I was looking for an alternative is that I'm just generally a
> bit averse to most rules that end with *.
> Once we went as far as generating them (instead of adding them to the
> general template) you'd hope that we can make only explicit paths.
> But it seems if people need separation they have to do so via the paths
> and that is it.
>
> Thanks Arnaud for the clarifications that helped me to get the nss details
> I was missing.
> Acked-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
>
>
>> Arnaud
>
>
We have a review and an ack, no negative feedback left and a test build +
make check was fine.
No 6.1.0 freeze active yet, so I'm pushing this to master with the
Review/Ack statements added.


> --
> Christian Ehrhardt
> Staff Engineer, Ubuntu Server
> Canonical Ltd
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200212/1ee7488d/attachment-0001.htm>


More information about the libvir-list mailing list