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

[Peter Crosthwaite]

On Tue, Sep 15, 2015 at 7:36 AM, Daniel P. Berrange <address@hidden> 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 <address@hidden>
> ---
>  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:

  CC       pixman-filter.lo
  CC       pixman-ppc.lo
In file included from pixman-combine-float.c:29:0:
../config.h:183:15: error: conflicting types for 'sqrt'
 #define sqrtf sqrt
               ^
  CC       pixman-edge.lo
  CC       pixman-edge-accessors.lo
In file included from pixman-access.c:28:0:
../config.h:183:15: error: conflicting types for 'sqrt'
 #define sqrtf sqrt
               ^
In file included from pixman-combine32.c:25:0:
../config.h:183:15: error: conflicting types for 'sqrt'
 #define sqrtf sqrt
               ^
  CC       pixman-fast-path.lo
  CC       pixman-glyph.lo
In file included from pixman.c:29:0:
pixman.c: In function 'optimize_operator':
pixman-private.h:1019:22: error: typedef 'compile_time_assertion'
locally defined but not used [-Werror=unused-local-typedefs]
     do { typedef int compile_time_assertion [(x)?1:-1]; } while (0)
                      ^
pixman.c:144:5: note: in expansion of macro 'COMPILE_TIME_ASSERT'
     COMPILE_TIME_ASSERT (FAST_PATH_IS_OPAQUE == (1 << OPAQUE_SHIFT));
     ^
In file included from pixman-access.c:28:0,
                 from pixman-access-accessors.c:3:
../config.h:183:15: error: conflicting types for 'sqrt'
 #define sqrtf sqrt
               ^
cc1: all warnings being treated as errors
In file included from pixman-conical-gradient.c:28:0:
../config.h:183:15: error: conflicting types for 'sqrt'
 #define sqrtf sqrt
               ^
make[3]: *** [pixman-combine-float.lo] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [pixman.lo] Error 1
make[3]: *** [pixman-access.lo] Error 1
make[3]: *** [pixman-combine32.lo] Error 1
make[3]: *** [pixman-access-accessors.lo] Error 1
make[3]: *** [pixman-conical-gradient.lo] Error 1
make[2]: *** [all-recursive] Error 1
make[1]: *** [all] Error 2
make: *** [subdir-pixman] Error 2

I made it go away with:

--- a/configure
+++ b/configure
@@ -415,9 +415,6 @@ if test "$debug_info" = "yes"; then
     LDFLAGS="-g $LDFLAGS"
 fi

-test_cflags=""
-test_libs=""
-
 # make source path absolute
 source_path=`cd "$source_path"; pwd`

@@ -5257,8 +5254,6 @@ 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
 echo "POD2MAN=$POD2MAN" >> $config_host_mak
 echo "TRANSLATE_OPT_CFLAGS=$TRANSLATE_OPT_CFLAGS" >> $config_host_mak
 if test "$gcov" = "yes" ; then

Regards,
Peter

>  echo "POD2MAN=$POD2MAN" >> $config_host_mak
>  echo "TRANSLATE_OPT_CFLAGS=$TRANSLATE_OPT_CFLAGS" >> $config_host_mak
>  if test "$gcov" = "yes" ; then
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 6cc7b53..dc46bc4 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -26,6 +26,516 @@
>
>  #ifdef CONFIG_GNUTLS
>
> +#include <gnutls/x509.h>
> +