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