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

2020-01-16 Thread Christian Ehrhardt
On Thu, Dec 5, 2019 at 6:25 PM Arnaud Patard  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 
> 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(, "  \"/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.


+break;
> +case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
> +virBufferAsprintf(, "  \"%s/{,*}\" rk,\n", sc_db);
> +break;
>

>From https://libvirt.org/formatdomain.html#elementsSmartcard
"An additional sub-element  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.

Furthermore actually this lets us define:
  
cert1
cert2
cert3
/etc/pki/nssdb/
  

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(, "  \"%s/{,*}\" rk,\n", sc_db);
maybe something like this is safer:
iterate on certs-that-are-defined => cert
virBufferAsprintf(, "  \"%s/%s\" rk,\n", sc_db, cert);



> +/*
> + * Nothing to do for passthrough, as the smartcard
> + * access is done through TCP or Spice
> + */
> +case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> +break;
> +case VIR_DOMAIN_SMARTCARD_TYPE_LAST:
> +break;
> +}
> +}
> +
>  if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
>  for (i = 0; i < ctl->def->nnets; i++) {
>  virDomainNetDefPtr net = ctl->def->nets[i];
>
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2020-01-15 Thread Arnaud Patard
Cole Robinson  writes:

Hi,

> On 12/5/19 12:11 PM, Arnaud Patard 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 
>> 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).
>> + */
>
> This doesn't seem like the type of thing to track in a permanent code
> comment, nor a commit message, but as part of the email discussion.
> Otherwise, for the code because I don't have a test setup:
>
> Reviewed-by: Cole Robinson 
>
> If apparmor maintainers agree they can strip out of the comment so
> doesn't require a repost either way IMO

This patch doesn't seem to have been merged. Did it get lost or is it
waiting for me to resubmit it without the comment ?

Thanks,
Arnaud


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list



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

2019-12-10 Thread Cole Robinson
On 12/5/19 12:11 PM, Arnaud Patard 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 
> 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).
> + */

This doesn't seem like the type of thing to track in a permanent code
comment, nor a commit message, but as part of the email discussion.
Otherwise, for the code because I don't have a test setup:

Reviewed-by: Cole Robinson 

If apparmor maintainers agree they can strip out of the comment so
doesn't require a repost either way IMO

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list



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

2019-12-05 Thread Arnaud Patard
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 
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(, "  \"/etc/pki/nssdb/{,*}\" rk,\n");
+break;
+case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
+virBufferAsprintf(, "  \"%s/{,*}\" rk,\n", sc_db);
+break;
+/*
+ * Nothing to do for passthrough, as the smartcard
+ * access is done through TCP or Spice
+ */
+case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
+break;
+case VIR_DOMAIN_SMARTCARD_TYPE_LAST:
+break;
+}
+}
+
 if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
 for (i = 0; i < ctl->def->nnets; i++) {
 virDomainNetDefPtr net = ctl->def->nets[i];



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list