Issue 7877 - please make gcrypt optional with newer gnutls
Summary: please make gcrypt optional with newer gnutls
Status: VERIFIED FIXED
Alias: None
Product: OpenLDAP
Classification: Unclassified
Component: build (show other issues)
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: ---
Assignee: OpenLDAP project
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-20 17:20 UTC by Ryan Tandy
Modified: 2014-10-23 07:28 UTC (History)
0 users

See Also:


Attachments
0001-ITS-7877-detect-whether-gnutls-uses-gcrypt.patch (2.48 KB, patch)
2014-06-21 00:23 UTC, Ryan Tandy
Details
0001-ITS-7877-support-nettle-in-smbk5pwd.patch (2.84 KB, patch)
2014-06-23 18:56 UTC, Ryan Tandy
Details

Note You need to log in before you can comment on or make changes to this issue.
Description Ryan Tandy 2014-06-20 17:20:55 UTC
Full_Name: Ryan Tandy
Version: HEAD
OS: Debian unstable
URL: 
Submission from: (NULL) (142.32.208.235)


Debian bug report: https://bugs.debian.org/745231

Quoting Andreas Metzler:

"given that gmp has been dual-licensed LGPLv3+/GPLv2+ it should be possible to
switch openldap over to the newer version of gnutls.

Upstream's 0205e83f4670d10ad3c6ae4b8fc5ec1d0c7020c0 lets the Debian package
build successfully (including testsuite).

However even with patch there is still some work to be done.
libraries/libldap/tls_g.c has some gcrypt related code that should be simply
unnecessary with gnutls3, therefore it should not link against libgcrypt either.
(Except for contrib/slapd-modules/smbk5pwd/smbk5pwd.c)."

The following changes make gcrypt optional for libldap. For versions where both
nettle and gcrypt are supported, I assume the default since no mechanism is
provided for detecting which is actually in use. Tested with GnuTLS 2.8.6 and
3.2.15.


--- a/libraries/libldap/tls_g.c
+++ b/libraries/libldap/tls_g.c
@@ -43,10 +43,17 @@
 
 #include <gnutls/gnutls.h>
 #include <gnutls/x509.h>
-#include <gcrypt.h>
 
 #if LIBGNUTLS_VERSION_NUMBER >= 0x020200
 #define        HAVE_CIPHERSUITES       1
+#else
+#undef HAVE_CIPHERSUITES
+#endif
+
+/* gnutls >= 2.11.1 no longer uses gcrypt by default */
+#if LIBGNUTLS_VERSION_NUMBER < 0x020b01
+#include <gcrypt.h>
+#if LIBGNUTLS_VERSION_NUMBER >= 0x020200
 /* This is a kludge. gcrypt 1.4.x has support. Recent GnuTLS requires gcrypt
1.4.x
  * but that dependency isn't reflected in their configure script, resulting in
  * build errors on older gcrypt. So, if they have a working build environment,
@@ -54,9 +61,10 @@
  */
 #define HAVE_GCRYPT_RAND       1
 #else
-#undef HAVE_CIPHERSUITES
 #undef HAVE_GCRYPT_RAND
 #endif
+#endif
+
 
 #ifndef HAVE_CIPHERSUITES
 /* Versions prior to 2.2.0 didn't handle cipher suites, so we had to
@@ -143,6 +151,7 @@ tlsg_mutex_unlock( void **lock )
        return ldap_pvt_thread_mutex_unlock( *lock );
 }
 
+#if GNUTLS_VERSION_NUMBER <= 0x020b00
 static struct gcry_thread_cbs tlsg_thread_cbs = {
        GCRY_THREAD_OPTION_USER,
        NULL,
@@ -158,6 +167,16 @@ tlsg_thr_init( void )
 {
        gcry_control (GCRYCTL_SET_THREAD_CBS, &tlsg_thread_cbs);
 }
+#else
+static void
+tlsg_thr_init( void )
+{
+       gnutls_global_set_mutex (tlsg_mutex_init,
+               tlsg_mutex_destroy,
+               tlsg_mutex_lock,
+               tlsg_mutex_unlock);
+}
+#endif
 #endif /* LDAP_R_COMPILE */
 
 /*


I have not looked at smbk5pwd yet.
Comment 1 Ryan Tandy 2014-06-21 00:23:33 UTC
This might be a better patch, if the build system change is acceptable.
Comment 2 Ryan Tandy 2014-06-23 18:56:03 UTC
And here are the changes for smbk5pwd. Tried to use gnutls' own api
since it abstracts gcrypt/nettle, but sadly it doesn't provide md4, so
nettle it is.

Note this patch assumes the HAVE_GNUTLS_GCRYPT define from the
configure addition in the previous patch.
Comment 3 Ryan Tandy 2014-06-23 19:33:47 UTC
On Fri, Jun 20, 2014 at 5:23 PM, Ryan Tandy <ryan@nardis.ca> wrote:
> This might be a better patch, if the build system change is acceptable.

As usual, I can't get anything right on the first try. That one was
missing a line (but apparently not one that stopped it from building
or working); and besides that I'm not supposed to be attaching these
in the first place, right?

Proper patch series against current master:

ftp://ftp.openldap.org/incoming/20140623-rtandy-0001-ITS-7877-detect-whether-gnutls-uses-gcrypt.patch
ftp://ftp.openldap.org/incoming/20140623-rtandy-0002-ITS-7877-support-nettle-in-smbk5pwd.patch

Sorry for the noise, administer cluehammer as necessary...

Comment 4 Howard Chu 2014-06-30 12:05:56 UTC
ryan@nardis.ca wrote:
> Full_Name: Ryan Tandy
> Version: HEAD
> OS: Debian unstable
> URL:
> Submission from: (NULL) (142.32.208.235)
>
>
> Debian bug report: https://bugs.debian.org/745231
>
> Quoting Andreas Metzler:
>
> "given that gmp has been dual-licensed LGPLv3+/GPLv2+ it should be possible to
> switch openldap over to the newer version of gnutls.
>
> Upstream's 0205e83f4670d10ad3c6ae4b8fc5ec1d0c7020c0 lets the Debian package
> build successfully (including testsuite).

The only reason GnuTLS support exists in OpenLDAP is because of Debian. 
Therefore, if Debian no longer uses libgcrypt, I'm happy to rip all of that 
crap out. There's no reason for us to even keep optional support for it 
because that gives the mistaken impression that we endorse its use. Which we 
most vehemently do not.

> However even with patch there is still some work to be done.
> libraries/libldap/tls_g.c has some gcrypt related code that should be simply
> unnecessary with gnutls3, therefore it should not link against libgcrypt either.
> (Except for contrib/slapd-modules/smbk5pwd/smbk5pwd.c)."
>
> The following changes make gcrypt optional for libldap. For versions where both
> nettle and gcrypt are supported, I assume the default since no mechanism is
> provided for detecting which is actually in use.

Yet another flaw in GnuTLS design...

> Tested with GnuTLS 2.8.6 and
> 3.2.15.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Comment 5 Howard Chu 2014-06-30 12:25:17 UTC
hyc@symas.com wrote:
> ryan@nardis.ca wrote:
>> Full_Name: Ryan Tandy
>> Version: HEAD
>> OS: Debian unstable
>> URL:
>> Submission from: (NULL) (142.32.208.235)

>> The following changes make gcrypt optional for libldap. For versions where both
>> nettle and gcrypt are supported, I assume the default since no mechanism is
>> provided for detecting which is actually in use.
>
> Yet another flaw in GnuTLS design...
>
>> Tested with GnuTLS 2.8.6 and
>> 3.2.15.

Just tell us at which version of GnuTLS you switched to nettle and we'll make 
that the minimum supported version.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Comment 6 Howard Chu 2014-06-30 20:29:18 UTC
changed notes
changed state Open to Test
moved from Incoming to Build
Comment 7 Ryan Tandy 2014-06-30 21:51:22 UTC
On Mon, Jun 30, 2014 at 5:05 AM, Howard Chu <hyc@symas.com> wrote:
> The only reason GnuTLS support exists in OpenLDAP is because of Debian.
> Therefore, if Debian no longer uses libgcrypt, I'm happy to rip all of that
> crap out.

Sounds good to me. So a patch that removes gcrypt entirely looks like:

ftp://ftp.openldap.org/incoming/20140630_rtandy_0001-ITS-7877-use-nettle-instead-of-gcrypt.patch

(I assume the explicit threading setup is important, if not maybe the
gnutls_global_set_mutex part could be removed too...)

That requires gnutls 2.12.0 or newer, so then I think we can also
remove the compatibility code for older versions:

ftp://ftp.openldap.org/incoming/20140630_rtandy_0002-assume-gnutls-provides-cipher-suites.patch
ftp://ftp.openldap.org/incoming/20140630_rtandy_0003-assume-gnutls-is-at-least-2.12.0.patch

> Just tell us at which version of GnuTLS you switched to nettle and we'll make
> that the minimum supported version.

Debian builds gnutls with nettle starting from 3.0.0. The API used in
tls_g.c is all backend agnostic though. I tried with 2.12.20 (with
gcrypt backend) and everything looks fine in slapd and clients
including the threading setup. I think 2.12.0 as minimum version would
be fine, and then nettle vs gcrypt only matters for smbk5pwd users.

Thanks for considering my patches.

Comment 8 Howard Chu 2014-07-01 03:24:16 UTC
ryan@nardis.ca wrote:
> On Mon, Jun 30, 2014 at 5:05 AM, Howard Chu <hyc@symas.com> wrote:
>> The only reason GnuTLS support exists in OpenLDAP is because of Debian.
>> Therefore, if Debian no longer uses libgcrypt, I'm happy to rip all of that
>> crap out.
>
> Sounds good to me. So a patch that removes gcrypt entirely looks like:
>
> ftp://ftp.openldap.org/incoming/20140630_rtandy_0001-ITS-7877-use-nettle-instead-of-gcrypt.patch
>
> (I assume the explicit threading setup is important, if not maybe the
> gnutls_global_set_mutex part could be removed too...)
>
> That requires gnutls 2.12.0 or newer, so then I think we can also
> remove the compatibility code for older versions:
>
> ftp://ftp.openldap.org/incoming/20140630_rtandy_0002-assume-gnutls-provides-cipher-suites.patch
> ftp://ftp.openldap.org/incoming/20140630_rtandy_0003-assume-gnutls-is-at-least-2.12.0.patch
>
>> Just tell us at which version of GnuTLS you switched to nettle and we'll make
>> that the minimum supported version.
>
> Debian builds gnutls with nettle starting from 3.0.0. The API used in
> tls_g.c is all backend agnostic though. I tried with 2.12.20 (with
> gcrypt backend) and everything looks fine in slapd and clients
> including the threading setup. I think 2.12.0 as minimum version would
> be fine, and then nettle vs gcrypt only matters for smbk5pwd users.
>
> Thanks for considering my patches.

Committed to master. I've also added a check for 2.12.0 to the configure script.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Comment 9 Quanah Gibson-Mount 2014-07-22 11:51:50 UTC
changed notes
changed state Test to Release
Comment 10 OpenLDAP project 2014-10-23 07:28:15 UTC
in master
in RE25
in RE24
Comment 11 Quanah Gibson-Mount 2014-10-23 07:28:15 UTC
changed notes
changed state Release to Closed