Thread: [HACKERS] GnuTLS support

Hi,

I have seen discussions from time to time about OpenSSL and its 
licensing issues so I decided to see how much work it would be to add 
support for another TLS library, and  I went with GnuTLS since it is the 
library I know best after OpenSSL and it is also a reasonably popular 
library.

Attached is a work in progress patch which implements the basics. I send 
it the list because I want feedback on some design questions and to 
check with the community if this is a feature we want.

= What is implemented

- Backend
- Frontend
- Diffie-Hellmann support
- Using GnuTLS for secure random numbers
- Using GnuTLS for sha2

= Work left to do

- Add GnuTLS support to sslinfo
- Add GnuTLS support to pgcrypto
- Support for GnuTLS's version of engines
- Test code with older versions of GnuTLS
- Update documentation
- Add support for all postgresql.conf options (see design question)
- Fix two failing tests (see design question)

= Design questions

== GnuTLS priority strings vs OpenSSL cipher lists

GnuTLS uses a different format for specifying ciphers. Instead of 
setting ciphers, protocol versions, and ECDH curves separately GnuTLS 
instead uses a single priority string[1].

For example the default settings of PostgreSQL (which include disabling 
SSLv3)

ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
ssl_prefer_server_ciphers = on
ssl_ecdh_curve = 'prime256v1'

is represented with a string similar to

SECURE128:+3DES-CBC:+GROUP-SECP256R1:%SERVER_PRECEDENCE

So the two libraries have decided on different ways to specify things.

One way to solve th issue would be to just let ssl_ciphers be the 
priority string and then add %SERVER_PRECEDENCE to it if 
ssl_prefer_server_ciphers is on. Adding the ssl_ecdh_curve is trickier 
since the curves have different names in GnuTLS (e.g. prime256v1 vs 
SECP256R1) and I would rather avoid having to add such a mapping to 
PostgreSQL. Thoughts?

== Potentially OpenSSL-specific  est cases

There are currently two failing SSL tests which at least to me seems 
more like they test specific OpenSSL behaviors rather than something 
which need to be true for all SSL libraries.

The two tests:

# Try with just the server CA's cert. This fails because the root file
# must contain the whole chain up to the root CA.
note "connect with server CA cert, without root CA";
test_connect_fails("sslrootcert=ssl/server_ca.crt sslmode=verify-ca");

# A CRL belonging to a different CA is not accepted, fails
test_connect_fails(
"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca 
sslcrl=ssl/client.crl");

For the missing root CA case GnuTLS seems to be happy enough with just 
an intermediate CA and as far as I can tell this behavior is not easy to 
configure.

And for the CRL belonging to a different CA case GnuTLS can be 
configured to either just store such a non-validating CRL or to ignore 
it, but not to return an error.

Personally I think thee two tests should just be removed but maybe I am 
missing something.

Notes:

1. https://gnutls.org/manual/html_node/Priority-Strings.html

Andreas

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> I have seen discussions from time to time about OpenSSL and its licensing
> issues so I decided to see how much work it would be to add support for
> another TLS library, and  I went with GnuTLS since it is the library I know
> best after OpenSSL and it is also a reasonably popular library.

Thanks for working on this.  I think it's good for PostgreSQL to have
more options in this area.

> = Design questions
>
> == GnuTLS priority strings vs OpenSSL cipher lists
>
> GnuTLS uses a different format for specifying ciphers. Instead of setting
> ciphers, protocol versions, and ECDH curves separately GnuTLS instead uses a
> single priority string[1].
>
> For example the default settings of PostgreSQL (which include disabling
> SSLv3)
>
> ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
> ssl_prefer_server_ciphers = on
> ssl_ecdh_curve = 'prime256v1'
>
> is represented with a string similar to
>
> SECURE128:+3DES-CBC:+GROUP-SECP256R1:%SERVER_PRECEDENCE
>
> So the two libraries have decided on different ways to specify things.

I think that what this shows is that the current set of GUCs is overly
OpenSSL-centric.  We created a set of GUCs that are actually specific
to one particular implementation but named them as if they were
generic.  My idea about this would be to actually rename the existing
GUCs to start with "openssl" rather than "ssl", and then add new GUCs
as needed for other SSL implementations.

Depending on what we think is best, GUCs for an SSL implementation
other than the one against which we compiled can either not exist at
all, or can exist but be limited to a single value (e.g. "none", as we
currently do when the compile has no SSL support at all).  Also, we
could add a read-only GUC with a name like ssl_library that exposes
the name of the underlying SSL implementation - none, openssl, gnutls,
or whatever.

I think if we go the route of insisting that every SSL implementation
has to use the existing GUCs, we're just trying to shove a square peg
into a round hole, and there's no real benefit for users.  If the
string that has to be stuffed into ssl_ciphers differs based on which
library was chosen at compile time, then you can't have a uniform
default configuration for all libraries anyway.  I think it'll be
easier to explain and document this if there's separate documentation
for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
documentation section that tries to explain every implementation
separately.

> There are currently two failing SSL tests which at least to me seems more
> like they test specific OpenSSL behaviors rather than something which need
> to be true for all SSL libraries.
>
> The two tests:
>
> # Try with just the server CA's cert. This fails because the root file
> # must contain the whole chain up to the root CA.
> note "connect with server CA cert, without root CA";
> test_connect_fails("sslrootcert=ssl/server_ca.crt sslmode=verify-ca");
>
> # A CRL belonging to a different CA is not accepted, fails
> test_connect_fails(
> "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca
> sslcrl=ssl/client.crl");
>
> For the missing root CA case GnuTLS seems to be happy enough with just an
> intermediate CA and as far as I can tell this behavior is not easy to
> configure.
>
> And for the CRL belonging to a different CA case GnuTLS can be configured to
> either just store such a non-validating CRL or to ignore it, but not to
> return an error.
>
> Personally I think thee two tests should just be removed but maybe I am
> missing something.

I don't know what we should do about these issues.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>> I have seen discussions from time to time about OpenSSL and its licensing
>> issues so I decided to see how much work it would be to add support for
>> another TLS library, and  I went with GnuTLS since it is the library I know
>> best after OpenSSL and it is also a reasonably popular library.

> Thanks for working on this.  I think it's good for PostgreSQL to have
> more options in this area.

+1.  We also have a patch in the queue to support macOS' TLS library,
and I suppose that's going to be facing similar issues.  It would be
a good plan, probably, to try to push both of these to conclusion in
the same development cycle.

> I think that what this shows is that the current set of GUCs is overly
> OpenSSL-centric.  We created a set of GUCs that are actually specific
> to one particular implementation but named them as if they were
> generic.  My idea about this would be to actually rename the existing
> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
> as needed for other SSL implementations.

Works for me.

>> There are currently two failing SSL tests which at least to me seems more
>> like they test specific OpenSSL behaviors rather than something which need
>> to be true for all SSL libraries.

> I don't know what we should do about these issues.

Maybe the SSL test suite needs to be implementation-specific as well.
        regards, tom lane

> On 01 Sep 2017, at 19:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>
>>> There are currently two failing SSL tests which at least to me seems more
>>> like they test specific OpenSSL behaviors rather than something which need
>>> to be true for all SSL libraries.
>
>> I don't know what we should do about these issues.
>
> Maybe the SSL test suite needs to be implementation-specific as well.

To properly test the macOS Secure Transport support we will need to use
Keychain files on top of plain PEM files, so I think we have to.  That being
said, we should probably define a (as large possible) minimum set which applies
to all to ensure compatability between different frontends and backends.

cheers ./daniel

On Fri, Sep 1, 2017 at 1:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>>> I have seen discussions from time to time about OpenSSL and its licensing
>>> issues so I decided to see how much work it would be to add support for
>>> another TLS library, and  I went with GnuTLS since it is the library I know
>>> best after OpenSSL and it is also a reasonably popular library.
>
>> Thanks for working on this.  I think it's good for PostgreSQL to have
>> more options in this area.
>
> +1.  We also have a patch in the queue to support macOS' TLS library,
> and I suppose that's going to be facing similar issues.  It would be
> a good plan, probably, to try to push both of these to conclusion in
> the same development cycle.

The thing which I think would save the most aggravation - at least for
my employer - is a Windows SSL implementation.  Relying on OpenSSL
means that every time OpenSSL puts out a critical security fix, we've
got to rewrap all the Windows installers to pick up the new version.
If we were relying on what's built into Windows, it would be
Microsoft's problem.  Granted, it's not anybody's job to solve
EnterpriseDB's problems except EnterpriseDB, but users might like it
too -- and anyone else who is building Windows installers for
PostgreSQL.

Depending on macOS TLS instead of OpenSSL has similar advantages, of
course, just for a somewhat less common platform.

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

> On 01 Sep 2017, at 20:00, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Sep 1, 2017 at 1:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:
>>>> I have seen discussions from time to time about OpenSSL and its licensing
>>>> issues so I decided to see how much work it would be to add support for
>>>> another TLS library, and  I went with GnuTLS since it is the library I know
>>>> best after OpenSSL and it is also a reasonably popular library.
>>
>>> Thanks for working on this.  I think it's good for PostgreSQL to have
>>> more options in this area.
>>
>> +1.  We also have a patch in the queue to support macOS' TLS library,
>> and I suppose that's going to be facing similar issues.  It would be
>> a good plan, probably, to try to push both of these to conclusion in
>> the same development cycle.
>
> The thing which I think would save the most aggravation - at least for
> my employer - is a Windows SSL implementation.

In 53EA546E.6020404@vmware.com, an early version of SChannel support was posted
by Heikki.  If anyone is keen to pick up the effort that would most likely be a
good starting point.

> Relying on OpenSSL
> means that every time OpenSSL puts out a critical security fix, we've
> got to rewrap all the Windows installers to pick up the new version.
> If we were relying on what's built into Windows, it would be
> Microsoft's problem.  Granted, it's not anybody's job to solve
> EnterpriseDB's problems except EnterpriseDB, but users might like it
> too -- and anyone else who is building Windows installers for
> PostgreSQL.
>
> Depending on macOS TLS instead of OpenSSL has similar advantages, of
> course, just for a somewhat less common platform.

I think providing alternatives to OpenSSL on platforms where OpenSSL can’t be
relied on to be already available (Windows and macOS come to mind) would be a
great thing for many users and app developers.

cheers ./daniel

Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:

> >> There are currently two failing SSL tests which at least to me seems more
> >> like they test specific OpenSSL behaviors rather than something which need
> >> to be true for all SSL libraries.
> 
> > I don't know what we should do about these issues.
> 
> Maybe the SSL test suite needs to be implementation-specific as well.

If only two tests fail currently, I suggest that the thing to do is to
split it up in generic vs. library-specific test files.  It should be
easy to do with the TAP framework -- just move the library-specific
tests to their own file and mark it as skipped at the start of the file
when a different library is detected.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep  1, 2017 at 12:09:35PM -0400, Robert Haas wrote:
> I think that what this shows is that the current set of GUCs is overly
> OpenSSL-centric.  We created a set of GUCs that are actually specific
> to one particular implementation but named them as if they were
> generic.  My idea about this would be to actually rename the existing
> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
> as needed for other SSL implementations.
> 
> Depending on what we think is best, GUCs for an SSL implementation
> other than the one against which we compiled can either not exist at
> all, or can exist but be limited to a single value (e.g. "none", as we
> currently do when the compile has no SSL support at all).  Also, we
> could add a read-only GUC with a name like ssl_library that exposes
> the name of the underlying SSL implementation - none, openssl, gnutls,
> or whatever.
> 
> I think if we go the route of insisting that every SSL implementation
> has to use the existing GUCs, we're just trying to shove a square peg
> into a round hole, and there's no real benefit for users.  If the
> string that has to be stuffed into ssl_ciphers differs based on which
> library was chosen at compile time, then you can't have a uniform
> default configuration for all libraries anyway.  I think it'll be
> easier to explain and document this if there's separate documentation
> for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
> documentation section that tries to explain every implementation
> separately.

I am worried about having 3x version of TLS controls in postgresql.conf,
and only one set being active.  Perhaps we need to break out the TLS
config to separate files or something.  Anyway, this needs more thought.

--  Bruce Momjian  <bruce@momjian.us>        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 +

On Fri, Sep 01, 2017 at 10:33:37PM +0200, Alvaro Herrera wrote:
> Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> > > On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> 
> > >> There are currently two failing SSL tests which at least to me seems more
> > >> like they test specific OpenSSL behaviors rather than something which need
> > >> to be true for all SSL libraries.
> > 
> > > I don't know what we should do about these issues.
> > 
> > Maybe the SSL test suite needs to be implementation-specific as well.
> 
> If only two tests fail currently, I suggest that the thing to do is to
> split it up in generic vs. library-specific test files.  It should be
> easy to do with the TAP framework -- just move the library-specific
> tests to their own file and mark it as skipped at the start of the file
> when a different library is detected.

This seems like a much smarter and more reliable way to test.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Hi,

On 09/04/2017 04:24 PM, Bruce Momjian wrote:
> On Fri, Sep  1, 2017 at 12:09:35PM -0400, Robert Haas wrote:
>> I think that what this shows is that the current set of GUCs is overly
>> OpenSSL-centric.  We created a set of GUCs that are actually specific
>> to one particular implementation but named them as if they were
>> generic.  My idea about this would be to actually rename the existing
>> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
>> as needed for other SSL implementations.
>>
>> Depending on what we think is best, GUCs for an SSL implementation
>> other than the one against which we compiled can either not exist at
>> all, or can exist but be limited to a single value (e.g. "none", as we
>> currently do when the compile has no SSL support at all).  Also, we
>> could add a read-only GUC with a name like ssl_library that exposes
>> the name of the underlying SSL implementation - none, openssl, gnutls,
>> or whatever.
>>
>> I think if we go the route of insisting that every SSL implementation
>> has to use the existing GUCs, we're just trying to shove a square peg
>> into a round hole, and there's no real benefit for users.  If the
>> string that has to be stuffed into ssl_ciphers differs based on which
>> library was chosen at compile time, then you can't have a uniform
>> default configuration for all libraries anyway.  I think it'll be
>> easier to explain and document this if there's separate documentation
>> for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
>> documentation section that tries to explain every implementation
>> separately.
> 
> I am worried about having 3x version of TLS controls in
> postgresql.conf, and only one set being active. Perhaps we need to
> break out the TLS config to separate files or something. Anyway, this
> needs more thought.
> 

Well, people won't be able to set the inactive options, just like you
can't set ssl=on when you build without OpenSSL support. But perhaps we
could simply not include the inactive options into the config file, no?

I don't see how breaking the TLS config into separate files improves the
situation - instead of dealing with GUCs in a single file you'll be
dealing with the same GUCs in multiple files. No?

regards

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

On 09/07/2017 11:34 PM, Tomas Vondra wrote:
>> I am worried about having 3x version of TLS controls in
>> postgresql.conf, and only one set being active. Perhaps we need to
>> break out the TLS config to separate files or something. Anyway, this
>> needs more thought.
> 
> Well, people won't be able to set the inactive options, just like you
> can't set ssl=on when you build without OpenSSL support. But perhaps we
> could simply not include the inactive options into the config file, no?

Yeah, I have been thinking about how bad it would be to dynamically 
generate the config file. I think I will try this.

Daniel: What options does Secure Transport need for configuring ciphers, 
ECDH, and cipher preference? Does it need any extra options (I think I 
saw something about the keychain)?

Andreas


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andreas Karlsson <andreas@proxel.se> writes:
> On 09/07/2017 11:34 PM, Tomas Vondra wrote:
>> Well, people won't be able to set the inactive options, just like you
>> can't set ssl=on when you build without OpenSSL support. But perhaps we
>> could simply not include the inactive options into the config file, no?

> Yeah, I have been thinking about how bad it would be to dynamically 
> generate the config file. I think I will try this.

I'm not exactly convinced that dynamically inserting some parameters
and not others is a great idea.  Remember that people tend to copy
postgresql.conf files forward from one installation to another.  Or
they might decide to rebuild the postmaster for an existing installation
with a different SSL library.  In any scenario like that, you've not
done them any real favor if the config file they have contains no trace
of the SSL parameters they need.

I think we might be best off just playing it straight and providing
a config file that contains a section along these lines:

# Parameters for OpenSSL.  Leave these commented out if not using OpenSSL.
#
#ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
#ssl_prefer_server_ciphers = on
#ssl_ecdh_curve = 'prime256v1'
#ssl_dh_params_file = ''
#ssl_cert_file = 'server.crt'
#ssl_key_file = 'server.key'
#ssl_ca_file = ''
#ssl_crl_file = ''
#
# Parameters for GnuTLS.  Leave these commented out if not using GnuTLS.
#
#gnufoo=...
#gnubar=...
#
# Parameters for macOS TLS.  ... you get the idea.

As previously noted, it'd be a good idea to rename the existing
ssl_xxx parameters to openssl_xxx, except maybe ones that we think
will be universal.  (But even if we do think that, it might be
simpler in the long run to just have three or four totally independent
sections of the config file, instead of some common and some library-
specific parameters.)
        regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Sep 7, 2017 at 10:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think we might be best off just playing it straight and providing
> a config file that contains a section along these lines:
>
> # Parameters for OpenSSL.  Leave these commented out if not using OpenSSL.
> #
> #ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
> #ssl_prefer_server_ciphers = on
> #ssl_ecdh_curve = 'prime256v1'
> #ssl_dh_params_file = ''
> #ssl_cert_file = 'server.crt'
> #ssl_key_file = 'server.key'
> #ssl_ca_file = ''
> #ssl_crl_file = ''
> #
> # Parameters for GnuTLS.  Leave these commented out if not using GnuTLS.
> #
> #gnufoo=...
> #gnubar=...
> #
> # Parameters for macOS TLS.  ... you get the idea.
>
> As previously noted, it'd be a good idea to rename the existing
> ssl_xxx parameters to openssl_xxx, except maybe ones that we think
> will be universal.  (But even if we do think that, it might be
> simpler in the long run to just have three or four totally independent
> sections of the config file, instead of some common and some library-
> specific parameters.)

+1 to all of that.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 09/15/2017 06:55 PM, Jeff Janes wrote:
> I can't build against gnutls-2.12.23-21.el6.x86_64 from CentOS 6.9

Thanks for testing my patch. I have fixed both these issues plus some of 
the other feedback. A new version of my patch is attached which should, 
at least on theory, support all GnuTLS versions >= 2.11.

I just very quickly fixed the broken SSL tests, as I am no fan of how 
the SSL tests currently are written and think they should be cleaned up.

Andreas

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

Andreas

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Mon, Nov 20, 2017 at 9:42 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> I don't want to be the annoying guy, but this patch no longer applies
> due to 642bafa0c5f9f08d106a14f31429e0e0c718b603 touching the tests :-(

Sorry about that. Something more specific needs to happen here as well
for channel binding support with SCRAM. CheckSCRAMAuth() now assumes
that the channel binding mechanism SCRAM-SHA-256-PLUS can be published
to the client if SSL is used, because OpenSSL is the only
implementation available. Does gnutls include an API to allow an
application to fetch the bytes from the TLS finished message? I can
see some references by glancing at the tarball of gnutls 3.6.1, but
you would need something similar to OpenSSL's SSL_get_peer_finished().
If that cannot be achieved I think that it will be necessary to tweak
auth.c to not publish the -PLUS mechanism if for example the result of
be_tls_get_peer_finished() is NULL. No need for a new SSL-specific
API. At the end it would prove to be more portable to do so for all
the SSL implementations, MacOS stuff does not document an API to get
the TLS finish message bytes.

> 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.
-- 
Michael

On 11/20/2017 02:56 AM, Michael Paquier wrote:
> Sorry about that. Something more specific needs to happen here as well
> for channel binding support with SCRAM. CheckSCRAMAuth() now assumes
> that the channel binding mechanism SCRAM-SHA-256-PLUS can be published
> to the client if SSL is used, because OpenSSL is the only
> implementation available. Does gnutls include an API to allow an
> application to fetch the bytes from the TLS finished message? I can
> see some references by glancing at the tarball of gnutls 3.6.1, but
> you would need something similar to OpenSSL's SSL_get_peer_finished().
> If that cannot be achieved I think that it will be necessary to tweak
> auth.c to not publish the -PLUS mechanism if for example the result of
> be_tls_get_peer_finished() is NULL. No need for a new SSL-specific
> API. At the end it would prove to be more portable to do so for all
> the SSL implementations, MacOS stuff does not document an API to get
> the TLS finish message bytes.

There is a function called gnutls_session_channel_binding() which seems 
to do something very similar to SSL_get*_finished() which has been in 
GnuTLS since 2.12.

https://www.gnutls.org/manual/gnutls.html#Channel-Bindings

>>   --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.

Agreed.

Andreas

On Tue, Nov 21, 2017 at 11:45 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> There is a function called gnutls_session_channel_binding() which seems to
> do something very similar to SSL_get*_finished() which has been in GnuTLS
> since 2.12.
>
> https://www.gnutls.org/manual/gnutls.html#Channel-Bindings

Nice. This way you can get tls-unique to work properly.
-- 
Michael

On 11/20/2017 02:56 AM, Michael Paquier wrote:
> On Mon, Nov 20, 2017 at 9:42 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> 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 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=<path> 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=<path>? 
I also do not know the Windows build code very well (or really at all).

Andreas

On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> The script for the windows version takes the
> --with-openssl=<path> 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=<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=<path>, and change the openssl comment to
match that.
-- 
Michael

On 11/27/2017 02:20 AM, Michael Paquier wrote:
> On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> The script for the windows version takes the
>> --with-openssl=<path> 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=<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=<path>, 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

On Mon, Nov 27, 2017 at 10:28 AM, Andreas Karlsson <andreas@proxel.se> 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

On Mon, Nov 27, 2017 at 2:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Nov 27, 2017 at 10:28 AM, Andreas Karlsson <andreas@proxel.se> 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

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

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

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=<path> 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=<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

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

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

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

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

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=<path> 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=<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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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.

            regards, tom lane

Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Jan 17, 2018 at 8:18 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

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

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

On Wed, Jan 17, 2018 at 6:48 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> ... Worse yet, users are
> not going to intrinsically know which SSL implementation was compiled
> into the server they have.

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.

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.

            regards, tom lane

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

On Wed, Jan 17, 2018 at 10:02 PM, Tom Lane <tgl@sss.pgh.pa.us> 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.

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

> On 19 Jan 2018, at 19:43, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

> 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.

Perhaps, I still think it’s a good idea to keep it in README.SSL

> 0004-Move-SSL-API-comments-to-header-files.patch
> 0005-Extract-common-bits-from-OpenSSL-implementation.patch
>
> Move copy-and-paste avoidance.

+1 on doing this type of refactoring before anything else lands, when
implementing ST the excessive copy-paste was annoying (but avoided for
scope-creep).

cheers ./daniel

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.

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

> On 25 Jan 2018, at 15:07, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

On Fri, Jan 19, 2018 at 01:55:30PM -0500, Robert Haas wrote:
> On Wed, Jan 17, 2018 at 10:02 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Fri, Jan 26, 2018 at 12:27:16AM +0100, Daniel Gustafsson wrote:
>> On 25 Jan 2018, at 15:07, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

> On 26 Jan 2018, at 02:10, Michael Paquier <michael.paquier@gmail.com> 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

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

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  <bruce@momjian.us>        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 +

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

On 1/27/18 19:49, Bruce Momjian wrote:
> To open another can of worms, are we ever going to rename "ssl"
> parameters to "tls"

I had been thinking about that, too, but it doesn't seem worth it.
While renaming server-side settings might be feasible with some
annoyance, this would also include renaming libpq connection parameters
and APIs, which would be much more problematic.

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.

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

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.

> 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.

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

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=<path> 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=<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

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

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

Andres Freund <andres@anarazel.de> 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

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

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

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: Peter Eisentraut 2018-01-03 <99680dba-cf63-8151-1de2-46ca93897e56@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

On Thu, Feb 1, 2018 at 5:08 AM, Christoph Berg <myon@debian.org> wrote:
> Re: Peter Eisentraut 2018-01-03 <99680dba-cf63-8151-1de2-46ca93897e56@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

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

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

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

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

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

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

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

> On Fri, Aug 31, 2018 at 1:28 PM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

On Thu, Nov 29, 2018 at 8:28 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Nov 29, 2018 at 8:28 AM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> 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

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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, Nov 29, 2018 at 8:28 AM Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> 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

Greetings,

* Andres Freund (andres@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