Re: [Qemu-devel] [PATCH PULL v3 08/11] crypto: add sanity checking of TLS x509 credentials

["Daniel P. Berrange" <berrange@redhat.com>]

On Sat, Sep 19, 2015 at 09:47:01PM -0700, Peter Crosthwaite wrote:
> On Tue, Sep 15, 2015 at 7:36 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
> > If the administrator incorrectly sets up their x509 certificates,
> > the errors seen at runtime during connection attempts are very
> > obscure and difficult to diagnose. This has been a particular
> > problem for people using openssl to generate their certificates
> > instead of the gnutls certtool, because the openssl tools don't
> > turn on the various x509 extensions that gnutls expects to be
> > present by default.
> >
> > This change thus adds support in the TLS credentials object to
> > sanity check the certificates when QEMU first loads them. This
> > gives the administrator immediate feedback for the majority of
> > common configuration mistakes, reducing the pain involved in
> > setting up TLS. The code is derived from equivalent code that
> > has been part of libvirt's TLS support and has been seen to be
> > valuable in assisting admins.
> >
> > It is possible to disable the sanity checking, however, via
> > the new 'sanity-check' property on the tls-creds object type,
> > with a value of 'no'.
> >
> > Unit tests are included in this change to verify the correctness
> > of the sanity checking code in all the key scenarios it is
> > intended to cope with. As part of the test suite, the pkix_asn1_tab.c
> > from gnutls is imported. This file is intentionally copied from the
> > (long since obsolete) gnutls 1.6.3 source tree, since that version
> > was still under GPLv2+, rather than the GPLv3+ of gnutls >= 2.0.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  configure                        |   22 +
> >  crypto/tlscredsx509.c            |  546 +++++++++++++++++++
> >  include/crypto/tlscredsx509.h    |    1 +
> >  tests/.gitignore                 |    3 +
> >  tests/Makefile                   |    5 +
> >  tests/crypto-tls-x509-helpers.c  |  485 +++++++++++++++++
> >  tests/crypto-tls-x509-helpers.h  |  133 +++++
> >  tests/pkix_asn1_tab.c            | 1104 ++++++++++++++++++++++++++++++++++++++
> >  tests/test-crypto-tlscredsx509.c |  731 +++++++++++++++++++++++++
> >  trace-events                     |    5 +
> >  10 files changed, 3035 insertions(+)
> >  create mode 100644 tests/crypto-tls-x509-helpers.c
> >  create mode 100644 tests/crypto-tls-x509-helpers.h
> >  create mode 100644 tests/pkix_asn1_tab.c
> >  create mode 100644 tests/test-crypto-tlscredsx509.c
> >
> > diff --git a/configure b/configure
> > index d7c24cd..bdd302c 100755
> > --- a/configure
> > +++ b/configure
> > @@ -416,6 +416,9 @@ if test "$debug_info" = "yes"; then
> >      LDFLAGS="-g $LDFLAGS"
> >  fi
> >
> > +test_cflags=""
> > +test_libs=""
> > +
> >  # make source path absolute
> >  source_path=`cd "$source_path"; pwd`
> >
> > @@ -2249,6 +2252,19 @@ if test "$gnutls_nettle" != "no"; then
> >      fi
> >  fi
> >
> > +##########################################
> > +# libtasn1 - only for the TLS creds/session test suite
> > +
> > +tasn1=yes
> > +if $pkg_config --exists "libtasn1"; then
> > +    tasn1_cflags=`$pkg_config --cflags libtasn1`
> > +    tasn1_libs=`$pkg_config --libs libtasn1`
> > +    test_cflags="$test_cflags $tasn1_cflags"
> > +    test_libs="$test_libs $tasn1_libs"
> > +else
> > +    tasn1=no
> > +fi
> > +
> >
> >  ##########################################
> >  # VTE probe
> > @@ -4574,6 +4590,7 @@ echo "GNUTLS support    $gnutls"
> >  echo "GNUTLS hash       $gnutls_hash"
> >  echo "GNUTLS gcrypt     $gnutls_gcrypt"
> >  echo "GNUTLS nettle     $gnutls_nettle ${gnutls_nettle+($nettle_version)}"
> > +echo "libtasn1          $tasn1"
> >  echo "VTE support       $vte"
> >  echo "curses support    $curses"
> >  echo "curl support      $curl"
> > @@ -4945,6 +4962,9 @@ if test "$gnutls_nettle" = "yes" ; then
> >    echo "CONFIG_GNUTLS_NETTLE=y" >> $config_host_mak
> >    echo "CONFIG_NETTLE_VERSION_MAJOR=${nettle_version%%.*}" >> $config_host_mak
> >  fi
> > +if test "$tasn1" = "yes" ; then
> > +  echo "CONFIG_TASN1=y" >> $config_host_mak
> > +fi
> >  if test "$vte" = "yes" ; then
> >    echo "CONFIG_VTE=y" >> $config_host_mak
> >    echo "VTE_CFLAGS=$vte_cflags" >> $config_host_mak
> > @@ -5268,6 +5288,8 @@ echo "EXESUF=$EXESUF" >> $config_host_mak
> >  echo "DSOSUF=$DSOSUF" >> $config_host_mak
> >  echo "LDFLAGS_SHARED=$LDFLAGS_SHARED" >> $config_host_mak
> >  echo "LIBS_QGA+=$libs_qga" >> $config_host_mak
> > +echo "TEST_LIBS=$test_libs" >> $config_host_mak
> > +echo "TEST_CFLAGS=$test_cflags" >> $config_host_mak
> 
> I am not too sure exactly why yet, but this breaks the build for me
> when using pixman submodule with --enable-werror configure:

[snip]

I have pixman pre-installed so didn't notice this. I'll investigate
and report back...


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|