On a Tuesday in 2021, Martin Kletzander wrote:
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@xxxxxxxxxx> --- 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.html -#
I like your justification for removing this dead link. Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx> Jano
-# 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"] -
Attachment:
signature.asc
Description: PGP signature
