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