Re: [libvirt] [PATCH] Make TLS support conditional
On 01/09/2013 02:48 AM, Daniel P. Berrange wrote: >> As it is, with your patch, I just got this failure on RHEL 5: >> >> /usr/bin/perl ./check-symfile.pl l ibvirt.yms \ >> .libs/libvirt.so >> Expected symbol virNetServerClientGetTLSKeySize is not in ELF library >> ... >> >> I still need to do more investigation, but it makes me wonder if we got >> the conditional symfile manipulation correct? > > Yeah, actually I think that's something I forgot to handle. We still need to fix that for when a user explicitly configures --without-gnutls. > on RHEL5, GNUTLS should be present so that symbol ought to have been > built, unless you were testing with --without-gnutls perhaps ? Figured it out: on RHEL 5, automake and autoconf are so old that they don't automatically reconfigure when you use just 'make', so config.h was out of date, and the new HAVE_GNUTLS macro wasn't defined. Forcing a fresh reconfigure fixed the issue to use gnutls in a default build. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Make TLS support conditional
On Tue, Jan 08, 2013 at 04:30:49PM -0700, Eric Blake wrote:
> On 01/08/2013 01:47 PM, Daniel P. Berrange wrote:
> > On Mon, Jan 07, 2013 at 05:37:30PM -0700, Eric Blake wrote:
> >>
> >> Touches quite a bit, but hopefully for the better. What platform are
> >> you targeting where you were unwilling to require gnutls as a prereq?
> >
> > No specific platform as such, just that if you build with
> > --without-remote and --without-libvirtd we should not be
> > mandating use of gnutls. Various people have asked for this
> > feature over the years, so I think it is worth it.
> >
> >>
> >> Overall, your patch looks sane, and you have a 'weak ACK' - that is, I'm
> >> willing to look the other way and let this patch go in, if you don't
> >> think it is worth even more refactoring to avoid quite so much leaky
> >> #ifdef throughout the code base.
> >
> > Basically I'm following the approach used for SASL. It would be nice to
> > try and adapt virnet{tls,sasl}context.c so that all the functions still
> > exist, but have no-op impls, but that's much more work - I've tried it
> > before with SASL but never got a satisfactory result
>
> As it is, with your patch, I just got this failure on RHEL 5:
>
> /usr/bin/perl ./check-symfile.pl l ibvirt.yms \
> .libs/libvirt.so
> Expected symbol virNetServerClientGetTLSKeySize is not in ELF library
> ...
>
> I still need to do more investigation, but it makes me wonder if we got
> the conditional symfile manipulation correct?
Yeah, actually I think that's something I forgot to handle. That said
on RHEL5, GNUTLS should be present so that symbol ought to have been
built, unless you were testing with --without-gnutls perhaps ?
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Make TLS support conditional
On 01/08/2013 01:47 PM, Daniel P. Berrange wrote:
> On Mon, Jan 07, 2013 at 05:37:30PM -0700, Eric Blake wrote:
>>
>> Touches quite a bit, but hopefully for the better. What platform are
>> you targeting where you were unwilling to require gnutls as a prereq?
>
> No specific platform as such, just that if you build with
> --without-remote and --without-libvirtd we should not be
> mandating use of gnutls. Various people have asked for this
> feature over the years, so I think it is worth it.
>
>>
>> Overall, your patch looks sane, and you have a 'weak ACK' - that is, I'm
>> willing to look the other way and let this patch go in, if you don't
>> think it is worth even more refactoring to avoid quite so much leaky
>> #ifdef throughout the code base.
>
> Basically I'm following the approach used for SASL. It would be nice to
> try and adapt virnet{tls,sasl}context.c so that all the functions still
> exist, but have no-op impls, but that's much more work - I've tried it
> before with SASL but never got a satisfactory result
As it is, with your patch, I just got this failure on RHEL 5:
/usr/bin/perl ./check-symfile.pl l ibvirt.yms \
.libs/libvirt.so
Expected symbol virNetServerClientGetTLSKeySize is not in ELF library
...
I still need to do more investigation, but it makes me wonder if we got
the conditional symfile manipulation correct?
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Make TLS support conditional
On Mon, Jan 07, 2013 at 05:37:30PM -0700, Eric Blake wrote:
>
> Touches quite a bit, but hopefully for the better. What platform are
> you targeting where you were unwilling to require gnutls as a prereq?
No specific platform as such, just that if you build with
--without-remote and --without-libvirtd we should not be
mandating use of gnutls. Various people have asked for this
feature over the years, so I think it is worth it.
>
> Overall, your patch looks sane, and you have a 'weak ACK' - that is, I'm
> willing to look the other way and let this patch go in, if you don't
> think it is worth even more refactoring to avoid quite so much leaky
> #ifdef throughout the code base.
Basically I'm following the approach used for SASL. It would be nice to
try and adapt virnet{tls,sasl}context.c so that all the functions still
exist, but have no-op impls, but that's much more work - I've tried it
before with SASL but never got a satisfactory result
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Make TLS support conditional
On 01/07/2013 08:24 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > Add checks for existance of GNUTLS and automatically disable s/existance/existence/ > it if not found. > > Signed-off-by: Daniel P. Berrange > --- > configure.ac | 70 > --- > daemon/libvirtd.c | 41 ++--- > daemon/remote.c | 2 ++ > src/Makefile.am | 8 - > src/libvirt.c | 17 --- > src/locking/lock_daemon.c | 12 ++-- > src/lxc/lxc_controller.c | 6 ++-- > src/qemu/qemu_migration.c | 15 -- > src/remote/remote_driver.c| 15 ++ > src/rpc/virnetclient.c| 20 ++--- > src/rpc/virnetclient.h| 8 - > src/rpc/virnetserver.c| 6 > src/rpc/virnetserver.h| 6 +++- > src/rpc/virnetserverclient.c | 63 ++ > src/rpc/virnetserverclient.h | 4 +++ > src/rpc/virnetserverservice.c | 31 ++- > src/rpc/virnetserverservice.h | 20 + > src/rpc/virnetsocket.c| 17 ++- > src/rpc/virnetsocket.h| 6 +++- > tests/Makefile.am | 11 ++- > 20 files changed, 311 insertions(+), 67 deletions(-) Touches quite a bit, but hopefully for the better. What platform are you targeting where you were unwilling to require gnutls as a prereq? > +++ b/configure.ac > @@ -1025,30 +1025,62 @@ CFLAGS="$old_cflags" > LIBS="$old_libs" > > dnl GnuTLS library > -GNUTLS_CFLAGS= > -GNUTLS_LIBS= > -GNUTLS_FOUND=no > -if test -x "$PKG_CONFIG" ; then > - PKG_CHECK_MODULES(GNUTLS, gnutls >= $GNUTLS_REQUIRED, > - [GNUTLS_FOUND=yes], [GNUTLS_FOUND=no]) > -fi > -if test "$GNUTLS_FOUND" = "no"; then > +AC_ARG_WITH([gnutls], > + AC_HELP_STRING([--with-gnutls], [use GNUTLS for encryption > @<:@default=check@:>@]), > + [], > + [with_gnutls=check]) > + > + > +if test "x$with_gnutls" != "xno"; then > + if test "x$with_gnutls" != "xyes" && test "x$with_gnutls" != "xcheck"; then > +GNUTLS_CFLAGS="-I$with_gnutls/include" > +GNUTLS_LIBS="-L$with_gnutls/lib" > + fi >fail=0 > + old_cflags="$CFLAGS" >old_libs="$LIBS" > - AC_CHECK_HEADER([gnutls/gnutls.h], [], [fail=1]) > - AC_CHECK_LIB([gnutls], [gnutls_handshake],[], [fail=1], [-lgcrypt]) > + CFLAGS="$CFLAGS $GNUTLS_CFLAGS" > + LIBS="$LIBS $GNUTLS_LIBS" > > - test $fail = 1 && > -AC_MSG_ERROR([You must install the GnuTLS library in order to compile > and run libvirt]) > + GNUTLS_FOUND=no > + if test -x "$PKG_CONFIG" ; then > +PKG_CHECK_MODULES(GNUTLS, gnutls >= $GNUTLS_REQUIRED, > + [GNUTLS_FOUND=yes], [GNUTLS_FOUND=no]) > + fi > + if test "$GNUTLS_FOUND" = "no"; then > +fail=0 > +AC_CHECK_HEADER([gnutls/gnutls.h], [], [fail=1]) > +AC_CHECK_LIB([gnutls], [gnutls_handshake],[], [fail=1], [-lgcrypt]) > + > +test $fail = 0 && GNUTLS_FOUND=yes > + > +GNUTLS_LIBS="$GNUTLS_LIBS -lgnutls" It looks fishy requiring -lgcrypt for the probe, but not using it here... > + fi > + if test "$GNUTLS_FOUND" = "no"; then > +if test "$with_gnutls" = "check"; then > + with_gnutls=no > + GNUTLS_LIBS= > + GNUTLS_CFLAGS= > +else > + 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" ...but then you unconditionally add it in later. I guess that's okay. > +++ b/daemon/libvirtd.c > @@ -449,7 +449,9 @@ static int daemonSetupNetworking(virNetServerPtr srv, > virNetServerServicePtr svc = NULL; > virNetServerServicePtr svcRO = NULL; > virNetServerServicePtr svcTCP = NULL; > +#if HAVE_GNUTLS > virNetServerServicePtr svcTLS = NULL; > +#endif This makes for a lot of #ifdef'd code, and I'm worried that we might get out of sync depending on whether gnutls is present or not. Would it be any easier to always declare this variable, but always have it NULL when tls is not present? > gid_t unix_sock_gid = 0; > int unix_sock_ro_mask = 0; > int unix_sock_rw_mask = 0; > @@ -474,9 +476,11 @@ static int daemonSetupNetworking(virNetServerPtr srv, > unix_sock_rw_mask, > unix_sock_gid, > config->auth_unix_rw, > +#if HAVE_GNUTLS > + NULL, > +#endif Likewise, having different signatures based on the #ifdef seems awkward, compared to making the conditional code just ignore the parameter when tls is not present. That said, this is not the first time we've done this (the code for HAVE_SASL is equally if-def'd), so I guess I can live with it. > > #if HAVE_SASL > if (config->auth_unix_r
[libvirt] [PATCH] Make TLS support conditional
From: "Daniel P. Berrange" Add checks for existance of GNUTLS and automatically disable it if not found. Signed-off-by: Daniel P. Berrange --- configure.ac | 70 --- daemon/libvirtd.c | 41 ++--- daemon/remote.c | 2 ++ src/Makefile.am | 8 - src/libvirt.c | 17 --- src/locking/lock_daemon.c | 12 ++-- src/lxc/lxc_controller.c | 6 ++-- src/qemu/qemu_migration.c | 15 -- src/remote/remote_driver.c| 15 ++ src/rpc/virnetclient.c| 20 ++--- src/rpc/virnetclient.h| 8 - src/rpc/virnetserver.c| 6 src/rpc/virnetserver.h| 6 +++- src/rpc/virnetserverclient.c | 63 ++ src/rpc/virnetserverclient.h | 4 +++ src/rpc/virnetserverservice.c | 31 ++- src/rpc/virnetserverservice.h | 20 + src/rpc/virnetsocket.c| 17 ++- src/rpc/virnetsocket.h| 6 +++- tests/Makefile.am | 11 ++- 20 files changed, 311 insertions(+), 67 deletions(-) diff --git a/configure.ac b/configure.ac index ab08f17..bb64bf6 100644 --- a/configure.ac +++ b/configure.ac @@ -1025,30 +1025,62 @@ CFLAGS="$old_cflags" LIBS="$old_libs" dnl GnuTLS library -GNUTLS_CFLAGS= -GNUTLS_LIBS= -GNUTLS_FOUND=no -if test -x "$PKG_CONFIG" ; then - PKG_CHECK_MODULES(GNUTLS, gnutls >= $GNUTLS_REQUIRED, - [GNUTLS_FOUND=yes], [GNUTLS_FOUND=no]) -fi -if test "$GNUTLS_FOUND" = "no"; then +AC_ARG_WITH([gnutls], + AC_HELP_STRING([--with-gnutls], [use GNUTLS for encryption @<:@default=check@:>@]), + [], + [with_gnutls=check]) + + +if test "x$with_gnutls" != "xno"; then + if test "x$with_gnutls" != "xyes" && test "x$with_gnutls" != "xcheck"; then +GNUTLS_CFLAGS="-I$with_gnutls/include" +GNUTLS_LIBS="-L$with_gnutls/lib" + fi fail=0 + old_cflags="$CFLAGS" old_libs="$LIBS" - AC_CHECK_HEADER([gnutls/gnutls.h], [], [fail=1]) - AC_CHECK_LIB([gnutls], [gnutls_handshake],[], [fail=1], [-lgcrypt]) + CFLAGS="$CFLAGS $GNUTLS_CFLAGS" + LIBS="$LIBS $GNUTLS_LIBS" - test $fail = 1 && -AC_MSG_ERROR([You must install the GnuTLS library in order to compile and run libvirt]) + GNUTLS_FOUND=no + if test -x "$PKG_CONFIG" ; then +PKG_CHECK_MODULES(GNUTLS, gnutls >= $GNUTLS_REQUIRED, + [GNUTLS_FOUND=yes], [GNUTLS_FOUND=no]) + fi + if test "$GNUTLS_FOUND" = "no"; then +fail=0 +AC_CHECK_HEADER([gnutls/gnutls.h], [], [fail=1]) +AC_CHECK_LIB([gnutls], [gnutls_handshake],[], [fail=1], [-lgcrypt]) + +test $fail = 0 && GNUTLS_FOUND=yes + +GNUTLS_LIBS="$GNUTLS_LIBS -lgnutls" + fi + if test "$GNUTLS_FOUND" = "no"; then +if test "$with_gnutls" = "check"; then + with_gnutls=no + GNUTLS_LIBS= + GNUTLS_CFLAGS= +else + 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" + +with_gnutls=yes + fi - 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="$LIBS -lgcrypt" LIBS="$old_libs" -else - GNUTLS_LIBS="$GNUTLS_LIBS -lgcrypt" + CFLAGS="$old_CFLAGS" fi +if test "x$with_gnutls" = "xyes" ; then + AC_DEFINE_UNQUOTED([HAVE_GNUTLS], 1, + [whether GNUTLS is available for encryption]) +fi +AM_CONDITIONAL([HAVE_GNUTLS], [test "x$with_gnutls" = "xyes"]) AC_SUBST([GNUTLS_CFLAGS]) AC_SUBST([GNUTLS_LIBS]) @@ -3168,7 +3200,11 @@ AC_MSG_NOTICE([ libssh2: $LIBSSH2_CFLAGS $LIBSSH2_LIBS]) else AC_MSG_NOTICE([ libssh2: no]) fi +if test "$with_gnutls" != "no" ; then AC_MSG_NOTICE([ gnutls: $GNUTLS_CFLAGS $GNUTLS_LIBS]) +else +AC_MSG_NOTICE([ gnutls: no]) +fi if test "$with_sasl" != "no" ; then AC_MSG_NOTICE([sasl: $SASL_CFLAGS $SASL_LIBS]) else diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index fa4d129..ff54af3 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -449,7 +449,9 @@ static int daemonSetupNetworking(virNetServerPtr srv, virNetServerServicePtr svc = NULL; virNetServerServicePtr svcRO = NULL; virNetServerServicePtr svcTCP = NULL; +#if HAVE_GNUTLS virNetServerServicePtr svcTLS = NULL; +#endif gid_t unix_sock_gid = 0; int unix_sock_ro_mask = 0; int unix_sock_rw_mask = 0; @@ -474,9 +476,11 @@ static int daemonSetupNetworking(virNetServerPtr srv, unix_sock_rw_mask, unix_sock_gid, config->auth_unix_rw, +#if HAVE_GNUTLS + NULL, +#endif false, -
