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

[Daniel P. Berrangé]

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