Re: [PATCH 1/2] tls: Drop support for tls_allowed_dn_list
On Wed, Nov 10, 2021 at 11:02:14AM +0100, Martin Kletzander wrote:
On Tue, Nov 09, 2021 at 07:02:54PM +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.
>
> gnutls_x509_crt_get_dn() hasn't changed behaviour recently.
>
> Back in 2016 it was accidentally changed, but they quickly
> fixed that mistake, and introduced a newer
> gnutls_x509_crt_get_dn3() which is what certtool -i uses
> and reports in the opposite order.
>
> What changed recently is the order in which fields are
> written into the created certificate by certtool.
>
> This is fine, libvirt's tls_allowed_dn_list setting
> has to be configured to match the order of the fields
> in the certificates you're actually using.
>
> The primary bug here from libvirt's POV, is that we're
> incorrectly telling people to use the order reported by
> "certtool -i". We need to tell them to use the *reverse*
> of what certtool -i reports.
>
> Or we could possibly add a 'tls_allowed_dn_list_revere = bool'
> setting to make ordering configurable.
>
One other option, if the output of gnutls_x509_crt_get_dn() is
guaranteed, would be to introduce a tiny helper binary that takes a
certificate on input and outputs the DN in the format libvirt will be
checking it.
Yes, that's pretty trivial - could do a 'virt-pki-query-dn' tool
In fact I wrote code for that when testing the bug behaviour
yesterday
#include <gnutls/gnutls.h>
#include <gnutls/crypto.h>
#include <gnutls/x509.h>
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <assert.h>
#include <stdlib.h>
int main (int argc, char **argv)
{
char dname[256];
char *dnameptr = dname;
size_t dnamesize = sizeof(dname);
gnutls_datum_t data;
gnutls_x509_crt_t cert = NULL;
int ret = -1;
char buf[8192];
int fd;
if (gnutls_x509_crt_init(&cert) < 0) {
abort();
}
fd = open(argv[1], O_RDONLY);
assert(fd >= 0);
read(fd, buf, sizeof(buf));
data.data = (unsigned char *)buf;
data.size = strlen(buf);
if (gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM) < 0) {
abort();
}
ret = gnutls_x509_crt_get_dn(cert, dname, &dnamesize);
fprintf(stderr, "%s\n", dname);
}
lack error checking, should use glib (eg for g_file_get_contents),
etc, but would nicely isolate us from any behaviour changes in
tools.
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 :|