rpc: move file access checks into TLS config API

A future patch will require fule access checks to be done
as part of locating the certificate files, as we will have
the ability to load many more files, most of which will be
optional.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/po/POTFILES b/po/POTFILES
index 23da794f842d4f95c2aea6950577741256444ae4..f0aad35c8cf163bc1387964a9985c08b4c9b11fd 100644 (file)
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -230,6 +230,7 @@ src/rpc/virnetserverservice.c
 src/rpc/virnetsocket.c
 src/rpc/virnetsshsession.c
 src/rpc/virnettlscert.c
+src/rpc/virnettlsconfig.c
 src/rpc/virnettlscontext.c
 src/secret/secret_driver.c
 src/security/security_apparmor.c

diff --git a/src/rpc/virnettlsconfig.c b/src/rpc/virnettlsconfig.c
index ffab3b4fc8b353a1de670d9843514515d730bd70..1479eb01aee1262fdd430750b891b8bd382709f8 100644 (file)
--- a/src/rpc/virnettlsconfig.c
+++ b/src/rpc/virnettlsconfig.c
@@ -21,12 +21,15 @@
 #include <config.h>
 
 #include "virnettlsconfig.h"
+#include "viralloc.h"
 #include "virlog.h"
 #include "virutil.h"
+#include "virfile.h"
+#include "virerror.h"
 
 #define VIR_FROM_THIS VIR_FROM_RPC
 
-VIR_LOG_INIT("rpc.nettlscontext");
+VIR_LOG_INIT("rpc.nettlsconfig");
 
 char *virNetTLSConfigUserPKIBaseDir(void)
 {
@@ -142,30 +145,143 @@ void virNetTLSConfigSystemIdentity(bool isServer,
                             key);
 }
 
-void virNetTLSConfigCustomCreds(const char *pkipath,
-                                bool isServer,
-                                char **cacert,
-                                char **cacrl,
-                                char **cert,
-                                char **key)
+
+int virNetTLSConfigCheckTrust(const char *cacert, const char *cacrl,
+                              bool *cacertExists, bool *cacrlExists,
+                              bool allowMissingCA)
+{
+    if (cacertExists)
+        *cacertExists = true;
+    if (cacrlExists)
+        *cacrlExists = true;
+    VIR_DEBUG("Checking CA certificate '%s' and CRL '%s'", cacert, NULLSTR(cacrl));
+    if (!virFileExists(cacert)) {
+        if (allowMissingCA) {
+            VIR_DEBUG("CA certificate '%s' does not exist", cacert);
+            if (cacertExists)
+                *cacertExists = false;
+        } else {
+            virReportSystemError(errno, _("CA certificate '%1$s' does not exist"),
+                             cacert);
+            return -1;
+        }
+    }
+    if (cacrl != NULL && !virFileExists(cacrl)) {
+        VIR_DEBUG("CA CRL '%s' does not exist", cacrl);
+        if (cacrlExists)
+            *cacrlExists = false;
+    }
+    return 0;
+}
+
+static int virNetTLSConfigEnsureTrust(char **cacert, char **cacrl,
+                                      bool allowMissingCA)
+{
+    bool cacertExists, cacrlExists;
+
+    if (virNetTLSConfigCheckTrust(*cacert, *cacrl,
+                                  &cacertExists, &cacrlExists,
+                                  allowMissingCA) < 0)
+        return -1;
+
+    if (!cacertExists)
+        VIR_FREE(*cacert);
+    if (!cacrlExists)
+        VIR_FREE(*cacrl);
+
+    return 0;
+}
+
+int virNetTLSConfigCheckIdentity(const char *cert, const char *key,
+                                 bool *identityExists, bool allowMissing)
+{
+    if (identityExists)
+        *identityExists = true;
+    VIR_DEBUG("Checking certificate '%s' and key '%s'", cert, key);
+    if (!virFileExists(cert)) {
+        int saved_errno = errno;
+        if (allowMissing) {
+            if (virFileExists(key)) {
+                virReportSystemError(
+                    saved_errno,
+                    _("Certificate '%1$s' does not exist, but key '%2$s' does"),
+                    cert, key);
+                return -1;
+            }
+            if (identityExists)
+                *identityExists = false;
+            VIR_DEBUG("Missing cert '%s' / key '%s'", cert, key);
+            return 0;
+        } else {
+            virReportSystemError(saved_errno, _("Certificate '%1$s' does not exist"),
+                                 cert);
+            return -1;
+        }
+    } else {
+        if (!virFileExists(key)) {
+            virReportSystemError(errno,
+                                 _("Key '%1$s' does not exist, but certificate '%2$s' does"),
+                                 key, cert);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+
+static int virNetTLSConfigEnsureIdentity(char **cert, char **key,
+                                         bool allowMissing)
+{
+    bool identityExists;
+
+    if (virNetTLSConfigCheckIdentity(*cert, *key, &identityExists,
+                                     allowMissing) < 0)
+      return -1;
+
+    if (!identityExists) {
+        VIR_FREE(*cert);
+        VIR_FREE(*key);
+    }
+
+    return 0;
+}
+
+
+int virNetTLSConfigCustomCreds(const char *pkipath,
+                               bool isServer,
+                               char **cacert,
+                               char **cacrl,
+                               char **cert,
+                               char **key)
 {
     VIR_DEBUG("Locating creds in custom dir %s", pkipath);
     virNetTLSConfigTrust(pkipath,
                          pkipath,
                          cacert,
                          cacrl);
+
+    if (virNetTLSConfigEnsureTrust(cacert, cacrl, false) < 0)
+        return -1;
+
     virNetTLSConfigIdentity(isServer,
                             pkipath,
                             pkipath,
                             cert,
                             key);
+
+
+    if (virNetTLSConfigEnsureIdentity(cert, key, !isServer) < 0)
+        return -1;
+
+    return 0;
 }
 
-void virNetTLSConfigUserCreds(bool isServer,
-                              char **cacert,
-                              char **cacrl,
-                              char **cert,
-                              char **key)
+int virNetTLSConfigUserCreds(bool isServer,
+                             char **cacert,
+                             char **cacrl,
+                             char **cert,
+                             char **key)
 {
     g_autofree char *pkipath = virNetTLSConfigUserPKIBaseDir();
 
@@ -175,18 +291,27 @@ void virNetTLSConfigUserCreds(bool isServer,
                          pkipath,
                          cacert,
                          cacrl);
+
+    if (virNetTLSConfigEnsureTrust(cacert, cacrl, true) < 0)
+        return -1;
+
     virNetTLSConfigIdentity(isServer,
                             pkipath,
                             pkipath,
                             cert,
                             key);
+
+    if (virNetTLSConfigEnsureIdentity(cert, key, true) < 0)
+        return -1;
+
+    return 0;
 }
 
-void virNetTLSConfigSystemCreds(bool isServer,
-                                char **cacert,
-                                char **cacrl,
-                                char **cert,
-                                char **key)
+int virNetTLSConfigSystemCreds(bool isServer,
+                               char **cacert,
+                               char **cacrl,
+                               char **cert,
+                               char **key)
 {
     VIR_DEBUG("Locating creds in system dir %s", LIBVIRT_PKI_DIR);
 
@@ -194,9 +319,18 @@ void virNetTLSConfigSystemCreds(bool isServer,
                          LIBVIRT_CACRL_DIR,
                          cacert,
                          cacrl);
+
+    if (virNetTLSConfigEnsureTrust(cacert, cacrl, false) < 0)
+        return -1;
+
     virNetTLSConfigIdentity(isServer,
                             LIBVIRT_CERT_DIR,
                             LIBVIRT_KEY_DIR,
                             cert,
                             key);
+
+    if (virNetTLSConfigEnsureIdentity(cert, key, !isServer) < 0)
+        return -1;
+
+    return 0;
 }

diff --git a/src/rpc/virnettlsconfig.h b/src/rpc/virnettlsconfig.h
index a9378c18b7df569d4c24602886ede8a6550481c4..9ad213fe06ea805939d141cdad753bdeca226cac 100644 (file)
--- a/src/rpc/virnettlsconfig.h
+++ b/src/rpc/virnettlsconfig.h
@@ -50,20 +50,25 @@ void virNetTLSConfigSystemIdentity(bool isServer,
                                    char **cert,
                                    char **key);
 
+int virNetTLSConfigCheckIdentity(const char *cert, const char *key,
+                                 bool *identityExists, bool allowMissing);
+int virNetTLSConfigCheckTrust(const char *cacert, const char *cacrl,
+                              bool *cacertExists, bool *cacrlExists,
+                              bool allowMissingCA);
 
-void virNetTLSConfigCustomCreds(const char *pkipath,
-                                bool isServer,
-                                char **cacert,
-                                char **cacrl,
-                                char **cert,
-                                char **key);
-void virNetTLSConfigUserCreds(bool isServer,
-                              char **cacert,
-                              char **cacrl,
-                              char **cert,
-                              char **key);
-void virNetTLSConfigSystemCreds(bool isServer,
-                                char **cacert,
-                                char **cacrl,
-                                char **cert,
-                                char **key);
+int virNetTLSConfigCustomCreds(const char *pkipath,
+                               bool isServer,
+                               char **cacert,
+                               char **cacrl,
+                               char **cert,
+                               char **key);
+int virNetTLSConfigUserCreds(bool isServer,
+                             char **cacert,
+                             char **cacrl,
+                             char **cert,
+                             char **key);
+int virNetTLSConfigSystemCreds(bool isServer,
+                               char **cacert,
+                               char **cacrl,
+                               char **cert,
+                               char **key);

diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 37f635f47fa02fea7d6e12c02998ac34571abadb..7061eb59531317b80d78399988592b1c48300b08 100644 (file)
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -31,7 +31,6 @@
 #include "virnettlscert.h"
 #include "virstring.h"
 
-#include "viralloc.h"
 #include "virerror.h"
 #include "virfile.h"
 #include "virutil.h"
@@ -88,22 +87,6 @@ static int virNetTLSContextOnceInit(void)
 VIR_ONCE_GLOBAL_INIT(virNetTLSContext);
 
 
-static int
-virNetTLSContextCheckCertFile(const char *type, const char *file, bool allowMissing)
-{
-    if (!virFileExists(file)) {
-        if (allowMissing)
-            return 1;
-
-        virReportSystemError(errno,
-                             _("Cannot read %1$s '%2$s'"),
-                             type, file);
-        return -1;
-    }
-    return 0;
-}
-
-
 static void virNetTLSLog(int level G_GNUC_UNUSED,
                          const char *str G_GNUC_UNUSED)
 {
@@ -112,7 +95,6 @@ static void virNetTLSLog(int level G_GNUC_UNUSED,
 
 
 static int virNetTLSContextLoadCredentials(virNetTLSContext *ctxt,
-                                           bool isServer,
                                            const char *cacert,
                                            const char *cacrl,
                                            const char *const *certs,
@@ -121,66 +103,42 @@ static int virNetTLSContextLoadCredentials(virNetTLSContext *ctxt,
     int err;
     size_t i;
 
-    if (cacert && cacert[0] != '\0') {
-        if (virNetTLSContextCheckCertFile("CA certificate", cacert, false) < 0)
-            return -1;
+    VIR_DEBUG("loading CA cert from %s", cacert);
+    err = gnutls_certificate_set_x509_trust_file(ctxt->x509cred,
+                                                 cacert,
+                                                 GNUTLS_X509_FMT_PEM);
+    if (err < 0) {
+        virReportError(VIR_ERR_SYSTEM_ERROR,
+                       _("Unable to set x509 CA certificate: %1$s: %2$s"),
+                       cacert, gnutls_strerror(err));
+        return -1;
+    }
 
-        VIR_DEBUG("loading CA cert from %s", cacert);
-        err = gnutls_certificate_set_x509_trust_file(ctxt->x509cred,
-                                                     cacert,
-                                                     GNUTLS_X509_FMT_PEM);
+    if (cacrl) {
+        VIR_DEBUG("loading CRL from %s", cacrl);
+        err = gnutls_certificate_set_x509_crl_file(ctxt->x509cred,
+                                                   cacrl,
+                                                   GNUTLS_X509_FMT_PEM);
         if (err < 0) {
             virReportError(VIR_ERR_SYSTEM_ERROR,
-                           _("Unable to set x509 CA certificate: %1$s: %2$s"),
-                           cacert, gnutls_strerror(err));
-            return -1;
-        }
-    }
-
-    if (cacrl && cacrl[0] != '\0') {
-        int rv;
-        if ((rv = virNetTLSContextCheckCertFile("CA revocation list", cacrl, true)) < 0)
+                           _("Unable to set x509 certificate revocation list: %1$s: %2$s"),
+                           cacrl, gnutls_strerror(err));
             return -1;
-
-        if (rv == 0) {
-            VIR_DEBUG("loading CRL from %s", cacrl);
-            err = gnutls_certificate_set_x509_crl_file(ctxt->x509cred,
-                                                       cacrl,
-                                                       GNUTLS_X509_FMT_PEM);
-            if (err < 0) {
-                virReportError(VIR_ERR_SYSTEM_ERROR,
-                               _("Unable to set x509 certificate revocation list: %1$s: %2$s"),
-                               cacrl, gnutls_strerror(err));
-                return -1;
-            }
-        } else {
-            VIR_DEBUG("Skipping non-existent CA CRL %s", cacrl);
         }
+    } else {
+        VIR_DEBUG("no CRL file to load");
     }
 
     for (i = 0; certs[i] != NULL && keys[i] != NULL; i++) {
-        int rv;
-        if ((rv = virNetTLSContextCheckCertFile("certificate", certs[i], !isServer)) < 0)
-            return -1;
-        if (rv == 0 &&
-            (rv = virNetTLSContextCheckCertFile("private key", keys[i], !isServer)) < 0)
+        VIR_DEBUG("loading cert and key from %s and %s", certs[i], keys[i]);
+        err = gnutls_certificate_set_x509_key_file(ctxt->x509cred,
+                                                   certs[i], keys[i],
+                                                   GNUTLS_X509_FMT_PEM);
+        if (err < 0) {
+            virReportError(VIR_ERR_SYSTEM_ERROR,
+                           _("Unable to set x509 key and certificate: %1$s, %2$s: %3$s"),
+                           keys[i], certs[i], gnutls_strerror(err));
             return -1;
-
-        if (rv == 0) {
-            VIR_DEBUG("loading cert and key from %s and %s", certs[i], keys[i]);
-            err =
-                gnutls_certificate_set_x509_key_file(ctxt->x509cred,
-                                                     certs[i], keys[i],
-                                                     GNUTLS_X509_FMT_PEM);
-            if (err < 0) {
-                virReportError(VIR_ERR_SYSTEM_ERROR,
-                               _("Unable to set x509 key and certificate: %1$s, %2$s: %3$s"),
-                               keys[i], certs[i], gnutls_strerror(err));
-                return -1;
-            }
-        } else {
-            VIR_DEBUG("Skipping non-existent cert %s key %s on client",
-                      certs[i], keys[i]);
         }
     }
 
@@ -200,6 +158,15 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
 {
     virNetTLSContext *ctxt;
     int err;
+    g_autofree char *certlist = certs ? g_strjoinv(", ", (char **)certs) : NULL;
+    g_autofree char *keylist = keys ? g_strjoinv(", ", (char **)keys) : NULL;
+    g_autofree char *acllist = x509dnACL ? g_strjoinv(", ", (char **)x509dnACL) : NULL;
+
+    VIR_DEBUG("CA cert=%s CRL=%s certs='%s' keys='%s' ACL='%s' "
+              "priority=%s sanity-check=%d require-valid=%d is-server=%d",
+              cacert, NULLSTR(cacrl), NULLSTR(certlist), NULLSTR(keylist),
+              NULLSTR(acllist), priority, sanityCheckCert, requireValidCert,
+              isServer);
 
     if (virNetTLSContextInitialize() < 0)
         return NULL;
@@ -228,7 +195,7 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
         virNetTLSCertSanityCheck(isServer, cacert, certs) < 0)
         goto error;
 
-    if (virNetTLSContextLoadCredentials(ctxt, isServer, cacert, cacrl,
+    if (virNetTLSContextLoadCredentials(ctxt, cacert, cacrl,
                                         certs, keys) < 0)
         goto error;
 
@@ -268,38 +235,22 @@ static int virNetTLSContextLocateCredentials(const char *pkipath,
      * files actually exist there
      */
     if (pkipath) {
-        virNetTLSConfigCustomCreds(pkipath, isServer,
-                                   cacert, cacrl,
-                                   cert, key);
+        if (virNetTLSConfigCustomCreds(pkipath, isServer,
+                                       cacert, cacrl,
+                                       cert, key) < 0)
+            return -1;
     } else {
-        if (tryUserPkiPath) {
+        if (tryUserPkiPath &&
             virNetTLSConfigUserCreds(isServer,
                                      cacert, cacrl,
-                                     cert, key);
-
-            /*
-             * If some of the files can't be found, fallback
-             * to the global location for them
-             */
-            if (!virFileExists(*cacert))
-                VIR_FREE(*cacert);
-            if (!virFileExists(*cacrl))
-                VIR_FREE(*cacrl);
-
-            /* Check these as a pair, since it they are
-             * mutually dependent
-             */
-            if (!virFileExists(*key) || !virFileExists(*cert)) {
-                VIR_FREE(*key);
-                VIR_FREE(*cert);
-            }
-        }
+                                     cert, key) < 0)
+            return -1;
 
-        virNetTLSConfigSystemCreds(isServer,
-                                   cacert, cacrl,
-                                   cert, key);
+        if (virNetTLSConfigSystemCreds(isServer,
+                                       cacert, cacrl,
+                                       cert, key) < 0)
+            return -1;
     }
-
     return 0;
 }
 
@@ -359,6 +310,13 @@ virNetTLSContext *virNetTLSContextNewServer(const char *cacert,
                                             bool sanityCheckCert,
                                             bool requireValidCert)
 {
+    size_t i;
+    if (virNetTLSConfigCheckTrust(cacert, cacrl, NULL, NULL, false) < 0)
+        return NULL;
+    for (i = 0; certs[i] != NULL && keys[i] != NULL; i++) {
+        if (virNetTLSConfigCheckIdentity(certs[i], keys[i], NULL, false) < 0)
+            return NULL;
+    }
     return virNetTLSContextNew(cacert, cacrl, certs, keys, x509dnACL, priority,
                                sanityCheckCert, requireValidCert, true);
 }
@@ -393,7 +351,7 @@ int virNetTLSContextReloadForServer(virNetTLSContext *ctxt,
     if (virNetTLSCertSanityCheck(true, cacert, certs))
         goto error;
 
-    if (virNetTLSContextLoadCredentials(ctxt, true, cacert, cacrl,
+    if (virNetTLSContextLoadCredentials(ctxt, cacert, cacrl,
                                         certs, keys))
         goto error;
 
@@ -417,6 +375,13 @@ virNetTLSContext *virNetTLSContextNewClient(const char *cacert,
                                             bool sanityCheckCert,
                                             bool requireValidCert)
 {
+    size_t i;
+    if (virNetTLSConfigCheckTrust(cacert, cacrl, NULL, NULL, false) < 0)
+        return NULL;
+    for (i = 0; certs[i] != NULL && keys[i] != NULL; i++) {
+        if (virNetTLSConfigCheckIdentity(certs[i], keys[i], NULL, false) < 0)
+            return NULL;
+    }
     return virNetTLSContextNew(cacert, cacrl, certs, keys, NULL, priority,
                                sanityCheckCert, requireValidCert, false);
 }