Support for Secure Transport SSL library on macOS as OpenSSL alternative

On 08/03/2017 01:02 PM, Daniel Gustafsson wrote:

In /messages/by-id/69DB7657-3F9D-4D30-8A4B-E06034251F61@yesql.se I
presented a WIP patch for adding support for the Apple Secure Transport SSL
library on macOS as, an alternative to OpenSSL. That patch got put on the
backburner for a bit, but I’ve now found the time to make enough progress to
warrant a new submission for discussions on this (and hopefully help hacking).

Hooray!

Keychains
=========
The frontend has support for using PEM files for certificates and keys. It can
also look up the key for the certificate in a Keychain, or both certificate and
key in a Keychain. The root certificate is still just read from a PEM file.

Why can't you have the root certificate in the keychain, too? Just not
implemented yet, or is there some fundamental problem with it?

The existence of an sslcert config trumps a keychain, but when a keychain is
used I’m currently using the sslcert parameter (awaiting a discussion on how to
properly do this) for the certificate label required to search the keychain.

There is a new frontend parameter called “keychain” with which a path to a
custom Keychain file can be passed. If set, this Keychain will be searched as
well as the default. If not, only the default user Keychain is used. There is
nothing that modifies the Keychain in this patch, it can read identities
(certificates and its key) but not alter them in any way.

OpenSSL also has a mechanism somewhat similar to the keychain, called
"engines". You can e.g. keep the private key corresponding a certificate
on a smart card, and speak to it with an OpenSSL "smart card reader"
engine. If you do that, the 'sslkey' syntax is "<engine name>:<key
name>". Perhaps we should adopt that syntax here as well? For example,
to read the client certificate from the key chain, you would use libpq
options like "keychain=/home/heikki/my_keychain
sslcert=keychain:my_client_cert".

“keychain” is obviously a very Secure Transport specific name, and I personally
think we should avoid that. Any new configuration added here should take
future potential implementation into consideration such that avoid the need for
lots of backend specific knobs. “sslcertstore” comes to mind as an
alternative, but we’d also need parameters to point into the certstore for
finding what we need. Another option would be to do a URL based scheme
perhaps.

I wouldn't actually mind using implementation-specific terms like
"keychain" here. It makes it clear that it's implementation-specific. I
think it would be confusing, to use the same generic option name, like
"sslcertstore", for both a macOS keychain and e.g. the private key store
in Windows. Or GNU Keyring. In the worst case, you might even have
multiple such "key stores" on the same system, so you'd anyways need a
way to specify which one you mean.

Actually, perhaps it should be made even more explicit, and call it
"secure_transport_keychain". That's quite long, though.

Wrt. keychains, is there a system-global or per-user keychain in macOS?
And does this patch use them? If you load a CRL into a global keychain,
does it take effect?

Testing
=======
In order to test this we need to provide an alternative to the openssl calls in
src/test/ssl/Makefile for Secure Transport.

Those openssl commands are only needed to re-generate the test
certificates. The certificates are included in the git repository, so
you only need to re-generate them if you want to modify them or add new
ones. I think it's OK to require the openssl tool for that, it won't be
needed just to run the test suite.

Documentation
=============
I have started fiddling with this a little, but to avoid spending time on the
wrong thing I have done very little awaiting the outcome of discussions here.
I have tried to add lots of comments to the code however, to explain the quirks
of Secure Transport.

I think this patch in general is in very good shape, and the next step
is to write the documentation. In particular, I'd like to see
documentation on how the keychain stuff should work. It'll be easier to
discuss the behavior and the interactions, once it's documented.

In fact, better to write the documentation for that now, and not bother
to change the code, until after we've discussed and are happy with the
documented behavior.

I went into this thinking I would write a README for how to implement a new SSL
library. But in the end, turns out the refactoring that went into our SSL code
earlier made that part almost too easy to warrant that. It’s really quite
straightforward.

That's nice to hear!

- Heikki

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

On 03 Aug 2017, at 13:06, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 08/03/2017 01:02 PM, Daniel Gustafsson wrote:

The frontend has support for using PEM files for certificates and keys. It can
also look up the key for the certificate in a Keychain, or both certificate and
key in a Keychain. The root certificate is still just read from a PEM file.

Why can't you have the root certificate in the keychain, too? Just not implemented yet, or is there some fundamental problem with it?

Just not implemented yet, awaiting Keychain discussions.

The existence of an sslcert config trumps a keychain, but when a keychain is
used I’m currently using the sslcert parameter (awaiting a discussion on how to
properly do this) for the certificate label required to search the keychain.
There is a new frontend parameter called “keychain” with which a path to a
custom Keychain file can be passed. If set, this Keychain will be searched as
well as the default. If not, only the default user Keychain is used. There is
nothing that modifies the Keychain in this patch, it can read identities
(certificates and its key) but not alter them in any way.

OpenSSL also has a mechanism somewhat similar to the keychain, called "engines". You can e.g. keep the private key corresponding a certificate on a smart card, and speak to it with an OpenSSL "smart card reader" engine. If you do that, the 'sslkey' syntax is "<engine name>:<key name>". Perhaps we should adopt that syntax here as well? For example, to read the client certificate from the key chain, you would use libpq options like "keychain=/home/heikki/my_keychain sslcert=keychain:my_client_cert”.

Thats definitely an option, although it carries a bit redudancy in this case
which can confuse users. With “keychain=/foo/bar.keychain sslcert=my_cert”,
did the user mean a file called my_cert or is it a reference to a cert in the
keychain? Nothing that strict parsing rules, good errormessages and
documentation can’t solve but needs careful thinking.

“keychain” is obviously a very Secure Transport specific name, and I personally
think we should avoid that. Any new configuration added here should take
future potential implementation into consideration such that avoid the need for
lots of backend specific knobs. “sslcertstore” comes to mind as an
alternative, but we’d also need parameters to point into the certstore for
finding what we need. Another option would be to do a URL based scheme
perhaps.

I wouldn't actually mind using implementation-specific terms like "keychain" here. It makes it clear that it's implementation-specific. I think it would be confusing, to use the same generic option name, like "sslcertstore", for both a macOS keychain and e.g. the private key store in Windows. Or GNU Keyring. In the worst case, you might even have multiple such "key stores" on the same system, so you'd anyways need a way to specify which one you mean.

Thats a good point.

Actually, perhaps it should be made even more explicit, and call it "secure_transport_keychain". That's quite long, though.

Yeah, secure_transport_ is a pretty long prefix.

Wrt. keychains, is there a system-global or per-user keychain in macOS? And does this patch use them? If you load a CRL into a global keychain, does it take effect?

Each user has a default Keychain , and on top of that you can create as many
Keychains as you want (a Keychain is really just a database file containing
secret things). The frontend use the default one for lookups unless the
keychain parameter is set in which case it uses both.

When evaluating trust, Secure Transport will use the default Keychain even if
not explicitly opened (as in the backend code). It does however only search
for intermediate certificates and not root certs/CRLs. Adding a policy object
for using CRLs should force it to afaik, but we’d need to support additional
keychains (if only to be able to test without polluting the default).

Testing
=======
In order to test this we need to provide an alternative to the openssl calls in
src/test/ssl/Makefile for Secure Transport.

Those openssl commands are only needed to re-generate the test certificates. The certificates are included in the git repository, so you only need to re-generate them if you want to modify them or add new ones. I think it's OK to require the openssl tool for that, it won't be needed just to run the test suite.

Ah, of course. We still need support for re-generating Keychains (or at all
generate them in case we don’t want binaries in the repository).

Documentation
=============
I have started fiddling with this a little, but to avoid spending time on the
wrong thing I have done very little awaiting the outcome of discussions here.
I have tried to add lots of comments to the code however, to explain the quirks
of Secure Transport.

I think this patch in general is in very good shape, and the next step is to write the documentation. In particular, I'd like to see documentation on how the keychain stuff should work. It'll be easier to discuss the behavior and the interactions, once it's documented.

I’ll try to polish off the patch I have documenting the current behavior.

In fact, better to write the documentation for that now, and not bother to change the code, until after we've discussed and are happy with the documented behavior.

Yeah, discussion -> documentation -> code was my plan.

cheers ./daniel

--
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 3, 2017 at 12:02 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

In /messages/by-id/69DB7657-3F9D-4D30-8A4B-E06034251F61@yesql.se I
presented a WIP patch for adding support for the Apple Secure Transport SSL
library on macOS as, an alternative to OpenSSL. That patch got put on the
backburner for a bit, but I’ve now found the time to make enough progress to
warrant a new submission for discussions on this (and hopefully help hacking).

It is a drop-in replacement for the OpenSSL code, and supports all the same
features and options, except for two things: compression is not supported and
the CRL cannot be loaded from a plain PEM file. A Keychain must be used for
that instead.

Is there a set of APIs to be able to get server certificate for the
frontend and the backend, and generate a hash of it? That matters for
channel binding support of SCRAM for tls-server-end-point. There were
no APIs to get the TLS finish message last time I looked at OSX stuff,
which mattered for tls-unique. It would be nice if we could get one.
--
Michael

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

On 03 Aug 2017, at 19:27, Michael Paquier <michael.paquier@gmail.com> wrote:

On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

In /messages/by-id/69DB7657-3F9D-4D30-8A4B-E06034251F61@yesql.se I
presented a WIP patch for adding support for the Apple Secure Transport SSL
library on macOS as, an alternative to OpenSSL. That patch got put on the
backburner for a bit, but I’ve now found the time to make enough progress to
warrant a new submission for discussions on this (and hopefully help hacking).

It is a drop-in replacement for the OpenSSL code, and supports all the same
features and options, except for two things: compression is not supported and
the CRL cannot be loaded from a plain PEM file. A Keychain must be used for
that instead.

Is there a set of APIs to be able to get server certificate for the
frontend and the backend, and generate a hash of it? That matters for
channel binding support of SCRAM for tls-server-end-point.

I believe we can use SSLCopyPeerTrust() for that. Admittedly I haven’t looked
at that yet so need to get my head around channel binding, but it seems to fit
the bill.

There were no APIs to get the TLS finish message last time I looked at OSX
stuff, which mattered for tls-unique. It would be nice if we could get one.

Yeah, AFAICT there is no API for that.

cheers ./daniel

--
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 3, 2017 at 11:26 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

On 03 Aug 2017, at 19:27, Michael Paquier <michael.paquier@gmail.com> wrote:
There were no APIs to get the TLS finish message last time I looked at OSX
stuff, which mattered for tls-unique. It would be nice if we could get one.

Yeah, AFAICT there is no API for that.

Perhaps my last words were confusing. I meant that it would be nice to
get at least one type of channel binding working.
--
Michael

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

On 03 Aug 2017, at 13:36, Daniel Gustafsson <daniel@yesql.se> wrote:

On 03 Aug 2017, at 13:06, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

Why can't you have the root certificate in the keychain, too? Just not implemented yet, or is there some fundamental problem with it?

Just not implemented yet, awaiting Keychain discussions.

Supported in this new patchset.

Yeah, discussion -> documentation -> code was my plan.

Attached is an updated set of patches, rebased on top of master, with bug fixes
and additional features missing in the first set. While not complete (yet), in
case anyone is testing this I’d rather send a fresh batch rather than sitting
on them too long while I keep hacking at the docs. While not every part of
this rather large changeset has been touched, this includes all the patches for
completeness sake.

This patchset has certificate revocation as the main thing left. I’ve done
work on supporting CRLs via a Keychain and a CRL policy but that needs more
work (any help is much welcome).

DH parameters are loaded via the GUC, and instead of DER format (which is what
Secure Transport wants) it uses PEM such that it can load the same precomputed
parameter as be-secure-openssl.c and the same files.

Keychains are supported for root certificates in the frontend as well and are
added to the backend for identities as a first test for how it can be
integrated. Referencing an identity in a keychain is done by prefixing the
certificate parameter with keychain: and specifying the common name rather than
filename. The current code doesn’t support setting the passphrase for a
Keychain, it will try with a blank password (since thats handy for testing) and
if that fails it will bring up a GUI element for the passphrase. How to set a
passphrase in the server is open for discussion, a Keychain cannot be created
without a passphrase (it can be blank, but you still need to pass ‘’ to it).

There is a first stab at documentation included, it needs a lot more work to
fully separate generic SSL information and backend specific information but
it’s a WIP.

Additionally, a fix for an issue in the SSL tests is included which only
surface in the Secure Transport tests since it uses connstring parameters with
spaces in them.

0001: Adds support for running the SSL tests against another set of server
binaries. Not changed from previous submission, except rebased on top of master.

0002: Secure Transport support for frontend & backend.

0003: A rough first stab at updating the docs to split SSL documentation into
generic SSL information, and backend specific information. Lot’s to do still
here but it’s a start.

0004: A fix for an issue in the SSL tests which broke Secure Transport testing.
The SELECT ‘$connstr’ clause in the test doesn’t play nice with connstrings
containing sslcert:'keychain:common name’ parameters.

cheers ./daniel

On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

Attached is an updated set of patches, rebased on top of master, with bug fixes
and additional features missing in the first set. While not complete (yet), in
case anyone is testing this I’d rather send a fresh batch rather than sitting
on them too long while I keep hacking at the docs. While not every part of
this rather large changeset has been touched, this includes all the patches for
completeness sake.

Hi,

+#if defined(USE_OPENSSL) || defined(USE_SECURETRANSPORT)
 #define USE_SSL
+#if defined(USE_OPENSSL)
+#define SSL_LIBRARY "OpenSSL"
+#elif defined(USE_SECURETRANSPORT)
+#define SSL_LIBRARY "Secure Transport"
+#endif
 #endif

If you configure with neither --with-securetransport nor
--with-openssl then SSL_LIBRARY finishes up undefined, and then guc.c
doesn't compile:

ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -O2 -I. -I.
-I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c
guc.c:3309:3: error: ‘SSL_LIBRARY’ undeclared here (not in a function)
SSL_LIBRARY,
^~~~~~~~~~~

I guess it should have a fallback definition, though I don't know what
it should be.

--
Thomas Munro
http://www.enterprisedb.com

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

On Sun, Aug 20, 2017 at 8:10 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

Attached is an updated set of patches, rebased on top of master, with bug fixes
and additional features missing in the first set. While not complete (yet), in
case anyone is testing this I’d rather send a fresh batch rather than sitting
on them too long while I keep hacking at the docs. While not every part of
this rather large changeset has been touched, this includes all the patches for
completeness sake.

Hi,

+#if defined(USE_OPENSSL) || defined(USE_SECURETRANSPORT)
#define USE_SSL
+#if defined(USE_OPENSSL)
+#define SSL_LIBRARY "OpenSSL"
+#elif defined(USE_SECURETRANSPORT)
+#define SSL_LIBRARY "Secure Transport"
+#endif
#endif

If you configure with neither --with-securetransport nor
--with-openssl then SSL_LIBRARY finishes up undefined, and then guc.c
doesn't compile:

ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -O2 -I. -I.
-I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c
guc.c:3309:3: error: ‘SSL_LIBRARY’ undeclared here (not in a function)
SSL_LIBRARY,
^~~~~~~~~~~

I guess it should have a fallback definition, though I don't know what
it should be.

Or maybe the guc should only exist if SSL_LIBRARY is defined? I mean
#if defined(SSL_LIBRARY) around this:

+       {
+               /* Can't be set in postgresql.conf */
+               {"ssl_library", PGC_INTERNAL, PRESET_OPTIONS,
+                       gettext_noop("Shows the SSL library used."),
+                       NULL,
+                       GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+               },
+               &ssl_library_string,
+               SSL_LIBRARY,
+               NULL, NULL, NULL
+       },

--
Thomas Munro
http://www.enterprisedb.com

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

On 19 Aug 2017, at 23:13, Thomas Munro <thomas.munro@enterprisedb.com> wrote:

On Sun, Aug 20, 2017 at 8:10 AM, Thomas Munro
<thomas.munro@enterprisedb.com <mailto:thomas.munro@enterprisedb.com>> wrote:

On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

Attached is an updated set of patches, rebased on top of master, with bug fixes
and additional features missing in the first set. While not complete (yet), in
case anyone is testing this I’d rather send a fresh batch rather than sitting
on them too long while I keep hacking at the docs. While not every part of
this rather large changeset has been touched, this includes all the patches for
completeness sake.

Hi,

+#if defined(USE_OPENSSL) || defined(USE_SECURETRANSPORT)
#define USE_SSL
+#if defined(USE_OPENSSL)
+#define SSL_LIBRARY "OpenSSL"
+#elif defined(USE_SECURETRANSPORT)
+#define SSL_LIBRARY "Secure Transport"
+#endif
#endif

If you configure with neither --with-securetransport nor
--with-openssl then SSL_LIBRARY finishes up undefined, and then guc.c
doesn't compile:

ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -O2 -I. -I.
-I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c
guc.c:3309:3: error: ‘SSL_LIBRARY’ undeclared here (not in a function)
SSL_LIBRARY,
^~~~~~~~~~~

I guess it should have a fallback definition, though I don't know what
it should be.

Or maybe the guc should only exist if SSL_LIBRARY is defined?

I think the intended use case of the GUC should drive the decision on fallback.
If the GUC isn’t supposed to be a way to figure out if the server was built
with SSL support, then not existing in non-SSL backends is fine. If, however,
we want to allow using the GUC to see if the server has SSL support, then there
needs to be a “None” or similar value for that case.

Personally I think there is risk of regrets down the line if this GUC is used
for two things, but thats more of a gut feeling than scientifically studied.

Clearly there shouldn’t be a compilation error in either case, sorry about
missing that in the submission.

cheers ./daniel

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

On Mon, Aug 21, 2017 at 6:21 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

On 19 Aug 2017, at 23:13, Thomas Munro <thomas.munro@enterprisedb.com> wrote:

I guess it should have a fallback definition, though I don't know what
it should be.

Or maybe the guc should only exist if SSL_LIBRARY is defined?

I think the intended use case of the GUC should drive the decision on fallback.
If the GUC isn’t supposed to be a way to figure out if the server was built
with SSL support, then not existing in non-SSL backends is fine. If, however,
we want to allow using the GUC to see if the server has SSL support, then there
needs to be a “None” or similar value for that case.

Only GUCs related to debugging have their existence defined based on a
#define, so it seems to me that if Postgres is compiled without any
SSL support, this parameter should still be visible, but set to
"none".
--
Michael

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

On Mon, Aug 21, 2017 at 9:46 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Aug 21, 2017 at 6:21 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

I think the intended use case of the GUC should drive the decision on fallback.
If the GUC isn’t supposed to be a way to figure out if the server was built
with SSL support, then not existing in non-SSL backends is fine. If, however,
we want to allow using the GUC to see if the server has SSL support, then there
needs to be a “None” or similar value for that case.

Only GUCs related to debugging have their existence defined based on a
#define, so it seems to me that if Postgres is compiled without any
SSL support, this parameter should still be visible, but set to
"none".

The last set of patches available here does not apply:
/messages/by-id/B5E2B87D-3E8A-4597-9A7F-8489B3B67556@yesql.se
The SSL test refactoring is one cause. I think as well that this is
crashing when attempting to use SCRAM authentication with the SSL
brand of macos and SCRAM's channel binding. I am going to send a patch
which allows handling of no support for channel bindings for a given
SSL implementation, something needed as well by the gnutls patch.
Please make sure that you define at least be_tls_get_peer_finished()
and pgtls_get_finished() with a NULL result and a length of 0 as
return results as, as far as I can see, macos does not give direct
access to the TLS finish message bytes. At least that's not
documented.
--
Michael

On Mon, Nov 20, 2017 at 11:35 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

The last set of patches available here does not apply:
/messages/by-id/B5E2B87D-3E8A-4597-9A7F-8489B3B67556@yesql.se
The SSL test refactoring is one cause. I think as well that this is
crashing when attempting to use SCRAM authentication with the SSL
brand of macos and SCRAM's channel binding. I am going to send a patch
which allows handling of no support for channel bindings for a given
SSL implementation, something needed as well by the gnutls patch.
Please make sure that you define at least be_tls_get_peer_finished()
and pgtls_get_finished() with a NULL result and a length of 0 as
return results as, as far as I can see, macos does not give direct
access to the TLS finish message bytes. At least that's not
documented.

This last comment is from last week, so I am marking the patch as
returned with feedback. This also needs more thoughts for channel
binding support with SCRAM.
--
Michael

Here’s an attempt at reviving an old patch that I’ve neglected for too long.

The attached patchset rebases Secure Transport support over HEAD and adds stub
functions for that the SCRAM support added to make everything compile and run
the SSL testsuite. There are no new features or bugfixes over the previously
posted patches.

Wrt SCRAM, I’m probably thick but I can’t really see what I need to do to
handle SCRAM, so I wouldn’t mind some cluesticks on that. The Secure Transport
API doesn’t allow for getting the TLS Finished message (at least I haven’t been
able to find a way), so channel binding can’t be supported afaict.

The testcode has been updated to handle Secure Transport, but it’s not
in a clean form, rather a quick hack to get something running while the project
settles on how to handle multiple SSL implementations.

I have for now excluded the previous doc changes awating the discussion on the
patch in 1f34fa82-52a0-1682-87ba-4c3c3d0afcc0@2ndquadrant.com, once that
settles I’ll revive and write the documentation. The same goes for GUCs etc
which are discussed in other threads.

As per before, my patch for running tests against another set of binaries is
included as well as a fix for connstrings with spaces, but with the recent
hacking by Peter I assume this is superfluous. It was handy for development so
I’ve kept it around though.

cheers ./daniel

On Mon, Jan 22, 2018 at 12:08:03AM +0100, Daniel Gustafsson wrote:

The attached patchset rebases Secure Transport support over HEAD and adds stub
functions for that the SCRAM support added to make everything compile and run
the SSL testsuite. There are no new features or bugfixes over the previously
posted patches.

Wrt SCRAM, I’m probably thick but I can’t really see what I need to do to
handle SCRAM, so I wouldn’t mind some cluesticks on that. The Secure Transport
API doesn’t allow for getting the TLS Finished message (at least I haven’t been
able to find a way), so channel binding can’t be supported afaict.

OK, thanks. That sucks, perhaps Apple will improve things in the future,
this is the kind of areas where there is always interest.

The testcode has been updated to handle Secure Transport, but it’s not
in a clean form, rather a quick hack to get something running while the project
settles on how to handle multiple SSL implementations.

I have for now excluded the previous doc changes awating the discussion on the
patch in 1f34fa82-52a0-1682-87ba-4c3c3d0afcc0@2ndquadrant.com, once that
settles I’ll revive and write the documentation. The same goes for GUCs etc
which are discussed in other threads.

As per before, my patch for running tests against another set of binaries is
included as well as a fix for connstrings with spaces, but with the recent
hacking by Peter I assume this is superfluous. It was handy for development so
I’ve kept it around though.

Yes that looks useful to test.

be-secure-securetransport.c is quite a long name by the way, perhaps we
could just live with be-secure-osx.c or be-secure-mac.c?

Here are some comments about the SCRAM/channel binding part..

+be_tls_get_peer_finished(Port *port, size_t *len)
+{
+   ereport(ERROR,
+           (errcode(ERRCODE_PROTOCOL_VIOLATION),
+            errmsg("channel binding is not supported by this build")));
+   return NULL;
Those should mention the channel binding type.

In CheckSCRAMAuth(), you are missing the fact that SCRAM-SHA-256-PLUS is
still published to the client. I think that this is a mistake as no
channel binding types are supported. We may want to add an API for each
SSL implementation to help with that as I want to keep the code paths
fetching the channel binding data only invoked when necessary. So adding
a new API which returns a list of channel binding types supported by a
given backend would make the most sense to me. If the list is empty,
then -PLUS is not published by the backend. This is not a problem
related to your patch, more a problem that I need to solve as gnutls
only supports tls-unique. So I'll create a new thread on the matter with
a proper patch.

 note "setting up data directory";
-my $node = get_new_node('master');
+my $node = get_new_node('master', $ENV{SSLTEST_SERVER_BIN});
 $node->init;
This bit is in 0001, but this concept is introduced in 0002. (Not sure
how to feel about that yet, there are similar use-cases with
pg_upgrade's TAP tests where you may want to enforce a PATH.)

Nit: patch has some whitespaces. You may want to run a git diff --check
or such before sending.
--
Michael

On 22 Jan 2018, at 02:46, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Jan 22, 2018 at 12:08:03AM +0100, Daniel Gustafsson wrote:

The attached patchset rebases Secure Transport support over HEAD and adds stub
functions for that the SCRAM support added to make everything compile and run
the SSL testsuite. There are no new features or bugfixes over the previously
posted patches.

Wrt SCRAM, I’m probably thick but I can’t really see what I need to do to
handle SCRAM, so I wouldn’t mind some cluesticks on that. The Secure Transport
API doesn’t allow for getting the TLS Finished message (at least I haven’t been
able to find a way), so channel binding can’t be supported afaict.

OK, thanks. That sucks, perhaps Apple will improve things in the future,
this is the kind of areas where there is always interest.

The testcode has been updated to handle Secure Transport, but it’s not
in a clean form, rather a quick hack to get something running while the project
settles on how to handle multiple SSL implementations.

I have for now excluded the previous doc changes awating the discussion on the
patch in 1f34fa82-52a0-1682-87ba-4c3c3d0afcc0@2ndquadrant.com, once that
settles I’ll revive and write the documentation. The same goes for GUCs etc
which are discussed in other threads.

As per before, my patch for running tests against another set of binaries is
included as well as a fix for connstrings with spaces, but with the recent
hacking by Peter I assume this is superfluous. It was handy for development so
I’ve kept it around though.

Yes that looks useful to test.

be-secure-securetransport.c is quite a long name by the way, perhaps we
could just live with be-secure-osx.c or be-secure-mac.c?

Since OpenSSL supports macOS, naming it be-secure-mac.c seems like it can add
confusion. On a philosophical level, since Secure Transport is available on
multiple operating systems (macOS, iOS, tvOS and watchOS) it also seems odd to
name the file after a platform even though postgres isn’t supported on the
others. That being said, I don’t really have any strong opinions. Perhaps
-stransport.c could be an option?

Here are some comments about the SCRAM/channel binding part..

+be_tls_get_peer_finished(Port *port, size_t *len)
+{
+   ereport(ERROR,
+           (errcode(ERRCODE_PROTOCOL_VIOLATION),
+            errmsg("channel binding is not supported by this build")));
+   return NULL;
Those should mention the channel binding type.

Good point, fixed.

In CheckSCRAMAuth(), you are missing the fact that SCRAM-SHA-256-PLUS is
still published to the client. I think that this is a mistake as no
channel binding types are supported. We may want to add an API for each
SSL implementation to help with that as I want to keep the code paths
fetching the channel binding data only invoked when necessary. So adding
a new API which returns a list of channel binding types supported by a
given backend would make the most sense to me. If the list is empty,
then -PLUS is not published by the backend. This is not a problem
related to your patch, more a problem that I need to solve as gnutls
only supports tls-unique. So I'll create a new thread on the matter with
a proper patch.

Aha, that does makes it clearer.

note "setting up data directory";
-my $node = get_new_node('master');
+my $node = get_new_node('master', $ENV{SSLTEST_SERVER_BIN});
$node->init;
This bit is in 0001, but this concept is introduced in 0002. (Not sure
how to feel about that yet, there are similar use-cases with
pg_upgrade's TAP tests where you may want to enforce a PATH.)

Doh, that was a git add -p error on my part. Fixed in the attached patchset.

Although I do think there is value to being able to specify a PATH for a set of
binaries to test against, the 0002 patch is as mentioned mainly included as a
show-and-tell patch to show what I’ve used for testing. If could of course be
extended to other test suites should there be interest.

Nit: patch has some whitespaces. You may want to run a git diff --check
or such before sending.

Fixed.

Thanks for looking at this!

cheers ./daniel

On 1/21/18 18:08, Daniel Gustafsson wrote:

As per before, my patch for running tests against another set of binaries is
included as well as a fix for connstrings with spaces, but with the recent
hacking by Peter I assume this is superfluous. It was handy for development so
I’ve kept it around though.

0002-Allow-running-SSL-tests-against-different-binar-v4.patch should be
obsoleted by f5da5683a86e9fc42fdf3eae2da8b096bda76a8a.

0003-Allow-spaces-in-connectionstrings-v4.patch looks reasonable, but
I'm not sure what circumstance is triggering that. Is it specific to
your implementation?

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

On 23 Jan 2018, at 18:20, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

On 1/21/18 18:08, Daniel Gustafsson wrote:

As per before, my patch for running tests against another set of binaries is
included as well as a fix for connstrings with spaces, but with the recent
hacking by Peter I assume this is superfluous. It was handy for development so
I’ve kept it around though.

0002-Allow-running-SSL-tests-against-different-binar-v4.patch should be
obsoleted by f5da5683a86e9fc42fdf3eae2da8b096bda76a8a.

Yes.

0003-Allow-spaces-in-connectionstrings-v4.patch looks reasonable, but
I'm not sure what circumstance is triggering that. Is it specific to
your implementation?

It’s not specific to the implementation per se, but it increases the likelyhood
of hitting it. In order to load certificates from Keychains the cert common
name must be specified in the connstr, when importing the testfiles into
keychains I ran into it for example src/test/ssl/client_ca.config.

cheers ./daniel

On 1/23/18 14:59, Daniel Gustafsson wrote:

It’s not specific to the implementation per se, but it increases the likelyhood
of hitting it. In order to load certificates from Keychains the cert common
name must be specified in the connstr, when importing the testfiles into
keychains I ran into it for example src/test/ssl/client_ca.config.

The change is

- 'psql', '-X', '-A', '-t', '-c', "SELECT 'connected with $connstr'",
+ 'psql', '-X', '-A', '-t', '-c', "SELECT \$\$connected with $connstr\$\$",

So the problem must have been a single quote in the connstr.

That can surely happen, but then so can having a $$. So without a
concrete example, I'm not sure how to proceed.

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

On 23 Jan 2018, at 20:59, Daniel Gustafsson <daniel@yesql.se> wrote:

On 23 Jan 2018, at 18:20, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

On 1/21/18 18:08, Daniel Gustafsson wrote:

As per before, my patch for running tests against another set of binaries is
included as well as a fix for connstrings with spaces, but with the recent
hacking by Peter I assume this is superfluous. It was handy for development so
I’ve kept it around though.

0002-Allow-running-SSL-tests-against-different-binar-v4.patch should be
obsoleted by f5da5683a86e9fc42fdf3eae2da8b096bda76a8a.

Yes.

On the note of patches made obsolete, the attached patch is rebased and updated
for the recent commits that moved common SSL code into shared files.

cheers ./daniel