On Tue, Jul 30, 2013 at 3:45 PM, Eric Blake <eblake@redhat.com> wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=951637

Newer gnutls uses nettle, rather than gcrypt, which is a lot nicer regarding initialization. Yet we were unconditionally initializing gcrypt even when gnutls wouldn't be using it, and having two crypto libraries linked into libvirt.so is pointless, but mostly harmless (it doesn't crash, but does interfere with certification efforts).

There are three distinct version ranges to worry about when determining which crypto lib gnutls uses, per these gnutls mails: 2.12: http://lists.gnu.org/archive/html/gnutls-devel/2011-03/msg00034.html 3.0: http://lists.gnu.org/archive/html/gnutls-devel/2011-07/msg00035.html

If pkg-config can prove version numbers and/or list the crypto library used for static linking, we have our proof; if not, it is safer (even if pointless) to continue to use gcrypt ourselves.

* configure.ac (WITH_GNUTLS): Probe whether to add -lgcrypt, and define a witness WITH_GNUTLS_GCRYPT. * src/libvirt.c (virTLSMutexInit, virTLSMutexDestroy) (virTLSMutexLock, virTLSMutexUnlock, virTLSThreadImpl) (virGlobalInit): Honor the witness. * libvirt.spec.in (BuildRequires): Make gcrypt usage conditional, no longer needed in Fedora 19.

Signed-off-by: Eric Blake <eblake@redhat.com> ---

v3: configure logic is enhanced to try harder to get the right answer for gentoo. I tested with Fedora 19 and RHEL 6, but need feedback from a gentoo tester before this can go in.

configure.ac | 37 ++++++++++++++++++++++++++++++------- libvirt.spec.in | 2 ++ src/libvirt.c | 10 ++++++---- 3 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/configure.ac b/configure.ac index 1817126..e3d148c 100644 --- a/configure.ac +++ b/configure.ac @@ -1076,12 +1076,26 @@ if test "x$with_gnutls" != "xno"; then LIBS="$LIBS $GNUTLS_LIBS"

GNUTLS_FOUND=no + GNUTLS_GCRYPT=unknown if test -x "$PKG_CONFIG" ; then + dnl Triple probe: gnutls < 2.12 only used gcrypt, gnutls >= 3.0 uses + dnl only nettle, and versions in between had a configure option. + dnl Our goal is to avoid gcrypt if we can prove gnutls uses nettle, + dnl but it is a safe fallback to use gcrypt if we can't prove anything. + if $PKG_CONFIG --exists 'gnutls >= 3.0'; then + GNUTLS_GCRYPT=no + elif $PKG_CONFIG --exists 'gnutls >= 2.12'; then + GNUTLS_GCRYPT=probe + else + GNUTLS_GCRYPT=yes + fi PKG_CHECK_MODULES(GNUTLS, gnutls >= $GNUTLS_REQUIRED, [GNUTLS_FOUND=yes], [GNUTLS_FOUND=no]) fi if test "$GNUTLS_FOUND" = "no"; then + dnl pkg-config couldn't help us, assume gcrypt is necessary fail=0 + GNUTLS_GCRYPT=yes AC_CHECK_HEADER([gnutls/gnutls.h], [], [fail=1]) AC_CHECK_LIB([gnutls], [gnutls_handshake],[], [fail=1], [-lgcrypt])

@@ -1098,13 +1112,22 @@ if test "x$with_gnutls" != "xno"; then AC_MSG_ERROR([You must install the GnuTLS library in order to compile and run libvirt]) fi else - dnl Not all versions of gnutls include -lgcrypt, and so we add - dnl it explicitly for the calls to gcry_control/check_version - GNUTLS_LIBS="$GNUTLS_LIBS -lgcrypt" - - dnl We're not using gcrypt deprecated features so define - dnl GCRYPT_NO_DEPRECATED to avoid deprecated warnings - GNUTLS_CFLAGS="$GNUTLS_CFLAGS -DGCRYPT_NO_DEPRECATED" + dnl See comments above about when to use gcrypt. + if test "$GNUTLS_GCRYPT" = probe; then + case `$PKG_CONFIG --libs --static gnutls` in + *gcrypt*) GNUTLS_GCRYPT=yes ;; + *nettle*) GNUTLS_GCRYPT=no ;; + *) GNUTLS_GCRYPT=unknown ;; + esac + fi + if test "$GNUTLS_GCRYPT" = yes || test "$GNUTLS_GCRYPT" = unknown; then + GNUTLS_LIBS="$GNUTLS_LIBS -lgcrypt" + dnl We're not using gcrypt deprecated features so define + dnl GCRYPT_NO_DEPRECATED to avoid deprecated warnings + GNUTLS_CFLAGS="$GNUTLS_CFLAGS -DGCRYPT_NO_DEPRECATED" + AC_DEFINE_UNQUOTED([WITH_GNUTLS_GCRYPT], 1, + [set to 1 if it is known or assumed that GNUTLS uses gcrypt]) + fi

dnl gnutls 3.x moved some declarations to a new header AC_CHECK_HEADERS([gnutls/crypto.h], [], [], [[ diff --git a/libvirt.spec.in b/libvirt.spec.in index fce7f91..7fd3c85 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -438,7 +438,9 @@ BuildRequires: readline-devel BuildRequires: ncurses-devel BuildRequires: gettext BuildRequires: libtasn1-devel +%if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?fedora} && 0%{?fedora} < 19) BuildRequires: libgcrypt-devel +%endif BuildRequires: gnutls-devel BuildRequires: libattr-devel %if %{with_libvirtd} diff --git a/src/libvirt.c b/src/libvirt.c index 8157488..66e8248 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -55,7 +55,9 @@ #include "intprops.h" #include "virconf.h" #if WITH_GNUTLS -# include <gcrypt.h> +# if WITH_GNUTLS_GCRYPT +# include <gcrypt.h> +# endif # include "rpc/virnettlscontext.h" #endif #include "vircommand.h" @@ -270,7 +272,7 @@ winsock_init(void) #endif

-#ifdef WITH_GNUTLS +#ifdef WITH_GNUTLS_GCRYPT static int virTLSMutexInit(void **priv) { virMutexPtr lock = NULL; @@ -323,7 +325,7 @@ static struct gcry_thread_cbs virTLSThreadImpl = { virTLSMutexUnlock, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL }; -#endif +#endif /* WITH_GNUTLS_GCRYPT */

/* Helper macros to implement VIR_DOMAIN_DEBUG using just C99. This * assumes you pass fewer than 15 arguments to VIR_DOMAIN_DEBUG, but @@ -407,7 +409,7 @@ virGlobalInit(void) virErrorInitialize() < 0) goto error;

-#ifdef WITH_GNUTLS +#ifdef WITH_GNUTLS_GCRYPT /* * This sequence of API calls it copied exactly from * gnutls 2.12.23 source lib/gcrypt/init.c, with -- 1.7.1

I can ACK this by visual code inspection. I haven't explicitly tested this patch but its a cleaned up version of the patch we previously discussed it. If you need me to test it, I can do that tonight. -- Doug Goldstein