This setting was unsafe for a number of reasons, so bear with me here. In the Distinguished Name (or rather the string representation of ASN.1 RelativeDistinguishedName if you will) the individial fields (or rather each AttributeTypeAndValue) can be in any order as defined in RFC4514, section 2.2. The function we use to get the DN is gnutls_x509_crt_get_dn(3) which claims to return the DN as described in the aforementioned RFC. The help we are providing to the user when no DN from the list of allowed DNs matches an incoming TLS connection says to check the output of certtool, particularly `certtool -i --infile clientcert.pem`. However in the output of that command the order of the fields changed in some previous version exposing the inconsistency (see bugzilla link below for more details). This is one reason why we should not depend on the order of the fields being stable as the same change can happen (and maybe already happened) with the gnutls_x509_crt_get_dn(3) function. Another issue is that we are matching the patterns with a simple glob match, particularly g_pattern_match_simple(3) from glib. Due to the fact that any asterisk in that pattern might match not only the field, but anything in the whole DN, it is very prone to errors, sometimes even not caused by the administrator or application setting up the allowed list. This functionality is therefore considered unsafe and should not be used, hence this commit makes the daemon fail during startup with a descriptive explanation which is the safest option that does not allow unwanted behaviour and makes the error message immediately apparent. Possible solutions were considered, such as ordering of the fields and implementing better matching configuration options and algorithm. However these could lead to unsafe behaviour if not implemented exactly based on the RFC and even with that taken into the consideration it is not really an efficient way of defining filters when done with the configuration in conf (ini) format. In case of using low level functions like gnutls_x509_crt_get_subject(3) and gnutls_x509_dn_get_rdn_ava(3) this would add huge amount of complexity to offer proper filtering and string representations (including encoding etc.). https://bugzilla.redhat.com/show_bug.cgi?id=2018488 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- I am very happy to discuss this in more detail. I am also working on a better way to provide ACLs for remote connections and I would be OK with postponing this patch until that is merged so that there is a supported way of limiting remote users if there are any current users of this functionality. --- docs/remote.html.in | 26 -------------- docs/tlscerts.html.in | 6 ---- src/remote/libvirtd.aug.in | 1 - src/remote/libvirtd.conf.in | 16 --------- src/remote/remote_daemon.c | 2 -- src/remote/remote_daemon_config.c | 19 +++++----- src/remote/remote_daemon_config.h | 1 - src/remote/test_libvirtd.aug.in | 4 --- src/rpc/virnettlscontext.c | 60 ++++++------------------------- src/rpc/virnettlscontext.h | 2 -- tests/virconfdata/libvirtd.conf | 17 --------- tests/virconfdata/libvirtd.out | 14 -------- tests/virnettlscontexttest.c | 1 - tests/virnettlssessiontest.c | 1 - 14 files changed, 21 insertions(+), 149 deletions(-) diff --git a/docs/remote.html.in b/docs/remote.html.in index cc8db80c959c..211557aad66b 100644 --- a/docs/remote.html.in +++ b/docs/remote.html.in @@ -236,32 +236,6 @@ Blank lines and comments beginning with <code>#</code> are ignored. If you set this to an empty string, then no CRL is loaded. </td> </tr> - <tr> - <td> tls_allowed_dn_list ["DN1", "DN2"] </td> - <td> (none - DNs are not checked) </td> - <td> - <p> - Enable an access control list of client certificate Distinguished - Names (DNs) which can connect to the TLS port on this server. - </p> - <p> - The default is that DNs are not checked. - </p> - <p> - This list may contain wildcards such as <code>"C=GB,ST=London,L=London,O=Libvirt Project,CN=*"</code> - See the POSIX <code>fnmatch</code> function for the format - of the wildcards. - </p> - <p> - Note that if this is an empty list, <i>no client can connect</i>. - </p> - <p> - Note also that GnuTLS returns DNs without spaces - after commas between the fields (and this is what we check against), - but the <code>openssl x509</code> tool shows spaces. -</p> - </td> - </tr> </table> <h2> <a id="Remote_IPv6">IPv6 support</a> diff --git a/docs/tlscerts.html.in b/docs/tlscerts.html.in index 5b7a5f56e4c2..c5206172f806 100644 --- a/docs/tlscerts.html.in +++ b/docs/tlscerts.html.in @@ -71,9 +71,6 @@ next section. <td> Installed on the client </td> <td> Client's certificate signed by the CA (<a href="#Remote_TLS_client_certificates">more info</a>) </td> - <td> Distinguished Name (DN) can be checked against an access - control list (<code>tls_allowed_dn_list</code>). - </td> </tr> <tr> <td> @@ -90,9 +87,6 @@ next section. <td> Installed on the client </td> <td> Client's certificate signed by the CA (<a href="#Remote_TLS_client_certificates">more info</a>) </td> - <td> Distinguished Name (DN) can be checked against an access - control list (<code>tls_allowed_dn_list</code>). - </td> </tr> </table> <p> diff --git a/src/remote/libvirtd.aug.in b/src/remote/libvirtd.aug.in index d744548f4126..5bfc0a501aa5 100644 --- a/src/remote/libvirtd.aug.in +++ b/src/remote/libvirtd.aug.in @@ -52,7 +52,6 @@ module @DAEMON_NAME_UC@ = let tls_authorization_entry = bool_entry "tls_no_verify_certificate" | bool_entry "tls_no_sanity_certificate" - | str_array_entry "tls_allowed_dn_list" | str_entry "tls_priority" @END@ diff --git a/src/remote/libvirtd.conf.in b/src/remote/libvirtd.conf.in index 8e709856aacb..5e4a8c34915f 100644 --- a/src/remote/libvirtd.conf.in +++ b/src/remote/libvirtd.conf.in @@ -285,22 +285,6 @@ #tls_no_verify_certificate = 1 -# An access control list of allowed x509 Distinguished Names -# This list may contain wildcards such as -# -# "C=GB,ST=London,L=London,O=Red Hat,CN=*" -# -# See the g_pattern_match function for the format of the wildcards: -# -# https://developer.gnome.org/glib/stable/glib-Glob-style-pattern-matching.htm... -# -# NB If this is an empty list, no client can connect, so comment out -# entirely rather than using empty list to disable these checks -# -# By default, no DN's are checked -#tls_allowed_dn_list = ["DN1", "DN2"] - - # Override the compile time default TLS priority string. The # default is usually "NORMAL" unless overridden at build time. # Only set this is it is desired for libvirt to deviate from diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index de43a54c2e75..448953f48a50 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -366,7 +366,6 @@ daemonSetupNetworking(virNetServer *srv, config->crl_file, config->cert_file, config->key_file, - (const char *const*)config->tls_allowed_dn_list, config->tls_priority, config->tls_no_sanity_certificate ? false : true, config->tls_no_verify_certificate ? false : true))) @@ -374,7 +373,6 @@ daemonSetupNetworking(virNetServer *srv, } else { if (!(ctxt = virNetTLSContextNewServerPath(NULL, !privileged, - (const char *const*)config->tls_allowed_dn_list, config->tls_priority, config->tls_no_sanity_certificate ? false : true, config->tls_no_verify_certificate ? false : true))) diff --git a/src/remote/remote_daemon_config.c b/src/remote/remote_daemon_config.c index 30653e82cff6..7ee2a0a77a61 100644 --- a/src/remote/remote_daemon_config.c +++ b/src/remote/remote_daemon_config.c @@ -203,13 +203,6 @@ daemonConfigFree(struct daemonConfig *data) g_free(data->sasl_allowed_username_list); #ifdef WITH_IP - tmp = data->tls_allowed_dn_list; - while (tmp && *tmp) { - g_free(*tmp); - tmp++; - } - g_free(data->tls_allowed_dn_list); - g_free(data->tls_priority); g_free(data->key_file); @@ -298,9 +291,17 @@ daemonConfigLoadOptions(struct daemonConfig *data, if (virConfGetValueString(conf, "crl_file", &data->crl_file) < 0) return -1; - if (virConfGetValueStringList(conf, "tls_allowed_dn_list", false, - &data->tls_allowed_dn_list) < 0) + if (virConfGetValue(conf, "tls_allowed_dn_list")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The tls_allowed_dn_list configuration setting has " + "been deprecated as unsafe and is not supported any " + "more. In order to assure complete safety all " + "certificates are forbidden from connecting until " + "this option is removed. Make sure your use case is " + "properly configured without this configuration knob " + "present so it can be safely removed.")); return -1; + } if (virConfGetValueString(conf, "tls_priority", &data->tls_priority) < 0) return -1; diff --git a/src/remote/remote_daemon_config.h b/src/remote/remote_daemon_config.h index 47839271d315..99d79602651c 100644 --- a/src/remote/remote_daemon_config.h +++ b/src/remote/remote_daemon_config.h @@ -54,7 +54,6 @@ struct daemonConfig { #ifdef WITH_IP bool tls_no_verify_certificate; bool tls_no_sanity_certificate; - char **tls_allowed_dn_list; char *tls_priority; unsigned int tcp_min_ssf; diff --git a/src/remote/test_libvirtd.aug.in b/src/remote/test_libvirtd.aug.in index c27680e1306e..07aff706e07f 100644 --- a/src/remote/test_libvirtd.aug.in +++ b/src/remote/test_libvirtd.aug.in @@ -31,10 +31,6 @@ module Test_@DAEMON_NAME@ = { "crl_file" = "@sysconfdir@/pki/CA/crl.pem" } { "tls_no_sanity_certificate" = "1" } { "tls_no_verify_certificate" = "1" } - { "tls_allowed_dn_list" - { "1" = "DN1"} - { "2" = "DN2"} - } { "tls_priority" = "NORMAL" } @END@ { "sasl_allowed_username_list" diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 1340faa22485..d1294a661a6b 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -60,7 +60,6 @@ struct _virNetTLSContext { bool isServer; bool requireValidCert; - const char *const *x509dnACL; char *priority; }; @@ -352,43 +351,12 @@ static int virNetTLSContextCheckCertKeyPurpose(gnutls_x509_crt_t cert, return 0; } -/* Check DN is on tls_allowed_dn_list. */ -static int -virNetTLSContextCheckCertDNACL(const char *dname, - const char *const *wildcards) -{ - while (*wildcards) { - if (g_pattern_match_simple(*wildcards, dname)) - return 1; - - wildcards++; - } - - /* Log the client's DN for debugging */ - VIR_DEBUG("Failed ACL check for client DN '%s'", dname); - - /* This is the most common error: make it informative. */ - virReportError(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 -virNetTLSContextCheckCertDN(gnutls_x509_crt_t cert, - const char *certFile, - const char *hostname, - const char *dname, - const char *const *acl) +virNetTLSContextCheckCertHostname(gnutls_x509_crt_t cert, + const char *certFile, + const char *hostname) { - if (acl && dname && - virNetTLSContextCheckCertDNACL(dname, acl) <= 0) - return -1; - if (hostname && !gnutls_x509_crt_check_hostname(cert, hostname)) { virReportError(VIR_ERR_RPC, @@ -673,7 +641,6 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert, const char *cacrl, const char *cert, const char *key, - const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert, @@ -738,7 +705,6 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert, } ctxt->requireValidCert = requireValidCert; - ctxt->x509dnACL = x509dnACL; ctxt->isServer = isServer; PROBE(RPC_TLS_CONTEXT_NEW, @@ -853,7 +819,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, static virNetTLSContext *virNetTLSContextNewPath(const char *pkipath, bool tryUserPkiPath, - const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert, @@ -866,9 +831,8 @@ static virNetTLSContext *virNetTLSContextNewPath(const char *pkipath, &cacert, &cacrl, &cert, &key) < 0) return NULL; - ctxt = virNetTLSContextNew(cacert, cacrl, cert, key, - x509dnACL, priority, sanityCheckCert, - requireValidCert, isServer); + ctxt = virNetTLSContextNew(cacert, cacrl, cert, key, priority, + sanityCheckCert, requireValidCert, isServer); VIR_FREE(cacert); VIR_FREE(cacrl); @@ -880,12 +844,11 @@ static virNetTLSContext *virNetTLSContextNewPath(const char *pkipath, virNetTLSContext *virNetTLSContextNewServerPath(const char *pkipath, bool tryUserPkiPath, - const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert) { - return virNetTLSContextNewPath(pkipath, tryUserPkiPath, x509dnACL, priority, + return virNetTLSContextNewPath(pkipath, tryUserPkiPath, priority, sanityCheckCert, requireValidCert, true); } @@ -895,7 +858,7 @@ virNetTLSContext *virNetTLSContextNewClientPath(const char *pkipath, bool sanityCheckCert, bool requireValidCert) { - return virNetTLSContextNewPath(pkipath, tryUserPkiPath, NULL, priority, + return virNetTLSContextNewPath(pkipath, tryUserPkiPath, priority, sanityCheckCert, requireValidCert, false); } @@ -904,12 +867,11 @@ virNetTLSContext *virNetTLSContextNewServer(const char *cacert, const char *cacrl, const char *cert, const char *key, - const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert) { - return virNetTLSContextNew(cacert, cacrl, cert, key, x509dnACL, priority, + return virNetTLSContextNew(cacert, cacrl, cert, key, priority, sanityCheckCert, requireValidCert, true); } @@ -967,7 +929,7 @@ virNetTLSContext *virNetTLSContextNewClient(const char *cacert, bool sanityCheckCert, bool requireValidCert) { - return virNetTLSContextNew(cacert, cacrl, cert, key, NULL, priority, + return virNetTLSContextNew(cacert, cacrl, cert, key, priority, sanityCheckCert, requireValidCert, false); } @@ -1059,8 +1021,8 @@ static int virNetTLSContextValidCertificate(virNetTLSContext *ctxt, sess->x509dname = g_strdup(dname); VIR_DEBUG("Peer DN is %s", dname); - if (virNetTLSContextCheckCertDN(cert, "[session]", sess->hostname, dname, - ctxt->x509dnACL) < 0) { + if (virNetTLSContextCheckCertHostname(cert, "[session]", + sess->hostname) < 0) { gnutls_x509_crt_deinit(cert); goto authdeny; } diff --git a/src/rpc/virnettlscontext.h b/src/rpc/virnettlscontext.h index 11c954ce4bcf..4a3677cb28b7 100644 --- a/src/rpc/virnettlscontext.h +++ b/src/rpc/virnettlscontext.h @@ -32,7 +32,6 @@ void virNetTLSInit(void); virNetTLSContext *virNetTLSContextNewServerPath(const char *pkipath, bool tryUserPkiPath, - const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert); @@ -47,7 +46,6 @@ virNetTLSContext *virNetTLSContextNewServer(const char *cacert, const char *cacrl, const char *cert, const char *key, - const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert); diff --git a/tests/virconfdata/libvirtd.conf b/tests/virconfdata/libvirtd.conf index 6d1fd33dcdd3..5bae913b21c3 100644 --- a/tests/virconfdata/libvirtd.conf +++ b/tests/virconfdata/libvirtd.conf @@ -177,23 +177,6 @@ crl_file = "/etc/pki/CA/crl.pem" # verification. tls_no_verify_certificate = 1 - -# An access control list of allowed x509 Distinguished Names -# This list may contain wildcards such as -# -# "C=GB,ST=London,L=London,O=Red Hat,CN=*" -# -# See the g_pattern_match function for the format of the wildcards. -# -# https://developer.gnome.org/glib/stable/glib-Glob-style-pattern-matching.htm... -# -# NB If this is an empty list, no client can connect, so comment out -# entirely rather than using empty list to disable these checks -# -# By default, no DN's are checked -tls_allowed_dn_list = ["DN1", "DN2"] - - # An access control list of allowed SASL usernames. The format for usernames # depends on the SASL authentication mechanism. Kerberos usernames # look like username@REALM diff --git a/tests/virconfdata/libvirtd.out b/tests/virconfdata/libvirtd.out index ce50480b8c69..f61aae4bdfd7 100644 --- a/tests/virconfdata/libvirtd.out +++ b/tests/virconfdata/libvirtd.out @@ -142,20 +142,6 @@ crl_file = "/etc/pki/CA/crl.pem" # Default is to always verify. Uncommenting this will disable # verification. tls_no_verify_certificate = 1 -# An access control list of allowed x509 Distinguished Names -# This list may contain wildcards such as -# -# "C=GB,ST=London,L=London,O=Red Hat,CN=*" -# -# See the g_pattern_match function for the format of the wildcards. -# -# https://developer.gnome.org/glib/stable/glib-Glob-style-pattern-matching.htm... -# -# NB If this is an empty list, no client can connect, so comment out -# entirely rather than using empty list to disable these checks -# -# By default, no DN's are checked -tls_allowed_dn_list = [ "DN1", "DN2" ] # An access control list of allowed SASL usernames. The format for usernames # depends on the SASL authentication mechanism. Kerberos usernames # look like username@REALM diff --git a/tests/virnettlscontexttest.c b/tests/virnettlscontexttest.c index 0ad42a77ed1a..d316dbb2b012 100644 --- a/tests/virnettlscontexttest.c +++ b/tests/virnettlscontexttest.c @@ -67,7 +67,6 @@ static int testTLSContextInit(const void *opaque) NULL, data->crt, KEYFILE, - NULL, "NORMAL", true, true); diff --git a/tests/virnettlssessiontest.c b/tests/virnettlssessiontest.c index b9249cca5654..9fc2fa8861c2 100644 --- a/tests/virnettlssessiontest.c +++ b/tests/virnettlssessiontest.c @@ -108,7 +108,6 @@ static int testTLSSessionInit(const void *opaque) NULL, data->servercrt, KEYFILE, - data->wildcards, "NORMAL", false, true); -- 2.33.1