Re: [HACKERS] GnuTLS support

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-11-29 16:34:13 -0500, Tom Lane wrote:
> > Yeah, I was disappointed too.  OpenSSL has had a squirrelly enough track
> > record that it'd be nice not to be totally dependent on it.
> 
> GnuTLS seems, if anything, worse though. There's obviously good reasons
> to add support for TLS libraries that make it easier to use PG on
> certain platforms, but GnuTLS doesn't achieve that.  So I don't think
> this is too sad.

There are very good reasons to give our users the option of different
TLS libraries, even if it's platforms where OpenSSL is also available,
for the reason Tom mentioned- OpenSSL hasn't had a terribly good track
record, and because there's been independent evaluation of different
libraries and OpenSSL doesn't top the list in those.

As such, I do believe it'd be good to have support for multiple
libraries, even on Linux or other platforms where OpenSSL is available.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Thu, Nov 29, 2018 at 8:28 AM Peter Eisentraut
> >  wrote:
> >> I have decided that I don't want to pursue this patch anymore.  It has
> >> served its purpose having allowed us to refine the SSL library
> >> abstractions so that alternative implementations such as macOS Secure
> >> Transport can go ahead.  But officially supporting GnuTLS as an
> >> alternative to OpenSSL doesn't seem to have any practical advantages, so
> >> I don't foresee this getting committed into PostgreSQL core.
> 
> > Hmm, I find that a bit disappointing. I'm not in a position to take up
> > the patch right now, unfortunately.
> 
> Yeah, I was disappointed too.  OpenSSL has had a squirrelly enough track
> record that it'd be nice not to be totally dependent on it.  But, like
> both of you, I'm not quite motivated enough to take up the patch myself.

I'm also pretty disappointed by this, although admittedly I think my
interest would be more in adding libNSS support than GnuTLS, but I had
viewed this as a good stepping stone to get there.  Perhaps it still can
be though.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Andres Freund]

Hi,

On 2018-11-29 16:34:13 -0500, Tom Lane wrote:
> Yeah, I was disappointed too.  OpenSSL has had a squirrelly enough track
> record that it'd be nice not to be totally dependent on it.

GnuTLS seems, if anything, worse though. There's obviously good reasons
to add support for TLS libraries that make it easier to use PG on
certain platforms, but GnuTLS doesn't achieve that.  So I don't think
this is too sad.

- Andres

Re: [HACKERS] GnuTLS support

[Tom Lane]

Robert Haas  writes:
> On Thu, Nov 29, 2018 at 8:28 AM Peter Eisentraut
>  wrote:
>> I have decided that I don't want to pursue this patch anymore.  It has
>> served its purpose having allowed us to refine the SSL library
>> abstractions so that alternative implementations such as macOS Secure
>> Transport can go ahead.  But officially supporting GnuTLS as an
>> alternative to OpenSSL doesn't seem to have any practical advantages, so
>> I don't foresee this getting committed into PostgreSQL core.

> Hmm, I find that a bit disappointing. I'm not in a position to take up
> the patch right now, unfortunately.

Yeah, I was disappointed too.  OpenSSL has had a squirrelly enough track
record that it'd be nice not to be totally dependent on it.  But, like
both of you, I'm not quite motivated enough to take up the patch myself.

Anyway, if the OpenSSL situation changes enough to affect the cost/benefit
calculus, at least we should be able to re-open this patch with some
confidence that we've not painted ourselves into a corner.

regards, tom lane

Re: [HACKERS] GnuTLS support

[Robert Haas]

On Thu, Nov 29, 2018 at 8:28 AM Peter Eisentraut
 wrote:
> On 29/11/2018 13:28, Dmitry Dolgov wrote:
> > Unfortunately it needs to be rebased one more time, could you do this? Also 
> > I'm
> > wondering about this:
> >
> >> I'm moving this patch forward to CF 2018-09, since it's not going to be
> >> ready for -07, and we're still whacking around some channel binding
> >> details, which would potentially interfere with this patch.
> >
> > Were you talking about this one [1]? As far as I see it's not a concern
> > anymore? I'll move it to the next CF.
>
> I have decided that I don't want to pursue this patch anymore.  It has
> served its purpose having allowed us to refine the SSL library
> abstractions so that alternative implementations such as macOS Secure
> Transport can go ahead.  But officially supporting GnuTLS as an
> alternative to OpenSSL doesn't seem to have any practical advantages, so
> I don't foresee this getting committed into PostgreSQL core.

Hmm, I find that a bit disappointing. I'm not in a position to take up
the patch right now, unfortunately.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 29/11/2018 13:28, Dmitry Dolgov wrote:
> Unfortunately it needs to be rebased one more time, could you do this? Also 
> I'm
> wondering about this:
> 
>> I'm moving this patch forward to CF 2018-09, since it's not going to be
>> ready for -07, and we're still whacking around some channel binding
>> details, which would potentially interfere with this patch.
> 
> Were you talking about this one [1]? As far as I see it's not a concern
> anymore? I'll move it to the next CF.

I have decided that I don't want to pursue this patch anymore.  It has
served its purpose having allowed us to refine the SSL library
abstractions so that alternative implementations such as macOS Secure
Transport can go ahead.  But officially supporting GnuTLS as an
alternative to OpenSSL doesn't seem to have any practical advantages, so
I don't foresee this getting committed into PostgreSQL core.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Dmitry Dolgov]

> On Fri, Aug 31, 2018 at 1:28 PM Peter Eisentraut 
>  wrote:
>
> On 20/08/2018 05:13, Michael Paquier wrote:
> > Patch v6 of this thread is failing to apply.  Could you rebase?
>
> attached
>
> Changes in v7 since v6:
>
> - Added support for ssl_passphrase_command.
>
> - Test suite needed some adjustment because GnuTLS doesn't appear to
> understand the previously used file format for encrypted keys.
>
> - Removed tls-unique channel binding support.  Support for
> tls-server-end-point still needs to be added, but it could be a separate
> patch.

Unfortunately it needs to be rebased one more time, could you do this? Also I'm
wondering about this:

> I'm moving this patch forward to CF 2018-09, since it's not going to be
> ready for -07, and we're still whacking around some channel binding
> details, which would potentially interfere with this patch.

Were you talking about this one [1]? As far as I see it's not a concern
anymore? I'll move it to the next CF.

[1]: 
https://www.postgresql.org/message-id/flat/20180712041410.GC7352%40paquier.xyz#84bdb384b9e70f039fb849eed8a45817

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 20/08/2018 05:13, Michael Paquier wrote:
> Patch v6 of this thread is failing to apply.  Could you rebase?

attached

Changes in v7 since v6:

- Added support for ssl_passphrase_command.

- Test suite needed some adjustment because GnuTLS doesn't appear to
understand the previously used file format for encrypted keys.

- Removed tls-unique channel binding support.  Support for
tls-server-end-point still needs to be added, but it could be a separate
patch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 85465f4ad3e210a3948216ebc5c6fbd8c6993bdb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 31 Aug 2018 13:00:26 +0200
Subject: [PATCH v7] GnuTLS support

---
 configure | 166 +++-
 configure.in  |  37 +-
 doc/src/sgml/client-auth.sgml |   2 +-
 doc/src/sgml/config.sgml  |  69 +-
 doc/src/sgml/installation.sgml|  47 +-
 doc/src/sgml/libpq.sgml   |  54 +-
 doc/src/sgml/runtime.sgml |  13 +-
 doc/src/sgml/sslinfo.sgml |   1 +
 src/Makefile.global.in|   1 +
 src/backend/libpq/Makefile|   4 +-
 src/backend/libpq/be-secure-gnutls.c  | 795 +
 src/backend/libpq/be-secure-openssl.c |   4 +-
 src/backend/libpq/be-secure.c |   3 +
 src/backend/libpq/hba.c   |   2 +-
 src/backend/utils/misc/guc.c  |  25 +
 src/backend/utils/misc/postgresql.conf.sample |  11 +-
 src/common/Makefile   |   4 +-
 src/common/sha2_gnutls.c  |  99 +++
 src/include/common/sha2.h |  14 +-
 src/include/libpq/libpq-be.h  |  15 +-
 src/include/libpq/libpq.h |   1 +
 src/include/pg_config.h.in|  17 +
 src/include/pg_config_manual.h|   2 +-
 src/interfaces/libpq/.gitignore   |   1 +
 src/interfaces/libpq/Makefile |  14 +-
 src/interfaces/libpq/fe-secure-gnutls.c   | 803 ++
 src/interfaces/libpq/fe-secure.c  |   2 +-
 src/interfaces/libpq/libpq-fe.h   |   2 +-
 src/interfaces/libpq/libpq-int.h  |  14 +-
 src/port/pg_strong_random.c   |  18 +-
 src/test/Makefile |   2 +-
 src/test/ssl/Makefile |   7 +-
 src/test/ssl/ssl/server-password.key  |  35 +-
 src/test/ssl/t/001_ssltests.pl|  63 +-
 src/test/ssl/t/002_scram.pl   |   2 +-
 src/tools/msvc/Mkvcbuild.pm   |  10 +
 src/tools/pgindent/typedefs.list  |   3 +
 37 files changed, 2248 insertions(+), 114 deletions(-)
 create mode 100644 src/backend/libpq/be-secure-gnutls.c
 create mode 100644 src/common/sha2_gnutls.c
 create mode 100644 src/interfaces/libpq/fe-secure-gnutls.c

diff --git a/configure b/configure
index dd94c5bbab..564f33ae5d 100755
--- a/configure
+++ b/configure
@@ -707,6 +707,7 @@ UUID_EXTRA_OBJS
 with_uuid
 with_systemd
 with_selinux
+with_gnutls
 with_openssl
 with_ldap
 with_krb_srvnam
@@ -853,6 +854,7 @@ with_bsd_auth
 with_ldap
 with_bonjour
 with_openssl
+with_gnutls
 with_selinux
 with_systemd
 with_readline
@@ -1553,6 +1555,7 @@ Optional Packages:
   --with-ldap build with LDAP support
   --with-bonjour  build with Bonjour support
   --with-openssl  build with OpenSSL support
+  --with-gnutls   build with GnuTLS support
   --with-selinux  build with SELinux support
   --with-systemd  build with systemd support
   --without-readline  do not use GNU Readline nor BSD Libedit for editing
@@ -7933,6 +7936,47 @@ fi
 $as_echo "$with_openssl" >&6; }
 
 
+
+#
+# GnuTLS
+#
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with GnuTLS 
support" >&5
+$as_echo_n "checking whether to build with GnuTLS support... " >&6; }
+
+
+
+# Check whether --with-gnutls was given.
+if test "${with_gnutls+set}" = set; then :
+  withval=$with_gnutls;
+  case $withval in
+yes)
+
+$as_echo "#define USE_GNUTLS 1" >>confdefs.h
+
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-gnutls option" "$LINENO" 
5
+  ;;
+  esac
+
+else
+  with_gnutls=no
+
+fi
+
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_gnutls" >&5
+$as_echo "$with_gnutls" >&6; }
+
+
+if test "$with_openssl" = yes && test "$with_gnutls" = yes; then
+  as_fn_error $? "cannot specify both --with-openssl and --with-gnutls" 
"$LINENO" 5
+fi
+
+
 #
 # SELinux
 #
@@ -12052,6 +12096,107 @@ done
 
 fi
 
+if test "$with_gnutls" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing 
gnutls_init" >&5
+$as_echo_n "checking for library 

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Thu, Mar 08, 2018 at 02:13:51PM -0500, Peter Eisentraut wrote:
> In the thread about Secure Transport we agreed to move the consideration
> of new SSL libraries to PG12.
> 
> Here is my current patch, after all the refactorings.
> 
> The status is that it works fine and could be used.
> 
> There are two failures in the SSL tests that I cannot explain.  The
> tests are for some rather obscure configurations, so the changed
> behaviors are not obviously wrong, perhaps legitimate implementation
> differences.  But someone wrote those tests with a purpose (probably),
> so we should have some kind of explanation for the regressions.

Patch v6 of this thread is failing to apply.  Could you rebase?
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Heikki Linnakangas]

On 05/06/18 00:44, Peter Eisentraut wrote:

On 6/2/18 16:50, Heikki Linnakangas wrote:

On 08/03/18 14:13, Peter Eisentraut wrote:

There are two failures in the SSL tests that I cannot explain.  The
tests are for some rather obscure configurations, so the changed
behaviors are not obviously wrong, perhaps legitimate implementation
differences.  But someone wrote those tests with a purpose (probably),
so we should have some kind of explanation for the regressions.


I applied this over commit 4e0c743c18 (because this doesn't compile
against current master, needs rebasing), and ran "make check" in
src/test/ssl. All the tests passed. I'm using GnuTLS version 3.5.8. What
failures did you see?


The patch adjusts the expected test results so that the tests pass.


Ah, gotcha.


Look for the tests named

- "connect with server CA cert, without root CA"


So, in this test, the client puts the server's certificate in 
sslrootcert, but not the CA cert that the server's certificate was 
signed with. OpenSSL doesn't accept that, but apparently GnuTLS is OK 
with it.


I think the GnuTLS behavior is reasonable, I was actually surprised that 
OpenSSL is so strict about that. If the user explicitly lists a server's 
certificate as trusted, by putting it in sslrootcert, it seems 
reasonable to accept it even if the CA cert is missing.



- "CRL belonging to a different CA"


Hmm. So in OpenSSL, when we load the CRL, we call 
X509_STORE_set_flags(cvstore, X509_V_FLAG_CRL_CHECK | 
X509_V_FLAG_CRL_CHECK_ALL). With that option, if a CRL for the server CA 
cannot be found (in this case, because the CRL is for a different CA), 
OpenSSL throws an error. Apparently, GnuTLS is more lenient. At a quick 
glance, I don't see an option in GnuTLS to change that behavior. But I 
think we can live with it, it's not wrong per se, just different.


- Heikki

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 3/8/18 20:13, Peter Eisentraut wrote:
> In the thread about Secure Transport we agreed to move the consideration
> of new SSL libraries to PG12.
> 
> Here is my current patch, after all the refactorings.
> 
> The status is that it works fine and could be used.
> 
> There are two failures in the SSL tests that I cannot explain.  The
> tests are for some rather obscure configurations, so the changed
> behaviors are not obviously wrong, perhaps legitimate implementation
> differences.  But someone wrote those tests with a purpose (probably),
> so we should have some kind of explanation for the regressions.
> 
> Other non-critical, nice-to-have issues:
> 
> - Do something about sslinfo, perhaps fold into pg_stat_ssl view.
> - Do something about pgcrypto.
> - Add tests for load_dh_file().
> - Implement channel binding tls-server-end-point.

Also, ...

- Add ssl_passphrase_command support.

I'm moving this patch forward to CF 2018-09, since it's not going to be
ready for -07, and we're still whacking around some channel binding
details, which would potentially interfere with this patch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 6/2/18 16:50, Heikki Linnakangas wrote:
> On 08/03/18 14:13, Peter Eisentraut wrote:
>> There are two failures in the SSL tests that I cannot explain.  The
>> tests are for some rather obscure configurations, so the changed
>> behaviors are not obviously wrong, perhaps legitimate implementation
>> differences.  But someone wrote those tests with a purpose (probably),
>> so we should have some kind of explanation for the regressions.
> 
> I applied this over commit 4e0c743c18 (because this doesn't compile 
> against current master, needs rebasing), and ran "make check" in 
> src/test/ssl. All the tests passed. I'm using GnuTLS version 3.5.8. What 
> failures did you see?

The patch adjusts the expected test results so that the tests pass.

Look for the tests named

- "connect with server CA cert, without root CA"
- "CRL belonging to a different CA"

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Heikki Linnakangas]

On 08/03/18 14:13, Peter Eisentraut wrote:

There are two failures in the SSL tests that I cannot explain.  The
tests are for some rather obscure configurations, so the changed
behaviors are not obviously wrong, perhaps legitimate implementation
differences.  But someone wrote those tests with a purpose (probably),
so we should have some kind of explanation for the regressions.


I applied this over commit 4e0c743c18 (because this doesn't compile 
against current master, needs rebasing), and ran "make check" in 
src/test/ssl. All the tests passed. I'm using GnuTLS version 3.5.8. What 
failures did you see?


- Heikki

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

In the thread about Secure Transport we agreed to move the consideration
of new SSL libraries to PG12.

Here is my current patch, after all the refactorings.

The status is that it works fine and could be used.

There are two failures in the SSL tests that I cannot explain.  The
tests are for some rather obscure configurations, so the changed
behaviors are not obviously wrong, perhaps legitimate implementation
differences.  But someone wrote those tests with a purpose (probably),
so we should have some kind of explanation for the regressions.

Other non-critical, nice-to-have issues:

- Do something about sslinfo, perhaps fold into pg_stat_ssl view.
- Do something about pgcrypto.
- Add tests for load_dh_file().
- Implement channel binding tls-server-end-point.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From c76b6efca0f52fe4deb6003009f4f8730201f041 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 8 Mar 2018 14:05:33 -0500
Subject: [PATCH v6] GnuTLS support

---
 configure | 258 ++--
 configure.in  |  37 +-
 doc/src/sgml/client-auth.sgml |   2 +-
 doc/src/sgml/config.sgml  |  81 ++-
 doc/src/sgml/installation.sgml|  47 +-
 doc/src/sgml/libpq.sgml   |  54 +-
 doc/src/sgml/runtime.sgml |  13 +-
 doc/src/sgml/sslinfo.sgml |   1 +
 src/Makefile.global.in|   1 +
 src/backend/libpq/Makefile|   4 +-
 src/backend/libpq/be-secure-gnutls.c  | 820 +
 src/backend/libpq/be-secure-openssl.c |   4 +-
 src/backend/libpq/be-secure.c |   4 +
 src/backend/libpq/hba.c   |   2 +-
 src/backend/utils/misc/guc.c  |  38 ++
 src/backend/utils/misc/postgresql.conf.sample |   7 +-
 src/common/Makefile   |   4 +-
 src/common/sha2_gnutls.c  |  99 +++
 src/include/common/sha2.h |  14 +-
 src/include/libpq/libpq-be.h  |  13 +-
 src/include/libpq/libpq.h |   2 +
 src/include/pg_config.h.in|  17 +
 src/include/pg_config_manual.h|   2 +-
 src/interfaces/libpq/.gitignore   |   1 +
 src/interfaces/libpq/Makefile |  14 +-
 src/interfaces/libpq/fe-secure-gnutls.c   | 836 ++
 src/interfaces/libpq/fe-secure.c  |   2 +-
 src/interfaces/libpq/libpq-fe.h   |   2 +-
 src/interfaces/libpq/libpq-int.h  |  14 +-
 src/port/pg_strong_random.c   |  18 +-
 src/test/Makefile |   2 +-
 src/test/ssl/Makefile |   2 +-
 src/test/ssl/t/001_ssltests.pl|  65 +-
 src/test/ssl/t/002_scram.pl   |   2 +-
 src/tools/msvc/Mkvcbuild.pm   |  10 +
 src/tools/pgindent/typedefs.list  |   3 +
 36 files changed, 2360 insertions(+), 135 deletions(-)
 create mode 100644 src/backend/libpq/be-secure-gnutls.c
 create mode 100644 src/common/sha2_gnutls.c
 create mode 100644 src/interfaces/libpq/fe-secure-gnutls.c

diff --git a/configure b/configure
index 3943711283..f2d4aa502a 100755
--- a/configure
+++ b/configure
@@ -707,6 +707,7 @@ UUID_EXTRA_OBJS
 with_uuid
 with_systemd
 with_selinux
+with_gnutls
 with_openssl
 with_ldap
 with_krb_srvnam
@@ -838,6 +839,7 @@ with_bsd_auth
 with_ldap
 with_bonjour
 with_openssl
+with_gnutls
 with_selinux
 with_systemd
 with_readline
@@ -1532,6 +1534,7 @@ Optional Packages:
   --with-ldap build with LDAP support
   --with-bonjour  build with Bonjour support
   --with-openssl  build with OpenSSL support
+  --with-gnutls   build with GnuTLS support
   --with-selinux  build with SELinux support
   --with-systemd  build with systemd support
   --without-readline  do not use GNU Readline nor BSD Libedit for editing
@@ -1999,6 +2002,52 @@ $as_echo "$ac_res" >&6; }
 
 } # ac_fn_c_check_func
 
+# ac_fn_c_check_decl LINENO SYMBOL VAR INCLUDES
+# -
+# Tests whether SYMBOL is declared in INCLUDES, setting cache variable VAR
+# accordingly.
+ac_fn_c_check_decl ()
+{
+  as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
+  as_decl_name=`echo $2|sed 's/ *(.*//'`
+  as_decl_use=`echo $2|sed -e 's/(/((/' -e 's/)/) 0&/' -e 's/,/) 0& (/g'`
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $as_decl_name is 
declared" >&5
+$as_echo_n "checking whether $as_decl_name is declared... " >&6; }
+if eval \${$3+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+$4
+int
+main ()
+{
+#ifndef 

Re: [HACKERS] GnuTLS support

[Robert Haas]

On Thu, Feb 1, 2018 at 5:08 AM, Christoph Berg  wrote:
> Re: Peter Eisentraut 2018-01-03 
> <99680dba-cf63-8151-1de2-46ca93897...@2ndquadrant.com>
>> One scenario is that if GnuTLS goes in, it's quite plausible that the
>> PG11 packages for Debian and Ubuntu will use it by default.  But if it
>> doesn't support tls-server-endpoint, then a JDBC client (assuming
>> channel binding support is added) can't connect to such a server with
>> SCRAM authentication over SSL (which we hope will be a popular
>> configuration), unless they manually disable channel binding altogether
>> using the new scramchannelbinding connection option.  That would be a
>> very poor experience.
>
> GnuTLS support would mean that Debian could finally link psql against
> libreadline (instead of just LD_PRELOADing it at runtime) because
> there's not OpenSSL license conflict anymore. But I'm only going to do
> that switch if there's no visible incompatibilities for clients, and
> even any server-side GUC name changes would need a damn good
> justification because they make upgrades harder. The LD_PRELOAD hack
> in psql works, there's no pressing urgency to remove it.

Yeah.  The original justification at top-of-thread for supporting
GnuTLS was licensing.  I am not a lawyer and I don't have an opinion
on how much of a licensing problem there is around OpenSSL, but if
somebody things (as they evidently do, 'cuz this patch exists) that
there's a problem there and is willing to do the work to get it fixed,
I think that's great.  Even the perception of a legal problem can
hinder adoption, and our goal is to get people to use PostgreSQL.  Or
at least, I think that should be our goal.

I don't expect that to generate a lot of work for us because, just as
we insist that people who want obscure operating systems supported
should help by arranging for buildfarm members, so too we should
insist as part of supporting GnuTLS that someone provide buildfarm
members for it.  If those buildfarm members break and don't get fixed,
then we'll have to consider removing GnuTLS support.  Of course, this
arrangement doesn't guarantee that it's going to be zero work for us,
just as people who primarily work on UNIX-like systems still have to
worry somewhat about Microsoft Windows.  But it keeps the effort
pretty manageable.  I think that's likely to be even more true for
GnuTLS, because there's a huge amount of code that can depend on
pointer width and endian-ness, whereas it seems unlikely that anything
other than SSL patches will need to care about GnuTLS.  And there's
just not that many of those.  The biggest risk seems to be that new
SSL-related features someone wants to add in the future would need to
be made to work with every SSL implementation, and I think that could
indeed be an annoyance, but I don't think we'll really know how much
of an annoyance until we try it.  It's not like we can't rip support
out again if it proves to be a huge problem (in contrast to a feature
like multixacts or partitioning, which you can't remove without
breaking upgrades).

Also, I have to admit that my experiences with OpenSSL have been
mostly negative.  The basic stuff works and is clearly documented, but
more complicated things, at least at the time I looked at it, were not
clearly documented or not documented at all, and I ended up reading
uncommented code to try to figure out how it was supposed to work.  I
don't think it's a bad thing if we do our bit to contribute to a
little competition among implementations.  I'm not sure that gcc was
worrying too much about their error message quality until llvm came
along, but I bet they're thinking about it now.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] GnuTLS support

[Christoph Berg]

Re: Peter Eisentraut 2018-01-03 
<99680dba-cf63-8151-1de2-46ca93897...@2ndquadrant.com>
> One scenario is that if GnuTLS goes in, it's quite plausible that the
> PG11 packages for Debian and Ubuntu will use it by default.  But if it
> doesn't support tls-server-endpoint, then a JDBC client (assuming
> channel binding support is added) can't connect to such a server with
> SCRAM authentication over SSL (which we hope will be a popular
> configuration), unless they manually disable channel binding altogether
> using the new scramchannelbinding connection option.  That would be a
> very poor experience.

GnuTLS support would mean that Debian could finally link psql against
libreadline (instead of just LD_PRELOADing it at runtime) because
there's not OpenSSL license conflict anymore. But I'm only going to do
that switch if there's no visible incompatibilities for clients, and
even any server-side GUC name changes would need a damn good
justification because they make upgrades harder. The LD_PRELOAD hack
in psql works, there's no pressing urgency to remove it.

Christoph

Re: [HACKERS] GnuTLS support

[Andreas Karlsson]

On 01/26/2018 03:54 AM, Peter Eisentraut wrote:

On 1/25/18 20:10, Michael Paquier wrote:

Peter, could you change ssl_version() and ssl_cipher() in sslinfo at the
same time please? I think that those should use the generic backend-side
APIs as well. sslinfo depends heavily on OpenSSL, OK, but if possible
getting this code more generic will help users of sslinfo to get
something partially working with other SSL implementations natively.


sslinfo is currently entirely dependent on OpenSSL, so I don't think
it's useful to throw in one or two isolated API changes.

I'm thinking maybe we should get rid of sslinfo and fold everything into
pg_stat_ssl.


I think sslinfo should either use the pg_tls_get_* functions or be 
removed. I do not like having an OpenSSL specific extension. One issue 
though is that pg_tls_get_* truncates strings to a given length while 
sslinfo allocates a copy and is therefore only limited by the maximum 
size of text, but this may not be an issue in practice.


Andreas

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Jan 29, 2018 at 09:16:56PM -0500, Peter Eisentraut wrote:
> On 1/28/18 23:43, Michael Paquier wrote:
>> The comment at the top of PQinitSSL mentions OpenSSL, you may want to
>> get rid of it as well.
> 
> I think that whole business is actually obsolete as of OpenSSL 1.1.0 and
> not applicable to GnuTLS, so we might just leave it to die with some
> appropriate documentation updates.

Yes, that would be fine as well.

>> To be honest, I find this refactoring confusing. As things stand on
>> HEAD, fe-secure.c depends on the contents of fe-secure-openssl.c, and
>> the dependency goes only in one direction.  With your patch, you also
>> make fe-secure-openssl.c call things within fe-secure.c.  It seems to me
>> that a cleaner split would be to introduce a common file for all the
>> low-level routines like the two ones you are introducing here, say
>> fe-secure-common.c. aND pq_verify_peer_name_matches_certificate_name and
>> pq_verify_peer_name_matches_certificate should be moved to that.
> 
> I like that.  Updated patch attached.

Thanks for the new version. This looks sane to me.

 ifeq ($(with_openssl),yes)
-OBJS += fe-secure-openssl.o sha2_openssl.o
+OBJS += fe-secure-openssl.o fe-secure-common.o sha2_openssl.o
 else
As fe-secure-common.c will only be built with SSL builds, you need also
to tweak Mkvcbuild.pm and remove it from libpq's compilation.

+   /* If string does not end in pattern (minus the wildcard), we don't
+* match */
Such comment blocks are not Postgres-like.

src/interfaces/libpq/nls.mk is updated automatically with translations
by the way?
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Andres Freund]

Hi,

On 2018-01-29 22:41:53 -0500, Tom Lane wrote:
> But I think a big part of the value here is to verify that we've
> cleaned up our internal APIs to the point where a different SSL/TLS
> implementation *could* be rolled underneath.

Yea, I completely agree with that.

> As part of that, we certainly want to look at gnutls.  There might be
> more practical value (at least in the short term) in porting to the
> macOS or Windows native TLS stacks.  But the more different libraries
> we look at in the process, the less likely we are to paint ourselves
> into a corner.

That's true. But any further development in the area is already going to
be painful with three libraries (openssl, native windows, native osx),
adding support for a fourth that doesn't buy as anything just seems to
make the situation worse.

Anyway, I'm only -0.5 on it, and I've said my spiel...

Greetings,

Andres Freund

Re: [HACKERS] GnuTLS support

[Tom Lane]

Andres Freund  writes:
> FWIW, I'm -0.5 on adding gnutls support. I've not seen any non-vague
> arguments for it, and having debugged gnutls using applications in the
> past, I'm not convinced we're not primarily increasing our workload by
> adding support. If gnutls would improve our windows or OSX situation,
> I'd think differently, but afaics it doesn't.

That's a fair point.  But I think a big part of the value here is to
verify that we've cleaned up our internal APIs to the point where a
different SSL/TLS implementation *could* be rolled underneath.  As part
of that, we certainly want to look at gnutls.  There might be more
practical value (at least in the short term) in porting to the macOS or
Windows native TLS stacks.  But the more different libraries we look at
in the process, the less likely we are to paint ourselves into a corner.

regards, tom lane

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Jan 29, 2018 at 07:39:33PM -0500, Peter Eisentraut wrote:
> I think most users actually still think about the whole topic as "SSL",
> and the leading library is called "OpenSSL", so I think we're fine.

Yes, that's my impression on the matter as well.  While renaming the
client-side parameters sounds not really plausible, the server-side
parameters could be renamed with an implementation-related prefix if
another implementation than OpenSSL is used.  Until that happens, any
server-side renaming does not justify the breakage in my opinion.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Jan 29, 2018 at 06:24:18PM -0800, Andres Freund wrote:
> FWIW, I'm -0.5 on adding gnutls support. I've not seen any non-vague
> arguments for it, and having debugged gnutls using applications in the
> past, I'm not convinced we're not primarily increasing our workload by
> adding support. If gnutls would improve our windows or OSX situation,
> I'd think differently, but afaics it doesn't.

That's an interesting point.  The last patch set presented by Peter
improves the pluggability situation for Windows and OSX as well, so
those are a good addition anyway.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Andres Freund]

On 2018-01-17 12:30:16 -0500, Peter Eisentraut wrote:
> On 1/2/18 10:35, Peter Eisentraut wrote:
> > On 11/26/17 20:05, Andreas Karlsson wrote:
> >> I have now implemented this in the attached patch (plus added support 
> >> for channel binding and rebased it) but I ran into one issue which I 
> >> have not yet solved. The script for the windows version takes the 
> >> --with-openssl= switch so that cannot just be translated to a 
> >> single --with-ssl switch. Should to have both --with-openssl and 
> >> --with-gnutls or --with-ssl=(openssl|gnutls) and --with-ssl-path=? 
> >> I also do not know the Windows build code very well (or really at all).
> > 
> > This patch appears to work well.
> 
> Seeing that Andres is apparently currently not available, I have started
> to dig through this patch myself and made some adjustments.

I presume you meant Andreas, right?


FWIW, I'm -0.5 on adding gnutls support. I've not seen any non-vague
arguments for it, and having debugged gnutls using applications in the
past, I'm not convinced we're not primarily increasing our workload by
adding support. If gnutls would improve our windows or OSX situation,
I'd think differently, but afaics it doesn't.

Greetings,

Andres Freund

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Sat, Jan 27, 2018 at 05:00:17PM -0500, Peter Eisentraut wrote:
> On 1/25/18 09:07, Peter Eisentraut wrote:
>> On 1/19/18 13:43, Peter Eisentraut wrote:
>>> Comparing the existing {be,fe}-secure-openssl.c with the proposed
>>> {be,fe}-secure-gnutls.c, and with half an eye on the previously proposed
>>> Apple Secure Transport implementation, I have identified a few more
>>> areas of refactoring that should be done in order to avoid excessive
>>> copy-and-pasting in the new implementations:
>> 
>> And here is another place that needs cleaning up, where the OpenSSL API
>> was used directly.
> 
> And here is another one.  The last one for now I think.

The comment at the top of PQinitSSL mentions OpenSSL, you may want to
get rid of it as well.

In short, this patch:
- moves wildcard_certificate_match from fe-secure-openssl.c to
fe-secure.c.
- splits verify_peer_name_matches_certificate into two, with one common
part as pq_verify_peer_name_matches_certificate and one part which is
SSL-specific as pgtls_verify_peer_name_matches_certificate_guts.
- splits verify_peer_name_matches_certificate_name into two, with one
common part as verify_peer_name_matches_certificate_name and one part
which is SSL-specific as openssl_verify_peer_name_matches_certificate_name().

To be honest, I find this refactoring confusing. As things stand on
HEAD, fe-secure.c depends on the contents of fe-secure-openssl.c, and
the dependency goes only in one direction.  With your patch, you also
make fe-secure-openssl.c call things within fe-secure.c.  It seems to me
that a cleaner split would be to introduce a common file for all the
low-level routines like the two ones you are introducing here, say
fe-secure-common.c. aND pq_verify_peer_name_matches_certificate_name and
pq_verify_peer_name_matches_certificate should be moved to that.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Bruce Momjian]

On Wed, Jan 17, 2018 at 10:02:35PM -0500, Tom Lane wrote:
> That is a really good point.  For precedent, note that darn near nobody
> seems to know whether their psql contains readline or libedit.  If we
> force the issue by giving the settings different names, then they'll be
> forced to figure out which SSL implementation they have.
> 
> On the other hand, you could argue that there are more user-friendly
> ways to expose that information than demanding that users play twenty
> questions with their config files.  I'd at least want us to recognize
> when somebody tries to set "openssl_foo" in a gnutls implementation,
> and respond with "you need to twiddle the gnutls_xxx variables instead"
> rather than just "unrecognized configuration parameter".  Maybe that'd
> be good enough, though.

To open another can of worms, are we ever going to rename "ssl"
parameters to "tls" since TLS is the protocol used by all modern secure
communication libraries.  SSL was deprecated in 2015:

https://www.globalsign.com/en/blog/ssl-vs-tls-difference/
Both SSL 2.0 and 3.0 have been deprecated by the IETF (in 2011
and 2015, respectively).

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/25/18 09:07, Peter Eisentraut wrote:
> On 1/19/18 13:43, Peter Eisentraut wrote:
>> Comparing the existing {be,fe}-secure-openssl.c with the proposed
>> {be,fe}-secure-gnutls.c, and with half an eye on the previously proposed
>> Apple Secure Transport implementation, I have identified a few more
>> areas of refactoring that should be done in order to avoid excessive
>> copy-and-pasting in the new implementations:
> 
> And here is another place that needs cleaning up, where the OpenSSL API
> was used directly.

And here is another one.  The last one for now I think.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From ab0bd8f655a5c75c8040b462a9d2c0111fbf323a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 27 Jan 2018 13:47:52 -0500
Subject: [PATCH] Refactor client-side SSL certificate checking code

Separate the parts specific to the SSL library from the general logic.

The previous code structure was

open_client_SSL()
calls verify_peer_name_matches_certificate()
calls verify_peer_name_matches_certificate_name()
calls wildcard_certificate_match()

and was completely in fe-secure-openssl.c.  The new structure is

open_client_SSL() [openssl]
calls pq_verify_peer_name_matches_certificate() [generic]
calls pgtls_verify_peer_name_matches_certificate_guts() [openssl]
calls openssl_verify_peer_name_matches_certificate_name() [openssl]
calls pq_verify_peer_name_matches_certificate_name() [generic]
calls wildcard_certificate_match() [generic]
---
 src/interfaces/libpq/fe-secure-openssl.c | 209 ---
 src/interfaces/libpq/fe-secure.c | 185 ++-
 src/interfaces/libpq/libpq-int.h |  20 +++
 3 files changed, 228 insertions(+), 186 deletions(-)

diff --git a/src/interfaces/libpq/fe-secure-openssl.c 
b/src/interfaces/libpq/fe-secure-openssl.c
index 9ab317320a..228ea5897a 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -60,9 +60,8 @@
 #endif
 #include 
 
-static bool verify_peer_name_matches_certificate(PGconn *);
 static int verify_cb(int ok, X509_STORE_CTX *ctx);
-static int verify_peer_name_matches_certificate_name(PGconn *conn,
+static int openssl_verify_peer_name_matches_certificate_name(PGconn *conn,

  ASN1_STRING *name,

  char **store_name);
 static void destroy_ssl_system(void);
@@ -492,76 +491,16 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
 
 
 /*
- * Check if a wildcard certificate matches the server hostname.
- *
- * The rule for this is:
- * 1. We only match the '*' character as wildcard
- * 2. We match only wildcards at the start of the string
- * 3. The '*' character does *not* match '.', meaning that we match only
- *a single pathname component.
- * 4. We don't support more than one '*' in a single pattern.
- *
- * This is roughly in line with RFC2818, but contrary to what most browsers
- * appear to be implementing (point 3 being the difference)
- *
- * Matching is always case-insensitive, since DNS is case insensitive.
- */
-static int
-wildcard_certificate_match(const char *pattern, const char *string)
-{
-   int lenpat = strlen(pattern);
-   int lenstr = strlen(string);
-
-   /* If we don't start with a wildcard, it's not a match (rule 1 & 2) */
-   if (lenpat < 3 ||
-   pattern[0] != '*' ||
-   pattern[1] != '.')
-   return 0;
-
-   if (lenpat > lenstr)
-   /* If pattern is longer than the string, we can never match */
-   return 0;
-
-   if (pg_strcasecmp(pattern + 1, string + lenstr - lenpat + 1) != 0)
-
-   /*
-* If string does not end in pattern (minus the wildcard), we 
don't
-* match
-*/
-   return 0;
-
-   if (strchr(string, '.') < string + lenstr - lenpat)
-
-   /*
-* If there is a dot left of where the pattern started to 
match, we
-* don't match (rule 3)
-*/
-   return 0;
-
-   /* String ended with pattern, and didn't have a dot before, so we match 
*/
-   return 1;
-}
-
-/*
- * Check if a name from a server's certificate matches the peer's hostname.
- *
- * Returns 1 if the name matches, and 0 if it does not. On error, returns
- * -1, and sets the libpq error message.
- *
- * The name extracted from the certificate is returned in *store_name. The
- * caller is responsible for freeing it.
+ * OpenSSL-specific wrapper around
+ * pq_verify_peer_name_matches_certificate_name(), converting the ASN1_STRING
+ * into a plain C string.
  */
 static int
-verify_peer_name_matches_certificate_name(PGconn 

Re: [HACKERS] GnuTLS support

[Daniel Gustafsson]

> On 26 Jan 2018, at 02:10, Michael Paquier  wrote:
> On Fri, Jan 26, 2018 at 12:27:16AM +0100, Daniel Gustafsson wrote:

>> While only tangentially related to the issue this patch solves, converting
>> be_tls_get_peerdn_name() to return const char * seems reasonable too to keep
>> the API consistent.
> 
> Why? This is not used for error message generation yet. We could always
> change the API as needed later on.

Mainly to keep the be_tls_* interface consistent in how the routines return
data.  I don’t have any strong opinions though, it just seemed like a good time
to do it.

cheers ./daniel

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/25/18 20:10, Michael Paquier wrote:
> Peter, could you change ssl_version() and ssl_cipher() in sslinfo at the
> same time please? I think that those should use the generic backend-side
> APIs as well. sslinfo depends heavily on OpenSSL, OK, but if possible
> getting this code more generic will help users of sslinfo to get
> something partially working with other SSL implementations natively.

sslinfo is currently entirely dependent on OpenSSL, so I don't think
it's useful to throw in one or two isolated API changes.

I'm thinking maybe we should get rid of sslinfo and fold everything into
pg_stat_ssl.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Fri, Jan 26, 2018 at 12:27:16AM +0100, Daniel Gustafsson wrote:
>> On 25 Jan 2018, at 15:07, Peter Eisentraut 
>>  wrote:
>> 
>> On 1/19/18 13:43, Peter Eisentraut wrote:
>>> Comparing the existing {be,fe}-secure-openssl.c with the proposed
>>> {be,fe}-secure-gnutls.c, and with half an eye on the previously proposed
>>> Apple Secure Transport implementation, I have identified a few more
>>> areas of refactoring that should be done in order to avoid excessive
>>> copy-and-pasting in the new implementations:
>> 
>> And here is another place that needs cleaning up, where the OpenSSL API
>> was used directly.
> 
> +1 on these cleanups.

Peter, could you change ssl_version() and ssl_cipher() in sslinfo at the
same time please? I think that those should use the generic backend-side
APIs as well. sslinfo depends heavily on OpenSSL, OK, but if possible
getting this code more generic will help users of sslinfo to get
something partially working with other SSL implementations natively.

> Regarding this hunk:
> 
>  extern int   be_tls_get_cipher_bits(Port *port);
>  extern bool be_tls_get_compression(Port *port);
> -extern void be_tls_get_version(Port *port, char *ptr, size_t len);
> -extern void be_tls_get_cipher(Port *port, char *ptr, size_t len);
> +extern const char *be_tls_get_version(Port *port);
> +extern const char *be_tls_get_cipher(Port *port);
>  extern void be_tls_get_peerdn_name(Port *port, char *ptr, size_t len);
> 
> While only tangentially related to the issue this patch solves, converting
> be_tls_get_peerdn_name() to return const char * seems reasonable too to keep
> the API consistent.

Why? This is not used for error message generation yet. We could always
change the API as needed later on.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Fri, Jan 19, 2018 at 01:55:30PM -0500, Robert Haas wrote:
> On Wed, Jan 17, 2018 at 10:02 PM, Tom Lane  wrote:
>> Also, this isn't really a good argument against using uniform names
>> for parameters that every implementation is certain to have, like
>> ssl_key_file.
> 
> Even then, it's not that hard to imagine minor variations between what
> different implementations will accept.  The most obvious difference is
> probably that they might expect different file formats, but it's also
> possible that a Windows-specific implementation might allow omitting
> the file extension while some other implementation does not, for
> example.  I agree that it would probably be fairly low-risk to use one
> parameter for the key file for every implementation, but I suggest
> that it would be cleaner and less prone to confusion if we enforce a
> full separation of parameters.  That also spares us having to make a
> judgement call about which parameters have semantics close enough that
> we need not separate them.

Having to debate about the most correct default values for one common
parameter depending on N SSL implemententations will likely prove at the
end that the most correct answer is just to split all parameters from
the start. Some implementations are platform-specific, the format file
is one thing that matters. There are also other things like ssl_ciphers
which could depend on what kind of cipher types an SSL implementation is
able to support... Another thing is that we cannot be 100% sure that one
parameter can be set to say GUC_SIGHUP for an SSL implementation and
needs to have  other modes with another implementations. Of course this
can be solved if some ifdefs, but splitting brings clarity.

At the end, I agree with the position of Robert to split everything and
rename the existing ssl_* parameters to openssl_* if v11 gains a new SSL
implementation.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Daniel Gustafsson]

> On 25 Jan 2018, at 15:07, Peter Eisentraut  
> wrote:
> 
> On 1/19/18 13:43, Peter Eisentraut wrote:
>> Comparing the existing {be,fe}-secure-openssl.c with the proposed
>> {be,fe}-secure-gnutls.c, and with half an eye on the previously proposed
>> Apple Secure Transport implementation, I have identified a few more
>> areas of refactoring that should be done in order to avoid excessive
>> copy-and-pasting in the new implementations:
> 
> And here is another place that needs cleaning up, where the OpenSSL API
> was used directly.

+1 on these cleanups.

Regarding this hunk:

 extern int be_tls_get_cipher_bits(Port *port);
 extern bool be_tls_get_compression(Port *port);
-extern void be_tls_get_version(Port *port, char *ptr, size_t len);
-extern void be_tls_get_cipher(Port *port, char *ptr, size_t len);
+extern const char *be_tls_get_version(Port *port);
+extern const char *be_tls_get_cipher(Port *port);
 extern void be_tls_get_peerdn_name(Port *port, char *ptr, size_t len);

While only tangentially related to the issue this patch solves, converting
be_tls_get_peerdn_name() to return const char * seems reasonable too to keep
the API consistent.

cheers ./daniel

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

Comparing the existing {be,fe}-secure-openssl.c with the proposed
{be,fe}-secure-gnutls.c, and with half an eye on the previously proposed
Apple Secure Transport implementation, I have identified a few more
areas of refactoring that should be done in order to avoid excessive
copy-and-pasting in the new implementations:

0001-Add-installcheck-support-to-more-test-suites.patch

This will help with interoperability testing, because you can then
create an installation with mixed SSL implementations and run the test
suite against it.

0002-Split-out-documentation-of-SSL-parameters-into-their.patch

Prepares and cleans up the documentation a bit before the addition of
new things, as discussed elsewhere.

0003-Move-EDH-support-to-common-files.patch

To avoid copy-and-paste, and also because the EDH explanation doesn't
really belong in a file header comment.  Maybe the whole thing is known
well enough nowadays that we can just remove the explanation.

0004-Move-SSL-API-comments-to-header-files.patch
0005-Extract-common-bits-from-OpenSSL-implementation.patch

Move copy-and-paste avoidance.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 5b5f568bf05b2424ceb522c0a6d2f94487fea3b7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 19 Jan 2018 12:17:35 -0500
Subject: [PATCH 1/5] Add installcheck support to more test suites

---
 src/test/authentication/Makefile | 3 +++
 src/test/ldap/Makefile   | 3 +++
 src/test/recovery/Makefile   | 3 +++
 src/test/ssl/Makefile| 3 +++
 src/test/subscription/Makefile   | 3 +++
 5 files changed, 15 insertions(+)

diff --git a/src/test/authentication/Makefile b/src/test/authentication/Makefile
index a435b13057..218452ec76 100644
--- a/src/test/authentication/Makefile
+++ b/src/test/authentication/Makefile
@@ -16,5 +16,8 @@ include $(top_builddir)/src/Makefile.global
 check:
$(prove_check)
 
+installcheck:
+   $(prove_installcheck)
+
 clean distclean maintainer-clean:
rm -rf tmp_check
diff --git a/src/test/ldap/Makefile b/src/test/ldap/Makefile
index 50e3c17e95..fef5742b82 100644
--- a/src/test/ldap/Makefile
+++ b/src/test/ldap/Makefile
@@ -16,5 +16,8 @@ include $(top_builddir)/src/Makefile.global
 check:
$(prove_check)
 
+installcheck:
+   $(prove_installcheck)
+
 clean distclean maintainer-clean:
rm -rf tmp_check
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index aecf37d89a..daf79a0b1f 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -18,5 +18,8 @@ include $(top_builddir)/src/Makefile.global
 check:
$(prove_check)
 
+installcheck:
+   $(prove_installcheck)
+
 clean distclean maintainer-clean:
rm -rf tmp_check
diff --git a/src/test/ssl/Makefile b/src/test/ssl/Makefile
index 4886e901d0..4e9095529a 100644
--- a/src/test/ssl/Makefile
+++ b/src/test/ssl/Makefile
@@ -132,3 +132,6 @@ clean distclean maintainer-clean:
 
 check:
$(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/test/subscription/Makefile b/src/test/subscription/Makefile
index 25c48e470d..0f3d2098ad 100644
--- a/src/test/subscription/Makefile
+++ b/src/test/subscription/Makefile
@@ -18,5 +18,8 @@ EXTRA_INSTALL = contrib/hstore
 check:
$(prove_check)
 
+installcheck:
+   $(prove_installcheck)
+
 clean distclean maintainer-clean:
rm -rf tmp_check

base-commit: 4e54dd2e0a750352ce2a5c45d1cc9183e887eec3
-- 
2.15.1

From d9160fc3cfa0e9725c857469fc1f156452a77922 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 18 Jan 2018 19:12:05 -0500
Subject: [PATCH 2/5] Split out documentation of SSL parameters into their own
 section

Split the "Authentication and Security" section into two separate
sections "Authentication" and "SSL".  The latter part has gotten much
longer over time, and doesn't primarily have to do with authentication.
---
 doc/src/sgml/config.sgml   | 233 +
 src/backend/utils/misc/guc.c   |  38 +++
 src/include/utils/guc_tables.h |   3 +-
 3 files changed, 143 insertions(+), 131 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 37a61a13c8..907bf1471d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -924,8 +924,9 @@ Connection Settings
 
  
  
- 
- Security and Authentication
+
+ 
+ Authentication
 
  
  
@@ -950,6 +951,123 @@ Security and Authentication
   
  
 
+ 
+  password_encryption (enum)
+  
+   password_encryption configuration 
parameter
+  
+  
+  
+   
+When a password is specified in  or
+, this parameter determines the 
algorithm
+to use to encrypt the password. The default value is 
md5,
+which stores the password as an MD5 hash (on is also
+accepted, as alias for md5). 

Re: [HACKERS] GnuTLS support

[Robert Haas]

On Wed, Jan 17, 2018 at 6:48 PM, Tomas Vondra
 wrote:
> What would be much worse is if a particular GUC did not have a matching
> concept in the library. Say if an SSL library did not have a concept of
> priority strings and instead used some other concept affecting cipher
> suite choice (not sure how that would like). That would make our GUC
> useless or confusing, possibly forcing us to translate the strings in
> some strange way.

I think that's pretty likely to happen, which is why I favor renaming
all of the SSL stuff to openssl_* and then having gnutls_* and
similarly for other implementations.  It's not going to be fun to
document that there's this single GUC which, depending on some
compiler flag which you don't know anything about, takes a
differently-formatted value and maybe does different stuff.  That's
what we'll end up with even for absolutely simple things like
ssl_ciphers, because it's extremely unlikely that every SSL library on
earth uses the same format that OpenSSL does.  Worse yet, users are
not going to intrinsically know which SSL implementation was compiled
into the server they have.

Now, if we can tell them something like this, then things will be better:

PostgreSQL can be compiled against any of several SSL implementations.
Currently, PostgreSQL supports OpenSSL, GnuTLS, AwesomeSSL, and
TLSBlah.  Each of these implementations is controlled by a different
group of settings; only settings for the SSL implementation against
which the server is compiled will exist.  For OpenSSL, the controlling
settings are openssl_thingy, openssl_thang, and openssl_thunk.  For
GnuTLS, the controlling settings are  etc. etc.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] GnuTLS support

[Tomas Vondra]

On 01/17/2018 11:14 PM, Peter Eisentraut wrote:
> On 1/17/18 14:05, Tom Lane wrote:
>> Although these corner cases are starting to make me feel like
>> changing my original vote. Maybe we should forget the prefixes, in
>> particular renaming gnutls_priorities to ssl_priorities, and just
>> accept the need to document some parameters as only relevant to
>> some implementations.
> 
> We could go the route of normalizing all implementation-specific 
> settings to some set of atomic concepts and create separate settings
> for those, and then map them back to the actual APIs in code.
> 

I think it's reasonable to expect that other SSL libraries implementing
the same crypto algorithms will have APIs with the same (or very
similar) concepts and parameters, so +1 to doing this.

> So we could take ssl_ciphers, ssl_prefer_server_ciphers,
> ssl_ecdh_curve and assemble internally something that we can pass to 
> gnutls_priority_init().
> 

I think the question here is how far we want to go. For example, the
various libraries are likely using different formats for the lists of
preferred cipher suites. Are we fine with that? The meaning of the GUC
will be the same, but the format (or cipher suite naming) will depend on
the library.

(I don't think we want to get into the business of inventing our custom
universal syntax, nor accepting OpenSSL syntax/naming as the right one
and translating everything to/from it.)

> But I think it would be more helpful in practice if the naming of
> the implementation-specific settings match with something you can
> look up in the documentation of that implementation. "GnuTLS priority
> string" is easy to look up and well documented. If instead we chop it
> up into something that is more like the OpenSSL settings, I think we
> are not helping anyone.
> 

Meh. As long as there's something like "priority string" for that
particular library, I think we're fine. We can just say "This is a
priority string, the format depends on the SSL library used," and
perhaps even link to additional docs for each supported library.

What would be much worse is if a particular GUC did not have a matching
concept in the library. Say if an SSL library did not have a concept of
priority strings and instead used some other concept affecting cipher
suite choice (not sure how that would like). That would make our GUC
useless or confusing, possibly forcing us to translate the strings in
some strange way.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/17/18 14:05, Tom Lane wrote:
> Although these corner cases are starting to make me feel like changing
> my original vote.  Maybe we should forget the prefixes, in particular
> renaming gnutls_priorities to ssl_priorities, and just accept the need
> to document some parameters as only relevant to some implementations.

We could go the route of normalizing all implementation-specific
settings to some set of atomic concepts and create separate settings for
those, and then map them back to the actual APIs in code.

So we could take ssl_ciphers, ssl_prefer_server_ciphers, ssl_ecdh_curve
and assemble internally something that we can pass to
gnutls_priority_init().

But I think it would be more helpful in practice if the naming of the
implementation-specific settings match with something you can look up in
the documentation of that implementation.  "GnuTLS priority string" is
easy to look up and well documented.  If instead we chop it up into
something that is more like the OpenSSL settings, I think we are not
helping anyone.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/17/18 13:18, Tom Lane wrote:
>> The proposed GnuTLS patch does make use of ssl_dh_params_file.
> 
> Right, but what happens if say macTLS doesn't?

The previously proposed patch for that also makes use of ssl_dh_params_file.

So while we can't guarantee that this will be the case for all TLS
implementations ever, this is a pretty good indicator to me that it is
an implementation-independent concept.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Tom Lane]

Magnus Hagander  writes:
> On Wed, Jan 17, 2018 at 8:18 PM, Tom Lane  wrote:
>> What I don't want to end up with is an unholy mixture of both approaches.
>> Therefore, if we are going to use method #2, we must be certain that
>> the basic "ssl_" parameters are supported by every implementation,
>> to the point where we'd reject an implementation that didn't have one.
>> I can see that we'd reject an implementation lacking CRL support
>> for instance, but I'm less clear that lack of configurable DH parameters
>> should be a disqualifying feature omission.  I'm prepared to be educated
>> either way, but that's the core question here.

> So in this particular case, does it mean that to do #2, we sould actually
> have an openssl_dh_params_file and a gnutls_dh_params_file, but only one at
> any given time?

That's where I think we end up if we don't want to assume that every
implementation has dh_params_file support.

> Thinking on that there is also the case of file formats. What if one
> provider takes a cert file, but not in the same format -- should that still
> be ssl_cert_file, or should it be a different parameter name? Given that
> you can't use it to point to the same file.

Meh --- as long as any given build supports only one SSL implementation,
I think insisting on that would be overly pedantic.  Seems like as long
as what you put into the config file is the same, it's okay to consider
a given parameter to be shared by two different implementations.

Although these corner cases are starting to make me feel like changing
my original vote.  Maybe we should forget the prefixes, in particular
renaming gnutls_priorities to ssl_priorities, and just accept the need
to document some parameters as only relevant to some implementations.
That certainly requires less betting on our ability to predict the
future.

regards, tom lane

Re: [HACKERS] GnuTLS support

[Magnus Hagander]

On Wed, Jan 17, 2018 at 8:18 PM, Tom Lane  wrote:

> Peter Eisentraut  writes:
> > On 1/17/18 12:39, Tom Lane wrote:
> >> I don't know too much about the internals here, so looking at your
> >> list, I wonder whether "ssl_dh_params_file" ought to be treated as
> >> implementation-specific too.  The other four files seem essential
> >> to any feature-complete implementation, but is that one?
>
> > The proposed GnuTLS patch does make use of ssl_dh_params_file.
>
> Right, but what happens if say macTLS doesn't?
>
> There are basically two approaches we can take here:
>
> 1. All the relevant parameters are named "ssl_something", and we have
> to flag in the documentation any that are supported only by some
> implementations.
>
> 2. Parameters that might be supported only by some implementations
> are named with implementation-specific names, and we have to accept
> that there might sometimes be both "foossl_xyz" and "barssl_xyz".
>
> What I don't want to end up with is an unholy mixture of both approaches.
> Therefore, if we are going to use method #2, we must be certain that
> the basic "ssl_" parameters are supported by every implementation,
> to the point where we'd reject an implementation that didn't have one.
> I can see that we'd reject an implementation lacking CRL support
> for instance, but I'm less clear that lack of configurable DH parameters
> should be a disqualifying feature omission.  I'm prepared to be educated
> either way, but that's the core question here.
>

So in this particular case, does it mean that to do #2, we sould actually
have an openssl_dh_params_file and a gnutls_dh_params_file, but only one at
any given time?

Thinking on that there is also the case of file formats. What if one
provider takes a cert file, but not in the same format -- should that still
be ssl_cert_file, or should it be a different parameter name? Given that
you can't use it to point to the same file.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/17/18 12:39, Tom Lane wrote:
> I don't know too much about the internals here, so looking at your
> list, I wonder whether "ssl_dh_params_file" ought to be treated as
> implementation-specific too.  The other four files seem essential
> to any feature-complete implementation, but is that one?

The proposed GnuTLS patch does make use of ssl_dh_params_file.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Tom Lane]

Peter Eisentraut  writes:
> Question for the group:  We currently have a number of config settings
> named ssl_*.  Some of these are specific to OpenSSL, some are not, namely:

> # general
> ssl
> ssl_dh_params_file
> ssl_cert_file
> ssl_key_file
> ssl_ca_file
> ssl_crl_file

> # OpenSSL
> ssl_ciphers
> ssl_prefer_server_ciphers
> ssl_ecdh_curve

> # GnuTLS (proposed)
> gnutls_priorities
> (effectively a combination of ssl_ciphers and ssl_prefer_server_ciphers)

> Should we rename the OpenSSL-specific settings to openssl_*?

> It think it would be better for clarity, and they are not set very
> commonly, so the user impact would be low.

Yeah, I think only the "general" parameters would be set by very
many people.  +1 for renaming the OpenSSL-only parameters.

I don't know too much about the internals here, so looking at your
list, I wonder whether "ssl_dh_params_file" ought to be treated as
implementation-specific too.  The other four files seem essential
to any feature-complete implementation, but is that one?

regards, tom lane

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/2/18 10:35, Peter Eisentraut wrote:
> On 11/26/17 20:05, Andreas Karlsson wrote:
>> I have now implemented this in the attached patch (plus added support 
>> for channel binding and rebased it) but I ran into one issue which I 
>> have not yet solved. The script for the windows version takes the 
>> --with-openssl= switch so that cannot just be translated to a 
>> single --with-ssl switch. Should to have both --with-openssl and 
>> --with-gnutls or --with-ssl=(openssl|gnutls) and --with-ssl-path=? 
>> I also do not know the Windows build code very well (or really at all).
> 
> This patch appears to work well.

Seeing that Andres is apparently currently not available, I have started
to dig through this patch myself and made some adjustments.

Question for the group:  We currently have a number of config settings
named ssl_*.  Some of these are specific to OpenSSL, some are not, namely:

# general
ssl
ssl_dh_params_file
ssl_cert_file
ssl_key_file
ssl_ca_file
ssl_crl_file

# OpenSSL
ssl_ciphers
ssl_prefer_server_ciphers
ssl_ecdh_curve

# GnuTLS (proposed)
gnutls_priorities
(effectively a combination of ssl_ciphers and ssl_prefer_server_ciphers)

Should we rename the OpenSSL-specific settings to openssl_*?

It think it would be better for clarity, and they are not set very
commonly, so the user impact would be low.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/3/18 04:59, Michael Paquier wrote:
> On Tue, Jan 02, 2018 at 10:54:29PM -0500, Peter Eisentraut wrote:
>> I think the solution is that we need to require that all SSL server-side
>> implementations support all channel binding types.
> 
> That could be a stop for Windows and macos SSL implementations then.

I'm surprised by that.  I thought tls-server-endpoint is basically
always possible to implement, because all you need is to obtain the peer
certificate and hash it.  It seems to me that any SSL implementation
should be able to do that.

> - Have the server publish the -PLUS mechanism only if an SSL
> implementation supports tls-unique.

But then a conforming client will never pick -PLUS.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Tue, Jan 02, 2018 at 10:54:29PM -0500, Peter Eisentraut wrote:
> I think the solution is that we need to require that all SSL server-side
> implementations support all channel binding types.

That could be a stop for Windows and macos SSL implementations then. I
would think that we would benefit by being softer here, say with the
following guidelines:
- Have the server publish the -PLUS mechanism only if an SSL
implementation supports tls-unique.
- The RFC makes tls-unique mandatory, so requiring only tls-unique to be
present looks like a good default for me.

It is true that JDBC makes this whole thing harder, tls-server-end-point
patch has been done mainly for them. Even for OpenSSL, I had to dig
within their code tree to figure out the APIs to use to get the hash
algorithm. I would not be surprised that the same investigation is
necessary for gnutls.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 1/2/18 18:35, Michael Paquier wrote:
> On Tue, Jan 02, 2018 at 10:35:16AM -0500, Peter Eisentraut wrote:
>> I see a potential problem with the SCRAM channel binding support.
>> GnuTLS will not support tls-server-endpoint, so we'll need to check what
>> happens when a client requests that.  (That's not the problem of this
>> patch, however.)
> 
> Doesn't it depend on the first patch merged into HEAD? At the end we'll
> need to make be_tls_get_certificate_hash() generate an ereport() with
> ERRCODE_FEATURE_NOT_SUPPORTED and have pgtls_get_peer_certificate_hash()
> return NULL with conn->errorMessage properly filled.

The problem I see is that SCRAM doesn't really support the negotiation
of the channel binding type.  The client picks one, and the server
either accepts it or the whole thing dies.

One scenario is that if GnuTLS goes in, it's quite plausible that the
PG11 packages for Debian and Ubuntu will use it by default.  But if it
doesn't support tls-server-endpoint, then a JDBC client (assuming
channel binding support is added) can't connect to such a server with
SCRAM authentication over SSL (which we hope will be a popular
configuration), unless they manually disable channel binding altogether
using the new scramchannelbinding connection option.  That would be a
very poor experience.

I think the solution is that we need to require that all SSL server-side
implementations support all channel binding types.

I think that can probably be done with GnuTLS, but we'd need to check
that out a bit.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Tue, Jan 02, 2018 at 10:35:16AM -0500, Peter Eisentraut wrote:
> I see a potential problem with the SCRAM channel binding support.
> GnuTLS will not support tls-server-endpoint, so we'll need to check what
> happens when a client requests that.  (That's not the problem of this
> patch, however.)

Doesn't it depend on the first patch merged into HEAD? At the end we'll
need to make be_tls_get_certificate_hash() generate an ereport() with
ERRCODE_FEATURE_NOT_SUPPORTED and have pgtls_get_peer_certificate_hash()
return NULL with conn->errorMessage properly filled.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 11/26/17 20:05, Andreas Karlsson wrote:
> I have now implemented this in the attached patch (plus added support 
> for channel binding and rebased it) but I ran into one issue which I 
> have not yet solved. The script for the windows version takes the 
> --with-openssl= switch so that cannot just be translated to a 
> single --with-ssl switch. Should to have both --with-openssl and 
> --with-gnutls or --with-ssl=(openssl|gnutls) and --with-ssl-path=? 
> I also do not know the Windows build code very well (or really at all).

This patch appears to work well.

As I had mentioned previously, I'm not fond of changing the existing
configure flags, and given the above issue, I'd just leave everything as
is and add --with-gnutls.

The patch contains a purported GUC variable gnutls_priority, but it is
not documented or used anywhere.

There are some test cases that are marked to be skipped.  We should
document why that is.

I see a potential problem with the SCRAM channel binding support.
GnuTLS will not support tls-server-endpoint, so we'll need to check what
happens when a client requests that.  (That's not the problem of this
patch, however.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Andreas Karlsson]

On 11/28/2017 04:19 PM, Peter Eisentraut wrote:

On 11/19/17 20:56, Michael Paquier wrote:

  --with-ssl=(openssl|gnutls)


I'm not sure whether this is a great improvement.  Why upset existing
build and packaging scripts?  The usual options style is
--with-nameoflib.  We can have separate options and error if conflicting
combinations are specified.


We already have a precedent in --with-uuid=LIB which has the backwards 
compatibility alias --with-ossp-uuid, so I think adding --with-ssl=LIB 
while keeping --with-openssl as an alias is consistent with that. I also 
think the code and the interface ended up pretty clean when I added 
--with-ssl in my latest patch.


The only issue I see with --with-ssl is Window's config.pl which might 
end up a bit ugly to support '$config->{openssl} = "path";' as a legacy 
alias.


Andreas

Re: [HACKERS] GnuTLS support

[Peter Eisentraut]

On 11/19/17 20:56, Michael Paquier wrote:
>> If I get it right we ignore gnutls and use openssl (as it's the first
>> checked in #ifdefs). Shouldn't we enforce in configure that only one TLS
>> implementation is enabled? Either by some elaborate check, or by
>> switching to something like
>>
>>  --with-ssl=(openssl|gnutls)
> WIth potential patches coming to use macos' SSL implementation or
> Windows channel, there should really be only one implementation
> available at compile time. That's more simple as a first step as well.
> So +1 for the --with-ssl switch.

I'm not sure whether this is a great improvement.  Why upset existing
build and packaging scripts?  The usual options style is
--with-nameoflib.  We can have separate options and error if conflicting
combinations are specified.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Nov 27, 2017 at 2:37 PM, Michael Paquier
 wrote:
> On Mon, Nov 27, 2017 at 10:28 AM, Andreas Karlsson  wrote:
>> Hm, after reading more of our MSVC code it seems like building with MSVC
>> does not really use switch, people rather have to create a config.pl. Is
>> that correct? The MSVC scripts also seems to only support uuid-ossp which it
>> just calls $config->{uuid}.
>>
>> If so we could either have:
>>
>> $config->{ssl} = "openssl";
>> $config->{sslpath} = "/path/to/openssl";
>
> Using this one may actually finish by being cleaner as there is no
> need to cross-check for both options defined.

Andreas, as I can see that you are still actively working on this
patch, I am bumping it to next CF.
-- 
Michael

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Nov 27, 2017 at 10:28 AM, Andreas Karlsson  wrote:
> Hm, after reading more of our MSVC code it seems like building with MSVC
> does not really use switch, people rather have to create a config.pl. Is
> that correct? The MSVC scripts also seems to only support uuid-ossp which it
> just calls $config->{uuid}.
>
> If so we could either have:
>
> $config->{ssl} = "openssl";
> $config->{sslpath} = "/path/to/openssl";

Using this one may actually finish by being cleaner as there is no
need to cross-check for both options defined.
-- 
Michael

Re: [HACKERS] GnuTLS support

[Andreas Karlsson]

On 11/27/2017 02:20 AM, Michael Paquier wrote:

On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson  wrote:

The script for the windows version takes the
--with-openssl= switch so that cannot just be translated to a single
--with-ssl switch. Should to have both --with-openssl and --with-gnutls or
--with-ssl=(openssl|gnutls) and --with-ssl-path=? I also do not know
the Windows build code very well (or really at all).


By default --with-openssl does not take a path with ./configure. When
using gnutls, do the name of the libraries and the binaries generated
change compared to openssl? If yes, and I guess that it is the case,
you will need to be able to make the difference between gnutls and
openssl anyway as the set of perl scripts in src/tools/msvc need to
make the difference with deliverables at name-level. I would be
personally fine with just listing gnutls in the list of options and
comment it as --with-ssl=, and change the openssl comment to
match that.


Hm, after reading more of our MSVC code it seems like building with MSVC 
does not really use switch, people rather have to create a config.pl. Is 
that correct? The MSVC scripts also seems to only support uuid-ossp 
which it just calls $config->{uuid}.


If so we could either have:

$config->{ssl} = "openssl";
$config->{sslpath} = "/path/to/openssl";

or

$config->{ssl} = "openssl";
$config->{openssl} = "/path/to/openssl";

or

$config->{openssl} = "/path/to/openssl";
vs
$config->{gnutls} = "/path/to/gnutls";

Andreas

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson  wrote:
> The script for the windows version takes the
> --with-openssl= switch so that cannot just be translated to a single
> --with-ssl switch. Should to have both --with-openssl and --with-gnutls or
> --with-ssl=(openssl|gnutls) and --with-ssl-path=? I also do not know
> the Windows build code very well (or really at all).

By default --with-openssl does not take a path with ./configure. When
using gnutls, do the name of the libraries and the binaries generated
change compared to openssl? If yes, and I guess that it is the case,
you will need to be able to make the difference between gnutls and
openssl anyway as the set of perl scripts in src/tools/msvc need to
make the difference with deliverables at name-level. I would be
personally fine with just listing gnutls in the list of options and
comment it as --with-ssl=, and change the openssl comment to
match that.
-- 
Michael

Re: [HACKERS] GnuTLS support

[Tomas Vondra]

Hi,

On 11/02/2017 11:33 PM, Andreas Karlsson wrote:
> On 09/18/2017 07:04 PM, Jeff Janes wrote:> You fixed the first issue,
> but I still get the second one:
>>
>> be-secure-gnutls.c: In function 'get_peer_certificate':
>> be-secure-gnutls.c:667: error: 'GNUTLS_X509_CRT_LIST_SORT' undeclared
>> (first use in this function)
>> be-secure-gnutls.c:667: error: (Each undeclared identifier is reported
>> only once
>> be-secure-gnutls.c:667: error: for each function it appears in.)
> 
> Thanks again for testing the code. I have now rebased the patch and
> fixed the second issue. I tested that it works on CentOS 6.
> 
> Work which remains:
> 
> - sslinfo
> - pgcrypto
> - Documentation
> - Decide if what I did with the config is a good idea
> 

I don't want to be the annoying guy, but this patch no longer applies
due to 642bafa0c5f9f08d106a14f31429e0e0c718b603 touching the tests :-(

BTW regarding the config, I believe you've kept is static (no hiding of
sections based on configure parameters), and you've separated the
openssl and gnutls options, right? Seems fine to me. The one thing is
that it was proposed to rename the openssl-specific options to start
with openssl_ instead of ssl_.

One question though. What happens when you do

  ./configure --with-openssl --with-gnutls

If I get it right we ignore gnutls and use openssl (as it's the first
checked in #ifdefs). Shouldn't we enforce in configure that only one TLS
implementation is enabled? Either by some elaborate check, or by
switching to something like

 --with-ssl=(openssl|gnutls)


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services