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

Wed Nov 10 12:18:33 UTC 2021

On Wed, Nov 10, 2021 at 10:27:03AM +0000, Daniel P. Berrangé wrote:
>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.
>

I am actually working on the more fine-grained ACLs for remote
connections, so that you can have the same granularity as with
polkit.

>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 :|
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211110/a03ae87f/attachment-0001.sig>