Re: [PATCH 1/2] tls: Drop support for tls_allowed_dn_list

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(a)redhat.com&gt; --- 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(-)