On Mon, Feb 10, 2020 at 7:36 AM Christian Ehrhardt < christian.ehrha...@canonical.com> wrote:
> > > On Fri, Jan 24, 2020 at 2:15 PM Arnaud Patard <apat...@hupstream.com> > wrote: > >> Christian Ehrhardt <christian.ehrha...@canonical.com> writes: >> >> > On Thu, Dec 5, 2019 at 6:25 PM Arnaud Patard <apat...@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 <apat...@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@fedora~]# rpm -qf /etc/pki/nssdb/ >> > nss-3.47.0-2.fc29.x86_64 >> > [root@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@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.ehrha...@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