On Wed, Jul 20, 2011 at 02:12:45PM +0100, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

* src/rpc/virnettlscontext.c: Fix mixed up error messages --- src/rpc/virnettlscontext.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index a40439b..402029f 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -194,7 +194,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, VIR_DEBUG("Cert %s key usage status %d usage %d", certFile, status, usage); if (status < 0) { virNetError(VIR_ERR_SYSTEM_ERROR, - _("Unable to query certificate %s basic constraints %s"), + _("Unable to query certificate %s key usage %s"), certFile, gnutls_strerror(status)); goto cleanup; } @@ -202,8 +202,8 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, if (usage & GNUTLS_KEY_KEY_CERT_SIGN) { if (!isCA) { virNetError(VIR_ERR_SYSTEM_ERROR, isServer ? - _("Certificate server usage is for certificate signing, but wanted a %s certificate") : - _("Certificate client usage is for certificate signing, but wanted a %s certificate"), + _("Certificate %s usage is for certificate signing, but wanted a server certificate") : + _("Certificate %s usage is for certificate signing, but wanted a client certificate"), certFile); goto cleanup; } @@ -248,25 +248,25 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, if (STREQ(buffer, GNUTLS_KP_TLS_WWW_SERVER)) { if (isCA || !isServer) { virNetError(VIR_ERR_SYSTEM_ERROR, isCA ? - _("Certificate CA purpose is TLS server, but wanted a %s certificate") : - _("Certificate TLS client purpose is TLS server, but wanted a %s certificate"), + _("Certificate %s purpose is TLS server, but wanted a CA certificate") : + _("Certificate %s client purpose is TLS server, but wanted a TLS client certificate"), certFile); goto cleanup; } } else if (STREQ(buffer, GNUTLS_KP_TLS_WWW_CLIENT)) { if (isCA || isServer) { virNetError(VIR_ERR_SYSTEM_ERROR, isCA ? - _("Certificate CA purpose is TLS client, but wanted a %s certificate") : - _("Certificate TLS server purpose is TLS client, but wanted a %s certificate"), + _("Certificate %s purpose is TLS client, but wanted a CA certificate") : + _("Certificate %s server purpose is TLS client, but wanted a TLS server certificate"), certFile); goto cleanup; } } else if (STRNEQ(buffer, GNUTLS_KP_ANY)) { virNetError(VIR_ERR_SYSTEM_ERROR, (isCA ? - _("Certificate CA purpose is wrong, wanted a %s certificate") : + _("Certificate %s purpose is wrong, wanted a CA certificate") : (isServer ? - _("Certificate TLS server purpose is wrong, wanted a %s certificate") : - _("Certificate TLS client purpose is wrong, wanted a %s certificate"))), + _("Certificate %s purpose is wrong, wanted a TLS server certificate") : + _("Certificate %s purpose is wrong, wanted a TLS client certificate"))), certFile); goto cleanup; }

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Wed, Jul 20, 2011 at 02:12:46PM +0100, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

If key usage or purpose data is not present in the cert, the RFC recommends that access be allowed. Also fix checking of key usage to include requirements for client/server certs, and fix key purpose checking to treat data as a list of bits --- src/rpc/virnettlscontext.c | 78 +++++++++++++++++++++++++------------------ 1 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 402029f..1ee5ab2 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -111,6 +111,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, char *buffer = NULL; size_t size; unsigned int usage; + bool allowClient = false, allowServer = false;

VIR_DEBUG("isServer %d isCA %d certFile %s", isServer, isCA, certFile); @@ -193,24 +194,34 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,

VIR_DEBUG("Cert %s key usage status %d usage %d", certFile, status, usage); if (status < 0) { - virNetError(VIR_ERR_SYSTEM_ERROR, - _("Unable to query certificate %s key usage %s"), - certFile, gnutls_strerror(status)); - goto cleanup; + if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { + usage = isCA ? GNUTLS_KEY_KEY_CERT_SIGN : + GNUTLS_KEY_DIGITAL_SIGNATURE|GNUTLS_KEY_KEY_ENCIPHERMENT; + } else { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Unable to query certificate %s key usage %s"), + certFile, gnutls_strerror(status)); + goto cleanup; + } }

- if (usage & GNUTLS_KEY_KEY_CERT_SIGN) { - if (!isCA) { - virNetError(VIR_ERR_SYSTEM_ERROR, isServer ? - _("Certificate %s usage is for certificate signing, but wanted a server certificate") : - _("Certificate %s usage is for certificate signing, but wanted a client certificate"), + if (isCA) { + if (!(usage & GNUTLS_KEY_KEY_CERT_SIGN)) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s usage does not permit certificate signing"), certFile); goto cleanup; } } else { - if (isCA) { + if (!(usage & GNUTLS_KEY_DIGITAL_SIGNATURE)) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s usage does not permit digital signature"), + certFile); + goto cleanup; + } + if (!(usage & GNUTLS_KEY_KEY_ENCIPHERMENT)) { virNetError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %s usage is for not certificate signing"), + _("Certificate %s usage does not permit key encipherment"), certFile); goto cleanup; } @@ -221,7 +232,11 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL);

if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { - VIR_DEBUG("No key purpose data available, skipping checks"); + VIR_DEBUG("No key purpose data available at slot %d", i); + + /* If there is no data at all, then we must allow client/server to pass */ + if (i == 0) + allowServer = allowClient = true; break; } if (status != GNUTLS_E_SHORT_MEMORY_BUFFER) { @@ -246,34 +261,31 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,

VIR_DEBUG("Key purpose %d %s", status, buffer); if (STREQ(buffer, GNUTLS_KP_TLS_WWW_SERVER)) { - if (isCA || !isServer) { - virNetError(VIR_ERR_SYSTEM_ERROR, isCA ? - _("Certificate %s purpose is TLS server, but wanted a CA certificate") : - _("Certificate %s client purpose is TLS server, but wanted a TLS client certificate"), - certFile); - goto cleanup; - } + allowServer = true; } else if (STREQ(buffer, GNUTLS_KP_TLS_WWW_CLIENT)) { - if (isCA || isServer) { - virNetError(VIR_ERR_SYSTEM_ERROR, isCA ? - _("Certificate %s purpose is TLS client, but wanted a CA certificate") : - _("Certificate %s server purpose is TLS client, but wanted a TLS server certificate"), - certFile); - goto cleanup; - } + allowClient = true; } else if (STRNEQ(buffer, GNUTLS_KP_ANY)) { - virNetError(VIR_ERR_SYSTEM_ERROR, (isCA ? - _("Certificate %s purpose is wrong, wanted a CA certificate") : - (isServer ? - _("Certificate %s purpose is wrong, wanted a TLS server certificate") : - _("Certificate %s purpose is wrong, wanted a TLS client certificate"))), - certFile); - goto cleanup; + allowServer = allowClient = true; }

VIR_FREE(buffer); }

+ if (!isCA) { /* No purpose checks required for CA certs */ + if (isServer && !allowServer) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s purpose does not allow use for with a TLS server"), + certFile); + goto cleanup; + } + if (!isServer && !allowClient) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s purpose does not allow use for with a TLS client"), + certFile); + goto cleanup; + } + } +

ret = 0;

Okay, not a trivial change but relax the contraints, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Wed, Jul 20, 2011 at 02:12:47PM +0100, Daniel P. Berrange wrote:

From: "Daniel P. Berrange" <berrange@redhat.com>

If a key purpose or usage field is marked as non-critical in the certificate, then a data mismatch is not (ordinarily) a cause for rejecting the connection

* src/rpc/virnettlscontext.c: Honour key usage/purpose criticality --- src/rpc/virnettlscontext.c | 78 ++++++++++++++++++++++++++++++------------- 1 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 1ee5ab2..1622bad 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -111,6 +111,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, char *buffer = NULL; size_t size; unsigned int usage; + unsigned int critical; bool allowClient = false, allowServer = false;

VIR_DEBUG("isServer %d isCA %d certFile %s", @@ -190,9 +191,9 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, goto cleanup; }

- status = gnutls_x509_crt_get_key_usage(cert, &usage, NULL); + status = gnutls_x509_crt_get_key_usage(cert, &usage, &critical);

Okay, I was wondering if we were missing something by passing NULL and we did :-)

- VIR_DEBUG("Cert %s key usage status %d usage %d", certFile, status, usage); + VIR_DEBUG("Cert %s key usage status %d usage %d critical %u", certFile, status, usage, critical); if (status < 0) { if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { usage = isCA ? GNUTLS_KEY_KEY_CERT_SIGN : @@ -207,28 +208,45 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,

if (isCA) { if (!(usage & GNUTLS_KEY_KEY_CERT_SIGN)) { - virNetError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %s usage does not permit certificate signing"), - certFile); - goto cleanup; + if (critical) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s usage does not permit certificate signing"), + certFile); + goto cleanup; + } else { + VIR_WARN(_("Certificate %s usage does not permit certificate signing"), + certFile); + } } } else { if (!(usage & GNUTLS_KEY_DIGITAL_SIGNATURE)) { - virNetError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %s usage does not permit digital signature"), - certFile); - goto cleanup; + if (critical) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s usage does not permit digital signature"), + certFile); + goto cleanup; + } else { + VIR_WARN(_("Certificate %s usage does not permit digital signature"), + certFile); + } } if (!(usage & GNUTLS_KEY_KEY_ENCIPHERMENT)) { - virNetError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %s usage does not permit key encipherment"), - certFile); - goto cleanup; + if (critical) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s usage does not permit key encipherment"), + certFile); + goto cleanup; + } else { + VIR_WARN(_("Certificate %s usage does not permit key encipherment"), + certFile); + } } }

+ critical = 0; for (i = 0 ; ; i++) { size = 0; + unsigned int purposeCritical; status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL);

if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { @@ -251,15 +269,17 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, goto cleanup; }

- status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL); + status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, &purposeCritical); if (status < 0) { virNetError(VIR_ERR_SYSTEM_ERROR, _("Unable to query certificate %s key purpose %s"), certFile, gnutls_strerror(status)); goto cleanup; } + if (purposeCritical) + critical = true;

- VIR_DEBUG("Key purpose %d %s", status, buffer); + VIR_DEBUG("Key purpose %d %s critical %u", status, buffer, purposeCritical); if (STREQ(buffer, GNUTLS_KP_TLS_WWW_SERVER)) { allowServer = true; } else if (STREQ(buffer, GNUTLS_KP_TLS_WWW_CLIENT)) { @@ -273,16 +293,26 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,

if (!isCA) { /* No purpose checks required for CA certs */ if (isServer && !allowServer) { - virNetError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %s purpose does not allow use for with a TLS server"), - certFile); - goto cleanup; + if (critical) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s purpose does not allow use for with a TLS server"), + certFile); + goto cleanup; + } else { + VIR_WARN(_("Certificate %s purpose does not allow use for with a TLS server"), + certFile); + } } if (!isServer && !allowClient) { - virNetError(VIR_ERR_SYSTEM_ERROR, - _("Certificate %s purpose does not allow use for with a TLS client"), - certFile); - goto cleanup; + if (critical) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s purpose does not allow use for with a TLS client"), + certFile); + goto cleanup; + } else { + VIR_WARN(_("Certificate %s purpose does not allow use for with a TLS client"), + certFile); + } } }

Okay, the only extra caution I would do are initialize critical and purposeCritical to 0 at declaration time to make sure they are initialized all the time though the gnutls calls should really set them. ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/