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

On Wed, Nov 10, 2021 at 11:15:36AM +0100, Martin Kletzander wrote: > 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(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. > > > > 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. We already have support for sasl_allowed_username_list = [....] in the same way as our DN allow list. They can both be used, if you happen to run SASL over TLS too.

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 :|