On Tue, Nov 09, 2021 at 05:14:59PM +0000, Daniel P. Berrangé wrote:
On Tue, Nov 09, 2021 at 05:30:33PM +0100, 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.Of course it can match anything in the whole DN - that's entirely the point of having wildcards. The DNs are controlled by the CA administrator, so it is predictable what information will be present there. The libvirt administrator can be as strict or loose as they wish to be. They don't even have to use globs if they don't want to.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.It is *not* unsafe, but the documentation and usability leaves a little to be desired. Sure you can screw up if you configure a glob that is too loose, but that doesn't mean the functionality is useless and needs to be ripped out.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.Please don't remove this functionality. It is the only way you can do fine grained access control on TLS clients, without introducing use of SASL on top.
The thing I am playing with on the side can utilise the SASL username as well as the client's DN, it will just have the same issues if we use the DN in the same format. I will at least change the error message so that it does not suggest using the subject from `certtool -i` so it is more usable for users.
--- 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(-)Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Attachment:
signature.asc
Description: PGP signature
