Refactor the certification validation code

There is some commonality between the code for sanity checking
certs when initializing libvirt and the code for validating
certs during a live TLS session handshake. This patchset splits
up the sanity checking function into several smaller functions
each doing a specific type of check. The cert validation code
is then updated to also call into these functions

* src/rpc/virnettlscontext.c: Refactor cert validation code

src/rpc/virnettlscontext.c

@@ -97,68 +97,51 @@ static void virNetTLSLog(int level, const char *str) {
 }
 
 
-static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
-                                                         bool isCA,
-                                                         const char *certFile)
+static int virNetTLSContextCheckCertTimes(gnutls_x509_crt_t cert,
+                                          const char *certFile,
+                                          bool isServer,
+                                          bool isCA)
 {
-    gnutls_datum_t data;
-    gnutls_x509_crt_t cert = NULL;
-    char *buf = NULL;
-    int ret = -1;
     time_t now;
-    int status;
-    int i;
-    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",
-              isServer, isCA, certFile);
 
     if ((now = time(NULL)) == ((time_t)-1)) {
         virReportSystemError(errno, "%s",
                              _("cannot get current time"));
-        goto cleanup;
-    }
-
-    if (gnutls_x509_crt_init(&cert) < 0) {
-        virNetError(VIR_ERR_SYSTEM_ERROR, "%s",
-                    _("Unable to initialize certificate"));
-        goto cleanup;
-    }
-
-    if (virFileReadAll(certFile, (1<<16), &buf) < 0)
-        goto cleanup;
-
-    data.data = (unsigned char *)buf;
-    data.size = strlen(buf);
-
-    if (gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM) < 0) {
-        virNetError(VIR_ERR_SYSTEM_ERROR, isServer ?
-                    _("Unable to import server certificate %s") :
-                    _("Unable to import client certificate %s"),
-                    certFile);
-        goto cleanup;
+        return -1;
     }
 
     if (gnutls_x509_crt_get_expiration_time(cert) < now) {
-        virNetError(VIR_ERR_SYSTEM_ERROR, isServer ?
+        virNetError(VIR_ERR_SYSTEM_ERROR,
+                    (isCA ?
+                     _("The CA certificate %s has expired") :
+                     (isServer ?
                       _("The server certificate %s has expired") :
-                    _("The client certificate %s has expired"),
+                      _("The client certificate %s has expired"))),
                     certFile);
-        goto cleanup;
+        return -1;
     }
 
     if (gnutls_x509_crt_get_activation_time(cert) > now) {
-        virNetError(VIR_ERR_SYSTEM_ERROR, isServer ?
+        virNetError(VIR_ERR_SYSTEM_ERROR,
+                    (isCA ?
+                     _("The CA certificate %s is not yet active") :
+                     (isServer ?
                       _("The server certificate %s is not yet active") :
-                    _("The client certificate %s is not yet active"),
+                      _("The client certificate %s is not yet active"))),
                     certFile);
-        goto cleanup;
+        return -1;
     }
 
+    return 0;
+}
+
+static int virNetTLSContextCheckCertBasicConstraints(gnutls_x509_crt_t cert,
+                                                     const char *certFile,
+                                                     bool isServer,
+                                                     bool isCA)
+{
+    int status;
+
     status = gnutls_x509_crt_get_basic_constraints(cert, NULL, NULL, NULL);
     VIR_DEBUG("Cert %s basic constraints %d", certFile, status);
 
@@ -168,29 +151,40 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
                         _("The certificate %s basic constraints show a CA, but we need one for a server") :
                         _("The certificate %s basic constraints show a CA, but we need one for a client"),
                         certFile);
-            goto cleanup;
+            return -1;
         }
     } else if (status == 0) { /* It is not a CA cert */
         if (isCA) {
             virNetError(VIR_ERR_SYSTEM_ERROR,
                         _("The certificate %s basic constraints do not show a CA"),
                         certFile);
-            goto cleanup;
+            return -1;
         }
     } else if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { /* Missing basicConstraints */
         if (isCA) {
             virNetError(VIR_ERR_SYSTEM_ERROR,
                         _("The certificate %s is missing basic constraints for a CA"),
                         certFile);
-            goto cleanup;
+            return -1;
         }
     } else { /* General error */
         virNetError(VIR_ERR_SYSTEM_ERROR,
                     _("Unable to query certificate %s basic constraints %s"),
                     certFile, gnutls_strerror(status));
-        goto cleanup;
+        return -1;
     }
 
+    return 0;
+}
+
+static int virNetTLSContextCheckCertKeyUsage(gnutls_x509_crt_t cert,
+                                             const char *certFile,
+                                             bool isCA)
+{
+    int status;
+    unsigned int usage;
+    unsigned int critical;
+
     status = gnutls_x509_crt_get_key_usage(cert, &usage, &critical);
 
     VIR_DEBUG("Cert %s key usage status %d usage %d critical %u", certFile, status, usage, critical);
@@ -202,7 +196,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
             virNetError(VIR_ERR_SYSTEM_ERROR,
                         _("Unable to query certificate %s key usage %s"),
                         certFile, gnutls_strerror(status));
-            goto cleanup;
+            return -1;
         }
     }
 
@@ -212,7 +206,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
                 virNetError(VIR_ERR_SYSTEM_ERROR,
                             _("Certificate %s usage does not permit certificate signing"),
                             certFile);
-                goto cleanup;
+                return -1;
             } else {
                 VIR_WARN("Certificate %s usage does not permit certificate signing",
                          certFile);
@@ -224,7 +218,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
                 virNetError(VIR_ERR_SYSTEM_ERROR,
                             _("Certificate %s usage does not permit digital signature"),
                             certFile);
-                goto cleanup;
+                return -1;
             } else {
                 VIR_WARN("Certificate %s usage does not permit digital signature",
                          certFile);
@@ -235,7 +229,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
                 virNetError(VIR_ERR_SYSTEM_ERROR,
                             _("Certificate %s usage does not permit key encipherment"),
                             certFile);
-                goto cleanup;
+                return -1;
             } else {
                 VIR_WARN("Certificate %s usage does not permit key encipherment",
                          certFile);
@@ -243,10 +237,25 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
         }
     }
 
+    return 0;
+}
+
+
+static int virNetTLSContextCheckCertKeyPurpose(gnutls_x509_crt_t cert,
+                                               const char *certFile,
+                                               bool isServer)
+{
+    int status;
+    int i;
+    unsigned int purposeCritical;
+    unsigned int critical;
+    char *buffer;
+    size_t size;
+    bool allowClient = false, allowServer = false;
+
     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) {
@@ -261,20 +270,21 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
             virNetError(VIR_ERR_SYSTEM_ERROR,
                         _("Unable to query certificate %s key purpose %s"),
                         certFile, gnutls_strerror(status));
-            goto cleanup;
+            return -1;
         }
 
         if (VIR_ALLOC_N(buffer, size) < 0) {
             virReportOOMError();
-            goto cleanup;
+            return -1;
         }
 
         status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, &purposeCritical);
         if (status < 0) {
+            VIR_FREE(buffer);
             virNetError(VIR_ERR_SYSTEM_ERROR,
                         _("Unable to query certificate %s key purpose %s"),
                         certFile, gnutls_strerror(status));
-            goto cleanup;
+            return -1;
         }
         if (purposeCritical)
             critical = true;
@@ -291,24 +301,25 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
         VIR_FREE(buffer);
     }
 
-    if (!isCA) { /* No purpose checks required for CA certs */
-        if (isServer && !allowServer) {
+    if (isServer) {
+        if (!allowServer) {
             if (critical) {
                 virNetError(VIR_ERR_SYSTEM_ERROR,
                             _("Certificate %s purpose does not allow use for with a TLS server"),
                             certFile);
-                goto cleanup;
+                return -1;
             } else {
                 VIR_WARN("Certificate %s purpose does not allow use for with a TLS server",
                          certFile);
             }
         }
-        if (!isServer && !allowClient) {
+    } else {
+        if (!allowClient) {
             if (critical) {
                 virNetError(VIR_ERR_SYSTEM_ERROR,
                             _("Certificate %s purpose does not allow use for with a TLS client"),
                             certFile);
-                goto cleanup;
+                return -1;
             } else {
                 VIR_WARN("Certificate %s purpose does not allow use for with a TLS client",
                          certFile);
@@ -316,47 +327,121 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
         }
     }
 
+    return 0;
+}
 
-    ret = 0;
+/* Check DN is on tls_allowed_dn_list. */
+static int
+virNetTLSContextCheckCertDNWhitelist(const char *dname,
+                                     const char *const*wildcards)
+{
+    while (*wildcards) {
+        int ret = fnmatch (*wildcards, dname, 0);
+        if (ret == 0) /* Succesful match */
+            return 1;
+        if (ret != FNM_NOMATCH) {
+            virNetError(VIR_ERR_INTERNAL_ERROR,
+                        _("Malformed TLS whitelist regular expression '%s'"),
+                        *wildcards);
+            return -1;
+        }
 
-cleanup:
-    if (ret != 0) {
-        gnutls_x509_crt_deinit(cert);
-        cert = NULL;
+        wildcards++;
     }
-    VIR_FREE(buffer);
-    VIR_FREE(buf);
-    return cert;
+
+    /* Log the client's DN for debugging */
+    VIR_DEBUG("Failed whitelist check for client DN '%s'", dname);
+
+    /* This is the most common error: make it informative. */
+    virNetError(VIR_ERR_SYSTEM_ERROR, "%s",
+                _("Client's Distinguished Name is not on the list "
+                  "of allowed clients (tls_allowed_dn_list).  Use "
+                  "'certtool -i --infile clientcert.pem' to view the"
+                  "Distinguished Name field in the client certificate,"
+                  "or run this daemon with --verbose option."));
+    return 0;
 }
 
 
-static int virNetTLSContextSanityCheckCredentials(bool isServer,
-                                                  const char *cacertFile,
-                                                  const char *certFile)
+static int
+virNetTLSContextCheckCertDN(gnutls_x509_crt_t cert,
+                            const char *certFile,
+                            const char *hostname,
+                            const char *const* whitelist)
 {
-    gnutls_x509_crt_t cert = NULL;
-    gnutls_x509_crt_t cacert = NULL;
-    int ret = -1;
-    unsigned int status;
+    int ret;
+    char name[256];
+    size_t namesize = sizeof name;
 
-    if (access(certFile, R_OK) == 0) {
-        if (!(cert = virNetTLSContextSanityCheckCert(isServer, false, certFile)))
-            goto cleanup;
+    memset(name, 0, namesize);
+
+    ret = gnutls_x509_crt_get_dn(cert, name, &namesize);
+    if (ret != 0) {
+        virNetError(VIR_ERR_SYSTEM_ERROR,
+                    _("Failed to get certificate %s distinguished name: %s"),
+                    certFile, gnutls_strerror(ret));
+        return -1;
     }
-    if (access(cacertFile, R_OK) == 0) {
-        if (!(cacert = virNetTLSContextSanityCheckCert(isServer, true, cacertFile)))
-            goto cleanup;
+
+    if (whitelist &&
+        virNetTLSContextCheckCertDNWhitelist(name, whitelist) <= 0)
+        return -1;
+
+    if (hostname &&
+        !gnutls_x509_crt_check_hostname(cert, hostname)) {
+        virNetError(VIR_ERR_RPC,
+                    _("Certificate %s owner does not match the hostname %s"),
+                    certFile, hostname);
+        return -1;
     }
 
-    if (cert && cacert) {
+    return 0;
+}
+
+
+static int virNetTLSContextCheckCert(gnutls_x509_crt_t cert,
+                                     const char *certFile,
+                                     bool isServer,
+                                     bool isCA)
+{
+    if (virNetTLSContextCheckCertTimes(cert, certFile,
+                                       isServer, isCA) < 0)
+        return -1;
+
+    if (virNetTLSContextCheckCertBasicConstraints(cert, certFile,
+                                                  isServer, isCA) < 0)
+        return -1;
+
+    if (virNetTLSContextCheckCertKeyUsage(cert, certFile,
+                                          isCA) < 0)
+        return -1;
+
+    if (!isCA &&
+        virNetTLSContextCheckCertKeyPurpose(cert, certFile,
+                                            isServer) < 0)
+        return -1;
+
+    return 0;
+}
+
+
+static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t cert,
+                                         const char *certFile,
+                                         gnutls_x509_crt_t cacert,
+                                         const char *cacertFile,
+                                         bool isServer)
+{
+    unsigned int status;
+
     if (gnutls_x509_crt_list_verify(&cert, 1,
                                     &cacert, 1,
                                     NULL, 0,
                                     0, &status) < 0) {
-            virNetError(VIR_ERR_SYSTEM_ERROR, "%s", isServer ?
-                        _("Unable to verify server certificate against CA certificate") :
-                        _("Unable to verify client certificate against CA certificate"));
-            goto cleanup;
+        virNetError(VIR_ERR_SYSTEM_ERROR, isServer ?
+                    _("Unable to verify server certificate %s against CA certificate %s") :
+                    _("Unable to verify client certificate %s against CA certificate %s"),
+                    certFile, cacertFile);
+        return -1;
     }
 
     if (status != 0) {
@@ -379,9 +464,83 @@ static int virNetTLSContextSanityCheckCredentials(bool isServer,
         virNetError(VIR_ERR_SYSTEM_ERROR,
                     _("Our own certificate %s failed validation against %s: %s"),
                     certFile, cacertFile, reason);
+        return -1;
+    }
+
+    return 0;
+}
+
+
+static gnutls_x509_crt_t virNetTLSContextLoadCertFromFile(const char *certFile,
+                                                          bool isServer,
+                                                          bool isCA)
+{
+    gnutls_datum_t data;
+    gnutls_x509_crt_t cert = NULL;
+    char *buf = NULL;
+    int ret = -1;
+
+    VIR_DEBUG("isServer %d isCA %d certFile %s",
+              isServer, isCA, certFile);
+
+    if (gnutls_x509_crt_init(&cert) < 0) {
+        virNetError(VIR_ERR_SYSTEM_ERROR, "%s",
+                    _("Unable to initialize certificate"));
+        goto cleanup;
+    }
+
+    if (virFileReadAll(certFile, (1<<16), &buf) < 0)
+        goto cleanup;
+
+    data.data = (unsigned char *)buf;
+    data.size = strlen(buf);
+
+    if (gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM) < 0) {
+        virNetError(VIR_ERR_SYSTEM_ERROR, isServer ?
+                    _("Unable to import server certificate %s") :
+                    _("Unable to import client certificate %s"),
+                    certFile);
         goto cleanup;
     }
+
+    ret = 0;
+
+cleanup:
+    if (ret != 0) {
+        gnutls_x509_crt_deinit(cert);
+        cert = NULL;
     }
+    VIR_FREE(buf);
+    return cert;
+}
+
+
+static int virNetTLSContextSanityCheckCredentials(bool isServer,
+                                                  const char *cacertFile,
+                                                  const char *certFile)
+{
+    gnutls_x509_crt_t cert = NULL;
+    gnutls_x509_crt_t cacert = NULL;
+    int ret = -1;
+
+    if ((access(certFile, R_OK) == 0) &&
+        !(cert = virNetTLSContextLoadCertFromFile(certFile, isServer, false)))
+        goto cleanup;
+    if ((access(cacertFile, R_OK) == 0) &&
+        !(cacert = virNetTLSContextLoadCertFromFile(cacertFile, isServer, false)))
+        goto cleanup;
+
+    if (cert &&
+        virNetTLSContextCheckCert(cert, certFile, isServer, false) < 0)
+        goto cleanup;
+
+    if (cacert &&
+        virNetTLSContextCheckCert(cacert, cacertFile, isServer, true) < 0)
+        goto cleanup;
+
+    if (cert && cacert &&
+        virNetTLSContextCheckCertPair(cert, certFile, cacert, cacertFile, isServer) < 0)
+        goto cleanup;
 
     ret = 0;
 
@@ -760,45 +919,6 @@ void virNetTLSContextRef(virNetTLSContextPtr ctxt)
 }
 
 
-/* Check DN is on tls_allowed_dn_list. */
-static int
-virNetTLSContextCheckDN(virNetTLSContextPtr ctxt,
-                        const char *dname)
-{
-    const char *const*wildcards;
-
-    /* If the list is not set, allow any DN. */
-    wildcards = ctxt->x509dnWhitelist;
-    if (!wildcards)
-        return 1;
-
-    while (*wildcards) {
-        int ret = fnmatch (*wildcards, dname, 0);
-        if (ret == 0) /* Succesful match */
-            return 1;
-        if (ret != FNM_NOMATCH) {
-            virNetError(VIR_ERR_INTERNAL_ERROR,
-                        _("Malformed TLS whitelist regular expression '%s'"),
-                        *wildcards);
-            return -1;
-        }
-
-        wildcards++;
-    }
-
-    /* Log the client's DN for debugging */
-    VIR_DEBUG("Failed whitelist check for client DN '%s'", dname);
-
-    /* This is the most common error: make it informative. */
-    virNetError(VIR_ERR_SYSTEM_ERROR, "%s",
-                _("Client's Distinguished Name is not on the list "
-                  "of allowed clients (tls_allowed_dn_list).  Use "
-                  "'certtool -i --infile clientcert.pem' to view the"
-                  "Distinguished Name field in the client certificate,"
-                  "or run this daemon with --verbose option."));
-    return 0;
-}
-
 static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt,
                                             virNetTLSSessionPtr sess)
 {
@@ -806,11 +926,6 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt,
     unsigned int status;
     const gnutls_datum_t *certs;
     unsigned int nCerts, i;
-    time_t now;
-    char name[256];
-    size_t namesize = sizeof name;
-
-    memset(name, 0, namesize);
 
     if ((ret = gnutls_certificate_verify_peers2(sess->session, &status)) < 0){
         virNetError(VIR_ERR_SYSTEM_ERROR,
@@ -819,12 +934,6 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt,
         goto authdeny;
     }
 
-    if ((now = time(NULL)) == ((time_t)-1)) {
-        virReportSystemError(errno, "%s",
-                             _("cannot get current time"));
-        goto authfail;
-    }
-
     if (status != 0) {
         const char *reason = _("Invalid certificate");
 
@@ -876,46 +985,37 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt,
             goto authfail;
         }
 
-        if (gnutls_x509_crt_get_expiration_time(cert) < now) {
-            /* Warning is reversed from what you expect, since with
-             * this code it is the Server checking the client and
-             * vica-versa */
-            virNetError(VIR_ERR_SYSTEM_ERROR, "%s", sess->isServer ?
-                        _("The client certificate has expired") :
-                        _("The server certificate has expired"));
+        if (virNetTLSContextCheckCertTimes(cert, "[session]",
+                                           sess->isServer, i > 0) < 0) {
             gnutls_x509_crt_deinit(cert);
             goto authdeny;
         }
 
-        if (gnutls_x509_crt_get_activation_time(cert) > now) {
-            /* client/server order reversed. see above */
-            virNetError(VIR_ERR_SYSTEM_ERROR, "%s", sess->isServer ?
-                        _("The client certificate is not yet active") :
-                        _("The server certificate is not yet active"));
+        if (i == 0) {
+            if (virNetTLSContextCheckCertDN(cert, "[session]", sess->hostname,
+                                            ctxt->x509dnWhitelist) < 0) {
                 gnutls_x509_crt_deinit(cert);
                 goto authdeny;
             }
 
-        if (i == 0) {
-            ret = gnutls_x509_crt_get_dn(cert, name, &namesize);
-            if (ret != 0) {
-                virNetError(VIR_ERR_SYSTEM_ERROR,
-                            _("Failed to get certificate distinguished name: %s"),
-                            gnutls_strerror(ret));
+            /* !sess->isServer, since on the client, we're validating the
+             * server's cert, and on the server, the client's cert
+             */
+            if (virNetTLSContextCheckCertBasicConstraints(cert, "[session]",
+                                                          !sess->isServer, false) < 0) {
                 gnutls_x509_crt_deinit(cert);
-                goto authfail;
+                goto authdeny;
             }
 
-            if (virNetTLSContextCheckDN(ctxt, name) <= 0) {
+            if (virNetTLSContextCheckCertKeyUsage(cert, "[session]",
+                                                  false) < 0) {
                 gnutls_x509_crt_deinit(cert);
                 goto authdeny;
             }
 
-            if (sess->hostname &&
-                !gnutls_x509_crt_check_hostname(cert, sess->hostname)) {
-                virNetError(VIR_ERR_RPC,
-                            _("Certificate's owner does not match the hostname (%s)"),
-                            sess->hostname);
+            /* !sess->isServer - as above */
+            if (virNetTLSContextCheckCertKeyPurpose(cert, "[session]",
+                                                    !sess->isServer) < 0) {
                 gnutls_x509_crt_deinit(cert);
                 goto authdeny;
             }