[libvirt] [patch 1/1] virt-aa-helper: Add support for smartcard host-certificates
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@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) { + 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; + 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];
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@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@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; + 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
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@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@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); Is it possible that the certificates might be in a lower directory? Ie, is '**' warranted?
+ 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. -- Jamie Strandboge | http://www.canonical.com
Jamie Strandboge <jamie@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@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@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.
participants (3)
-
Arnaud Patard -
Cole Robinson -
Jamie Strandboge