Re: Support for Secure Transport SSL library on macOS as OpenSSL alternative

In https://postgr.es/m/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.

Current state
=============
The frontend and backend are implemented more or less fully, the two main
things missing being the CRL support (further details below) and loading DH
files (to support the GUC added in c0a15e07cd). All non-CRL tests but one are
passing. The failing test is in the frontend and it also fails when running
against an OpenSSL backend, but I can’t find where the logic is flawed and
could do with some help there.

Threads
=======
Just like the CFLocaleCopyCurrent() call referenced in postmaster.c, the Secure
Transport APIs makes the process multithreaded. To keep this out of the
postmaster, and contained in the backend, I’ve moved all functionality to
open_server and left init empty. I could definitely need some clues on how to
properly handle this, or if it’s a complete showstopper.

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

The backend is only supporting PEM files at this point.

Once we have support for Keychains, we can however use them for additionally
supporting other Secure Transport features like OCSP etc.

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

Certificate Revocation
======================
Secure Transport supports loading CRL lists into Keychain files, the command
line certtool can for example do that. When doing the trust evaluation on the
connection, a policy can be added which enables revocation checking via CRL. I
have however been unable to figure out how to load a CRL list programmatically,
and as far as I can tell there is no API for this. The certtool utility is
using the low-level CSSM APIs for this it seems, but adding that level of
complexity doesn’t seem worth it for us to maintain (I did a test and it turned
big, ugly and messy).

Unless someone knows how to implement this, we’d be limited to requiring the
use of a Keychain file for CRL support. This would break drop-in replacement
support, but supporting certificate revocation seems more important.

Platform Support
================
I’ve tested this on 10.11 El Capitan and 10.12 Sierra, which are the systems I
have access to. Supporting prairiedog and dromedary in the buildfarm will
probably be too hard (if at all possible), but down to 10.9 Mavericks should be
quite possible (if someone has the required systems to test on). It adds a
dependency on Core Foundation on top of Secure Transport, no other macOS APIs
are used.

Testing
=======
In order to test this we need to provide an alternative to the openssl calls in
src/test/ssl/Makefile for Secure Transport. On top of that, code to generate
Keychains is required. The certtool application can do all the Keychain
generations (I think) but this is still left open. The main thing to figure
out here is how to avoid having to type in the Keychain password in a modal GUI
that Secure Transport pops up. Since a Keychain can be passwordless it should
be doable, but the right certtool incantations for that is still evading me.
I’ve included a show-and-tell patch for this which I’ve used locally for
testing during hacking.

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

Patches
=======
0001 - Adds support for running the SSL tests against another set of server
binaries. This is only useful for testing during the implementation of a new
SSL library, but then it’s crucial. Nothing Secure Transport specific in this
patch.

0002 - Secure Transport fronten and backend as well as required scaffolding for
building and a new GUC to show the current backend.

securetransport_test.diff - Some ugly hacks I’ve used for testing, included as
a show-and-tell, not for any form of submission.

This leaves quite a few open questions that I hope to get some feedback on, and
some code issues which I’d love eyes/hacking on. Do we still want this, and if
so how to handle that we can’t be fully drop-in compatible with OpenSSL with
regards to using a PEM file only configuration? Should we support PEM files at
all in the backend or only Keychains? Another option could be to support
PKCS12 files instead of (or additionally to) PEM since there is likely to be
API support for loading PKCS12 and they can afaik contain CRLs. How can we
ensure that new parameters hopefully covers more libraries than just Secure
Transport?

cheers ./daniel

On 08/03/2017 01:02 PM, Daniel Gustafsson wrote:
> In https://postgr.es/m/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

> On 03 Aug 2017, at 13:06, Heikki Linnakangas <hlinnaka(at)iki(dot)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

On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> In https://postgr.es/m/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

> On 03 Aug 2017, at 19:27, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>
> On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>> In https://postgr.es/m/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

On Thu, Aug 3, 2017 at 11:26 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>> On 03 Aug 2017, at 19:27, Michael Paquier <michael(dot)paquier(at)gmail(dot)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

> On 03 Aug 2017, at 13:36, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
>> On 03 Aug 2017, at 13:06, Heikki Linnakangas <hlinnaka(at)iki(dot)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(at)yesql(dot)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

On Sun, Aug 20, 2017 at 8:10 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson <daniel(at)yesql(dot)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

> On 19 Aug 2017, at 23:13, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>
> On Sun, Aug 20, 2017 at 8:10 AM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com <mailto:thomas(dot)munro(at)enterprisedb(dot)com>> wrote:
>> On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson <daniel(at)yesql(dot)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

On Mon, Aug 21, 2017 at 6:21 AM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>> On 19 Aug 2017, at 23:13, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)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

On Mon, Aug 21, 2017 at 9:46 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Aug 21, 2017 at 6:21 AM, Daniel Gustafsson <daniel(at)yesql(dot)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:
https://www.postgresql.org/message-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(dot)paquier(at)gmail(dot)com> wrote:
> The last set of patches available here does not apply:
> https://www.postgresql.org/message-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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(dot)paquier(at)gmail(dot)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(at)2ndquadrant(dot)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(dot)eisentraut(at)2ndquadrant(dot)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(at)yesql(dot)se> wrote:
>
>> On 23 Jan 2018, at 18:20, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)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

> On 23 Jan 2018, at 22:04, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> 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.

Right, looking back at B5E2B87D-3E8A-4597-9A7F-8489B3B67556(at)yesql(dot)se I realized
I was misremembering, the issue was that I had sslcert:'keychain:common name’
parameters to encapsulate the whitespace into a string value. Sorry about that.

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

Awaiting with this until the discussion on how to handle configuration and
parameter per SSL implementation lands is probably best.

cheers ./daniel

On 1/23/18 16:18, Daniel Gustafsson wrote:
>> 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.
> Right, looking back at B5E2B87D-3E8A-4597-9A7F-8489B3B67556(at)yesql(dot)se I realized
> I was misremembering, the issue was that I had sslcert:'keychain:common name’
> parameters to encapsulate the whitespace into a string value. Sorry about that.

OK, makes sense. Committed that.

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

> On 23 Jan 2018, at 22:08, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
>> On 23 Jan 2018, at 20:59, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>
>>> On 23 Jan 2018, at 18:20, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)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.

And another rebase and update after the refactoring in c1869542b3a4da4b12ca.
Also fixed some typos in comments. The other patches originally posted in this
patchset are either committed or made redundant.

cheers ./daniel

Hi,

On 2018-01-26 23:30:08 +0100, Daniel Gustafsson wrote:
> And another rebase and update after the refactoring in c1869542b3a4da4b12ca.
> Also fixed some typos in comments. The other patches originally posted in this
> patchset are either committed or made redundant.

Could you provide a quick summary about where you think this patch
stands currently? The cover letter you had for this thread was great...

- Andres

> On 02 Mar 2018, at 10:10, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2018-01-26 23:30:08 +0100, Daniel Gustafsson wrote:
>> And another rebase and update after the refactoring in c1869542b3a4da4b12ca.
>> Also fixed some typos in comments. The other patches originally posted in this
>> patchset are either committed or made redundant.
>
> Could you provide a quick summary about where you think this patch
> stands currently? The cover letter you had for this thread was great…

I’m actually working on that right now, combined with a rebase. I’m travelling
today and tomorrow with limited time and internet available but I will provide
this on Sunday at the latest.

cheers ./daniel

> On 02 Mar 2018, at 03:10, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2018-01-26 23:30:08 +0100, Daniel Gustafsson wrote:
>> And another rebase and update after the refactoring in c1869542b3a4da4b12ca.
>> Also fixed some typos in comments. The other patches originally posted in this
>> patchset are either committed or made redundant.
>
> Could you provide a quick summary about where you think this patch
> stands currently? The cover letter you had for this thread was great…

Below is a summary of the current state of the patch.

Current State
=============
I’ve tried to keep this patch current with the SSL work that Peter E has been
doing, and I believe it is using the new generic infrastructure provided
wherever possible. The new SCRAM channel binding support is supported in the
sense that it isn’t supported (errors out “gracefully”) - due to Secure
Transport lacking the needed APIs.

Keychain support
================
In an earlier version this patch was not supporting Keychains for the backend
part, but I have since implemented that such that both frontend and backend can
load identities from Keychains. The common way to use Keychains in macOS apps
is to always include the user default Keychain in addition to any specific
ones. My opinion, which is what I’ve implemented, is that we (at least) in the
backend don’t want to include the user default Keychain by default, but instead
have a config setting which turns that on should the user want to. My
reasoning is that a) server applications want more fine grained control and; b)
including a Keychain we don’t control when running tests seems a like a
terrible idea. Allowing the default Keychain to be turned on with a config
knob seems a better option, so that’s what I’ve done. (this config knob was
added last week on the plane and was thus not in the last revision published, I
will post a new revision shortly but I want to read this code again first to
ensure I’m not wasting reviewer time with bugs/things I should’ve caught.)

Certificate Revocation
======================
There is no API for CRL files in Secure Transport, they are expected to be
loaded into the Keychain. The ssl_crl_file setting is erroring out with a hint
to avoid silently dropping important configuration. This is not a change from
any previously published version, but a background reminder for the next
section..

Testing
=======
Due to lack of better ideas, I’d added an ugly kludge in the tests to bypass
CRL tests on Secure Transport. I think however that I figured out how to
properly programmatically create Keychains that contain the identity and CRL
info to match the current SSL tests. I’m coding that right now, and hopefully
this means that there will be no (or few) diffs required against the tests,
only the infrastructure.

Commitfest Status
=================
Do I think this patch is realistic to target for v11? Well. Given where we
are in the cycle, I don’t think any new TLS implementation going in is
realistic at this point since none of the proposed ones have had enough tyre
kicking done. That might change should there be lots of interest and work
started soon, but as has been discussed elsewhere recently the project has
limited resources. I have time to work on this, and support reviewers of it,
but that’s only piece of the puzzle.

If no new TLS library is supported in v11, we still got cleaner SSL support out
of it due to the work performed to further remove our dependency on OpenSSL, so
we still come out on top IMO. Thanks Peter et.al!

The current revision of the patch applies cleanly on HEAD except for the tests,
but I hope to post a new version with reworked tests properly using Keychains
shortly (as in, early this week)

So, TL;DR: both frontend and backend API are implemented except for SCRAM
channel binding and CRL file support, it needs tests and documentation, it does
not implement pgcrypto, it will be fixed to support whichever GUC strategy the
project decides on for multiple TLS library support.

cheers ./daniel

On Sun, Mar 04, 2018 at 11:15:58PM +0100, Daniel Gustafsson wrote:
> Commitfest Status
> =================
> Do I think this patch is realistic to target for v11? Well. Given where we
> are in the cycle, I don’t think any new TLS implementation going in is
> realistic at this point since none of the proposed ones have had enough tyre
> kicking done. That might change should there be lots of interest and work
> started soon, but as has been discussed elsewhere recently the project has
> limited resources. I have time to work on this, and support reviewers of it,
> but that’s only piece of the puzzle.

This patch has been around for some time now, and a couple of people
have expressed interest in the feature, so it seems to me that there is
still some room to get new features in the tree. I am not personally
planning to spend much time on reviewing this specific implementation
though, however...

> If no new TLS library is supported in v11, we still got cleaner SSL support out
> of it due to the work performed to further remove our dependency on OpenSSL, so
> we still come out on top IMO. Thanks Peter et.al!

I am definitely interested in plugging in more generic APIs for this
release, so as people can also fork Postgres and implement their own SSL
implementation more easily. One patch that still applies in this area
is I think this one to allow SSL implementations let the backend know
more easily is channel binding needs to be implemented or not:
https://commitfest.postgresql.org/17/1491/

> So, TL;DR: both frontend and backend API are implemented except for SCRAM
> channel binding and CRL file support, it needs tests and documentation, it does
> not implement pgcrypto, it will be fixed to support whichever GUC strategy the
> project decides on for multiple TLS library support.

One bit of conclusion in this area is that at Peter E has argued in
favor of having separate configure switches for each each SSL
implementation instead of reworking things into a single, generic
switch. The GUC portion is also pointing in the direction of having one
set of parameters per implementation so as assigning default values is a
no-brainer. Seeing the GUC portion changed. Even if this is not
changed, there is always the point of assuming that the existing
SSL parameters map to OpenSSL, and add on top of it the new sets. But
that would be confusing for the user than renaming simply the existing
parameters.
--
Michael

> On 05 Mar 2018, at 02:41, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Sun, Mar 04, 2018 at 11:15:58PM +0100, Daniel Gustafsson wrote:

>> If no new TLS library is supported in v11, we still got cleaner SSL support out
>> of it due to the work performed to further remove our dependency on OpenSSL, so
>> we still come out on top IMO. Thanks Peter et.al!
>
> I am definitely interested in plugging in more generic APIs for this
> release, so as people can also fork Postgres and implement their own SSL
> implementation more easily. One patch that still applies in this area
> is I think this one to allow SSL implementations let the backend know
> more easily is channel binding needs to be implemented or not:
> https://commitfest.postgresql.org/17/1491/

Right. Regardless of the state of this patch I hope that can make it into 11
to further make 11 a good base for hacking on SSL code.

>> So, TL;DR: both frontend and backend API are implemented except for SCRAM
>> channel binding and CRL file support, it needs tests and documentation, it does
>> not implement pgcrypto, it will be fixed to support whichever GUC strategy the
>> project decides on for multiple TLS library support.
>
> One bit of conclusion in this area is that at Peter E has argued in
> favor of having separate configure switches for each each SSL
> implementation instead of reworking things into a single, generic
> switch. The GUC portion is also pointing in the direction of having one
> set of parameters per implementation so as assigning default values is a
> no-brainer.

FWIW I completely agree with this approach. With a single set of GUCs for all
implementations I fear that we risk ending up with configurations that require
shoehorning, and that will no-doubt open the risk for vulnerable servers due to
the configuration being hard to get right. Either approach does introduce a
problem for moving a hand-edited postgresql.conf files between clusters running
different libraries, but thats hard to avoid I think.

cheers ./daniel

On 2018-03-05 10:41:01 +0900, Michael Paquier wrote:
> > If no new TLS library is supported in v11, we still got cleaner SSL support out
> > of it due to the work performed to further remove our dependency on OpenSSL, so
> > we still come out on top IMO. Thanks Peter et.al!
>
> I am definitely interested in plugging in more generic APIs for this
> release, so as people can also fork Postgres and implement their own SSL
> implementation more easily.

I don't think that should be a goal. A positive side-effect, yes, but we
imo shouldn't let that as a goal guide us.

Greetings,

Andres Freund

On 3/4/18 17:15, Daniel Gustafsson wrote:
> Do I think this patch is realistic to target for v11? Well. Given where we
> are in the cycle, I don’t think any new TLS implementation going in is
> realistic at this point since none of the proposed ones have had enough tyre
> kicking done. That might change should there be lots of interest and work
> started soon, but as has been discussed elsewhere recently the project has
> limited resources. I have time to work on this, and support reviewers of it,
> but that’s only piece of the puzzle.

I think it would be best if both this patch and the GnuTLS patch are
moved to the next CF and are attacked early in the PG12 cycle.

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> On 3/4/18 17:15, Daniel Gustafsson wrote:
>> Do I think this patch is realistic to target for v11? Well. Given where we
>> are in the cycle, I don’t think any new TLS implementation going in is
>> realistic at this point since none of the proposed ones have had enough tyre
>> kicking done. That might change should there be lots of interest and work
>> started soon, but as has been discussed elsewhere recently the project has
>> limited resources. I have time to work on this, and support reviewers of it,
>> but that’s only piece of the puzzle.

> I think it would be best if both this patch and the GnuTLS patch are
> moved to the next CF and are attacked early in the PG12 cycle.

+1. I think it's fairly important that those two get reviewed/committed
in the same cycle, in case we need to adjust the relevant APIs. It
seems unlikely that we can muster the effort to get both done for v11.

regards, tom lane

> On 06 Mar 2018, at 22:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> On 3/4/18 17:15, Daniel Gustafsson wrote:
>>> Do I think this patch is realistic to target for v11? Well. Given where we
>>> are in the cycle, I don’t think any new TLS implementation going in is
>>> realistic at this point since none of the proposed ones have had enough tyre
>>> kicking done. That might change should there be lots of interest and work
>>> started soon, but as has been discussed elsewhere recently the project has
>>> limited resources. I have time to work on this, and support reviewers of it,
>>> but that’s only piece of the puzzle.
>
>> I think it would be best if both this patch and the GnuTLS patch are
>> moved to the next CF and are attacked early in the PG12 cycle.
>
> +1. I think it's fairly important that those two get reviewed/committed
> in the same cycle, in case we need to adjust the relevant APIs.

I completely agree with all of the above.

cheers ./daniel

> On 6 Mar 2018, at 22:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> On 3/4/18 17:15, Daniel Gustafsson wrote:
>>> Do I think this patch is realistic to target for v11? Well. Given where we
>>> are in the cycle, I don’t think any new TLS implementation going in is
>>> realistic at this point since none of the proposed ones have had enough tyre
>>> kicking done. That might change should there be lots of interest and work
>>> started soon, but as has been discussed elsewhere recently the project has
>>> limited resources. I have time to work on this, and support reviewers of it,
>>> but that’s only piece of the puzzle.
>
>> I think it would be best if both this patch and the GnuTLS patch are
>> moved to the next CF and are attacked early in the PG12 cycle.
>
> +1. I think it's fairly important that those two get reviewed/committed
> in the same cycle, in case we need to adjust the relevant APIs. It
> seems unlikely that we can muster the effort to get both done for v11.

Attached is an updated patch for supporting the native macOS Secure Transport
library, rebased on top of current master. This patch still fails a couple of
the SSL tests, and doesn’t support any channel binding for SCRAM, but is IMO a
good enough WIP/snifftest to see if the current implementation approach is at
all any good and/or of interest.

Apart from being rebased on current master, this version contains mostly
general cleanups as well as removing support for anything older than High
Sierra (I no longer have access to systems on older versions so I’m unable to
test). On top of that, the few notable new things are:

* adds support for disallowing usage of the default user Keychain

* adds support for ssl_passphrase_command which was added in 8a3d9425290ff5f64

* extends the SSL tests to pass in separate expected output per backend. How
to refactor the test code to cope with multiple backends hasn’t really been
discussed, with the GnuTLS patch taking another approach, but this was handy
for me while testing to keep the capabilities and functionality separate.
(Reordering the parameters to test_connect_fails() was initially by mistake but
I like how it matches the order in test_connect_ok() so kept it). This should
probably be discussed on a separate thread though.

If there are parts which are insufficiently commented, let me know and I’ll do
my best to extend.

cheers ./daniel

> On 27 Jun 2018, at 14:32, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> Attached is an updated patch for supporting the native macOS Secure Transport
> library, rebased on top of current master.

Courtesy of the ever-present Murphy I managed to forget some testcode in
src/backend/Makefile which broke compilation for builds without secure
transport, attached v8 patch fixes that.

cheers ./daniel

On 27/06/18 21:57, Daniel Gustafsson wrote:
>> On 27 Jun 2018, at 14:32, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
>> Attached is an updated patch for supporting the native macOS Secure Transport
>> library, rebased on top of current master.
>
> Courtesy of the ever-present Murphy I managed to forget some testcode in
> src/backend/Makefile which broke compilation for builds without secure
> transport, attached v8 patch fixes that.

I've read through this patch now in more detail. Looks pretty good, but
I have a laundry list of little things below. The big missing item is
documentation.

--- laundry list begins ---

What exactly does 'ssl_is_server_start' mean? I don't understand that
mechanism. ISTM it's only checked in load_key(), via
be_tls_open_server(), which should only be called after be_tls_init(),
in which case it's always 'true' when it's checked. Should there be an
Assertion on it or something?

The "-framework" option, being added to CFLAGS, is clang specific. I
think we need some more autoconf magic, to make this work with gcc.

In be_tls_open_server(), I believe there are several cases of using
variables uninitialized. For example, load_identity_keychain() doesn't
always set the 'identity' output parameter, but there's an "if (identity
== NULL)" check after the call to it. And 'certificates' is left
uninitialized, if load_identity_keychain() is used. Also, 'dh_len' is
used uninitialized here in the "if (!dh_buf || dh_len == 0)" line, if
the 'ssl_dh_params_file' option is not set. Did clang not give warnings
about these?

> + /*
> + * Certificate Revocation List are not supported in the Secure Transport
> + * API
> + */

The corresponding client code has a longer comment on that:

> + /*
> + * There is no API to load CRL lists in Secure Transport, they can however
> + * be imported into a Keychain with the commandline application "certtool".
> + * For libpq to use them, the certificate/key and root certificate needs to
> + * be using an identity in a Keychain into which the CRL have been
> + * imported. That needs to be documented.
> + */

Is it possible to programmatically create a temporary keychain, in
memory, and load the CRL to it? (I googled a bit, and couldn't find any
suitable API for it, so I guess not..)

> + if (ssl_crl_file[0])
> + ereport(FATAL,
> + (errmsg("CRL files not supported with Secure Transport")));

I think this should be COMMERROR, like other errors around this. We
don't want to pass that error message to the client. Although I guess it
won't reach the client as we haven't negotiated TLS yet.

> + /*
> + * We use kTryAuthenticate here since we don't know which sslmode the
> + * client is using. If we were to use kAlwaysAuthenticate then sslmode
> + * require won't work as intended.
> + */
> + if (ssl_loaded_verify_locations)
> + SSLSetClientSideAuthenticate((SSLContextRef) port->ssl, kTryAuthenticate);

That comment sounds wrong. This is in backend code, and
SSLSetClientSideAuthenticate() is all about checking the client's
certificate in the server, while libpq 'sslmode' is about checking the
server's certificate in the client.

> + for (;;)
> + {
> + status = SSLHandshake((SSLContextRef) port->ssl);
> + if (status == noErr)
> + break;
> +
> + if (status == errSSLWouldBlock)
> + continue;
> + ...
> + }

Does this busy-wait, if there's not enough data from the client? That
seems bad. In the corresponding client-side code, there's a comment on
that too, but it's still bad.

In be_tls_get_version and PQsslAttribute, can we add support for
kTLSProtocol13? Perhaps with an autoconf test, if the constant is not
available on all macOS versions.

In be_tls_get_peerdn_name, wouldn't SecCertificateCopyCommonName() be
more appropriate than SecCertificateCopyLongDescription()?

In be_tls_get_cipher, returning "unknown" would seem better than
erroring out, if the cipher isn't recognized.

Check error message style. eg.:

> + ereport(COMMERROR,
> + (errmsg("could not load server certificate \"%s\": \"%s\"",
> + ssl_cert_file, pg_SSLerrmessage(status))));
>

Why is the result of pg_SSLerrmessage() in quotes? Maybe errdetail?

> + * Any consumers of the Keychain array should always call this to ensure that
> + * it is set up in a manner that reflect the configuration. If it not, then

s/reflect/reflects/

> + else if (keychain_count == 2)
> + {
> + if (ssl_keychain_file[0] == '\0' || !ssl_keychain_use_default)
> + goto error;
> +
> + return;
> + }
> + else
> + /* Fallthrough to erroring out */
> +
> +error:
> + ereport(FATAL,
> + (errmsg("Incorrect loading of Keychains detected")));
> +}

That fallthrough looks dangerous.

> --- a/src/backend/utils/misc/postgresql.conf.sample
> +++ b/src/backend/utils/misc/postgresql.conf.sample
> @@ -106,6 +106,7 @@
> #ssl_dh_params_file = ''
> #ssl_passphrase_command = ''
> #ssl_passphrase_command_supports_reload = off
> +#ssl_keychain_file = ''

Do we want to have this Secure Transport-specific option in the sample
config file? Comment at least

> +#elif USE_SECURETRANSPORT
> + void *ssl;
> + int ssl_buffered;

Add comments. Mention the actual type of 'ssl' (SSLContextRef), and what
'ssl_buffered' means.

libpq-be.h and libpq-int.h use a different strategy to avoid clashes
between PostgreSQL's and Secure Transport's versions of Size and uint64.
Let's be consistent. (I like the libpq-fe.h version more, i.e. using
"void *" and avoiding the #include)

> - * SSL implementation (e.g. be-secure-openssl.c)
> + * SSL implementation (e.g. be-secure-<implementation>.c)

Since this is just an example, I think leaving it as be-secure-openssl.c
is fine.

Also update pg_config.h.win32 with the new USE_SECURETRANSPOT flag being
added to pg_config.h.in.

In the call to SSLSetPeerDomainName(), should check return code.

> + /*
> + * In sslmode "require" we accept some certificate verification
> + * failures when we don't have a rootcert since MITM protection
> + * isn't enforced. Check the reported failure and trust in case
> + * the cert is missing, self signed or expired/future.
> + */
> + if (strcmp(conn->sslmode, "require") == 0 && !conn->st_rootcert)
> + {

Not just "require", but "allow" as well, right?

freePGconn should free conn->keychain. Would be good to write a little
test that opens & closes millions of connectins, to check for memory leaks.

> + * Queries the specified Keychain, or the default unless not defined, for a

"unless not defined" is a double-negative. I don't understand that
sentence. (there are two copies of the same comment in the patch, in FE
and BE. And the FE function is called "load_identity_keychain", but its
comment says "import_identity_keychain" )

PQinitOpenSSL and PQinitSSL are inconsistent in whether to call
pgtls_init_library(), when compiled with Secure Transport.
pgtls_init_library() is a no-op, so it doesn't matter, but let's be
consistent. Perhaps do "#define pgtls_init_library() ((void)true)" in
the header?

s/readiblitity/readability/
s/dont/don't/
s/securetransport_common.h/securetransport.h/
s/f.e/for example/ (at least I'm not familiar with that abbreviation)

--- end of laundry list ---

- Heikki

Heikki Linnakangas <hlinnaka(at)iki(dot)fi> writes:
> On 27/06/18 21:57, Daniel Gustafsson wrote:
>> Courtesy of the ever-present Murphy I managed to forget some testcode in
>> src/backend/Makefile which broke compilation for builds without secure
>> transport, attached v8 patch fixes that.

> I've read through this patch now in more detail. Looks pretty good, but
> I have a laundry list of little things below. The big missing item is
> documentation.

I did some simple testing on this today. The patch has bit-rotted,
mostly as a consequence of 77291139c which removed tls-unique channel
binding. That's probably a good thing for the Secure Transport code,
since it wasn't supporting that anyway, but it needs to be updated.

I ripped out the failing-to-compile code (maybe that was too simplistic?)
but could not figure out whether there was anything still useful about
the diffs in ssl/t/002_scram.pl, so I left those out. Anyway, the
attached update applies cleanly to today's HEAD, and the openssl part
still compiles cleanly and passes regression tests. The securetransport
part compiles cleanly, but it fails 8 of the 68 tests in 001_ssltests.pl.
I'm not sure how many of those might be new and how many were there as
of the previous submission.

> The "-framework" option, being added to CFLAGS, is clang specific. I
> think we need some more autoconf magic, to make this work with gcc.

AFAIK, Apple's "gcc" *is* clang; it certainly has no problem with
these switches (and I rather doubt there's any hope of linking to
Secure Transport without 'em). However, I agree that the technique
of teaching random makefiles about this explicitly is mighty ugly,
and we ought to put the logic into configure instead, if possible.
Possibly it could be modeled on LDAP_LIBS or ICU_LIBS, ie configure
sets up a macro that pulls in the openssl libraries, or the
secure transport libraries, or $other-implementation, or nothing.
The CFLAGS hacks need similar treatment (btw, should they be
CPPFLAGS hacks instead? I think they're morally equivalent to
-I switches). And avoid using "override" if at all possible.

Some other comments:

* I notice that contrib/sslinfo hasn't been touched. That probably
ought to be on the to-do list for this, though I don't insist that
it needs to be in the first commit.

* I'm not sure what the "keychain" additions to test/ssl/Makefile
are for, but they don't seem to be wired up to anything. Running
"make keychains" has no impact on the test failures, either.

* I do not like making the libpq connection parameters for this be
#ifdef'd out when the option isn't selected. I believe project policy is
that we accept all parameters always, and either ignore unsupported ones,
or throw errors if they're set to nondefault values (cf. the comment above
the sslmode parameter in fe-connect.c). I realize that some earlier
patches like GSSAPI did not get the word on this, but that's not a reason
to emulate their mistake. I'm not sure about the equivalent decision
w.r.t. backend GUCs, but we need to figure that out.

* In place of changes like
-#ifdef USE_SSL
+#if defined(USE_SSL) && defined(USE_OPENSSL)
I'd be inclined to just do "#ifdef USE_OPENSSL", ie assume that macro
can't be set without USE_SSL.

regards, tom lane

> On 26 Sep 2018, at 01:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Heikki Linnakangas <hlinnaka(at)iki(dot)fi> writes:
>> On 27/06/18 21:57, Daniel Gustafsson wrote:
>>> Courtesy of the ever-present Murphy I managed to forget some testcode in
>>> src/backend/Makefile which broke compilation for builds without secure
>>> transport, attached v8 patch fixes that.
>
>> I've read through this patch now in more detail. Looks pretty good, but
>> I have a laundry list of little things below. The big missing item is
>> documentation.
>
> I did some simple testing on this today. The patch has bit-rotted,
> mostly as a consequence of 77291139c which removed tls-unique channel
> binding. That's probably a good thing for the Secure Transport code,
> since it wasn't supporting that anyway, but it needs to be updated.

Thanks for taking a look!

> I ripped out the failing-to-compile code (maybe that was too simplistic?)
> but could not figure out whether there was anything still useful about
> the diffs in ssl/t/002_scram.pl, so I left those out. Anyway, the
> attached update applies cleanly to today's HEAD, and the openssl part
> still compiles cleanly and passes regression tests. The securetransport
> part compiles cleanly, but it fails 8 of the 68 tests in 001_ssltests.pl.
> I'm not sure how many of those might be new and how many were there as
> of the previous submission.

I had some local diffs that I hacked up based on Heikkis review this summer,
but I never got around to sending them out. I’ve rebased these changes on top
of your v9 patch as the attached v10. With this patch I get 5 failing tests on
High Sierra.

>> The "-framework" option, being added to CFLAGS, is clang specific. I
>> think we need some more autoconf magic, to make this work with gcc.
>
> AFAIK, Apple's "gcc" *is* clang; it certainly has no problem with
> these switches (and I rather doubt there's any hope of linking to
> Secure Transport without 'em).

That is correct, there is however q legitimate question as to how to support
those who install an actual gcc via for example homebrew. -framework is a
linker option which according to the GCC docs isn’t being passed to the linker
by GCC. AFAICT from reading, -framework is however merely a convenient way of
using -l, so it should be doable to just use normal -l for both clang and gcc,
though I haven’t researched that. Another option is to require a macOS
clang(gcc) for secure transport support, at least initially.

It’s not clear to me just how common it is to use GCC via homebrew on macOS.

> However, I agree that the technique
> of teaching random makefiles about this explicitly is mighty ugly,
> and we ought to put the logic into configure instead, if possible.
> Possibly it could be modeled on LDAP_LIBS or ICU_LIBS, ie configure
> sets up a macro that pulls in the openssl libraries, or the
> secure transport libraries, or $other-implementation, or nothing.
> The CFLAGS hacks need similar treatment (btw, should they be
> CPPFLAGS hacks instead? I think they're morally equivalent to
> -I switches). And avoid using "override" if at all possible.

Agreed, thats a better idea.

> Some other comments:
>
> * I notice that contrib/sslinfo hasn't been touched. That probably
> ought to be on the to-do list for this, though I don't insist that
> it needs to be in the first commit.

Right, that one is on the todo.

> * I'm not sure what the "keychain" additions to test/ssl/Makefile
> are for, but they don't seem to be wired up to anything. Running
> "make keychains" has no impact on the test failures, either.

Since keychain files are binary blobs the make target is intended to create the
required keychain files out of the existing certificate/keys for use with the
SSL tests. Generating keychains needs to be wired to running the tests on
secure transport enabled systems (iff we want to have tests for keychains of
course).

> * I do not like making the libpq connection parameters for this be
> #ifdef'd out when the option isn't selected. I believe project policy is
> that we accept all parameters always, and either ignore unsupported ones,
> or throw errors if they're set to nondefault values (cf. the comment above
> the sslmode parameter in fe-connect.c). I realize that some earlier
> patches like GSSAPI did not get the word on this, but that's not a reason
> to emulate their mistake. I'm not sure about the equivalent decision
> w.r.t. backend GUCs, but we need to figure that out.

Makes sense. I added these just after the #if defined(ENABLE_GSS) lines and
just took a page from that playbook. Haven’t changed in the attached but agree
it should be done.

> * In place of changes like
> -#ifdef USE_SSL
> +#if defined(USE_SSL) && defined(USE_OPENSSL)
> I'd be inclined to just do "#ifdef USE_OPENSSL", ie assume that macro
> can't be set without USE_SSL.

Good point, fixed in the attached.

Also, below are responses to the laundrylist raised by Heikki upthread:

> --- laundry list begins ---
>
> What exactly does 'ssl_is_server_start' mean? I don't understand that mechanism. ISTM it's only checked in load_key(), via be_tls_open_server(), which should only be called after be_tls_init(), in which case it's always 'true' when it's checked. Should there be an Assertion on it or something?

Correct, this was a broken attempt at handling the server reload that didn’t
work due to brainfade. Removed and tidied up.

> In be_tls_open_server(), I believe there are several cases of using variables uninitialized. For example, load_identity_keychain() doesn't always set the 'identity' output parameter, but there's an "if (identity == NULL)" check after the call to it. And 'certificates' is left uninitialized, if load_identity_keychain() is used. Also, 'dh_len' is used uninitialized here in the "if (!dh_buf || dh_len == 0)" line, if the 'ssl_dh_params_file' option is not set. Did clang not give warnings about these?’

Nope, no warnings were issueed. I do agree with your findings and have fixed
these and looked for others.

>> + /*
>> + * Certificate Revocation List are not supported in the Secure Transport
>> + * API
>> + */
>
> The corresponding client code has a longer comment on that:
>
>> + /*
>> + * There is no API to load CRL lists in Secure Transport, they can however
>> + * be imported into a Keychain with the commandline application "certtool".
>> + * For libpq to use them, the certificate/key and root certificate needs to
>> + * be using an identity in a Keychain into which the CRL have been
>> + * imported. That needs to be documented.
>> + */
>
> Is it possible to programmatically create a temporary keychain, in memory, and load the CRL to it? (I googled a bit, and couldn't find any suitable API for it, so I guess not..)

I attempted this first using the lowlevel CSSM/CDSA APIs, but deleted it again
as it turned into well over 1500 lines of invoking undocumented CSSM API calls.
And it wasn’t event a complete support at that point. No official APIs exist
for dealing with CRL files. Having the user load the CRL into the Keychain is
about the only thing we can do.

>> + if (ssl_crl_file[0])
>> + ereport(FATAL,
>> + (errmsg("CRL files not supported with Secure Transport")));
>
> I think this should be COMMERROR, like other errors around this. We don't want to pass that error message to the client. Although I guess it won't reach the client as we haven't negotiated TLS yet.

Fixed.

>> + /*
>> + * We use kTryAuthenticate here since we don't know which sslmode the
>> + * client is using. If we were to use kAlwaysAuthenticate then sslmode
>> + * require won't work as intended.
>> + */
>> + if (ssl_loaded_verify_locations)
>> + SSLSetClientSideAuthenticate((SSLContextRef) port->ssl, kTryAuthenticate);
>
> That comment sounds wrong. This is in backend code, and SSLSetClientSideAuthenticate() is all about checking the client's certificate in the server, while libpq 'sslmode' is about checking the server's certificate in the client.

Fixed.

>> + for (;;)
>> + {
>> + status = SSLHandshake((SSLContextRef) port->ssl);
>> + if (status == noErr)
>> + break;
>> +
>> + if (status == errSSLWouldBlock)
>> + continue;
>> + ...
>> + }
>
> Does this busy-wait, if there's not enough data from the client? That seems bad. In the corresponding client-side code, there's a comment on that too, but it's still bad.

It does busy-wait, and was doing so because I had misunderstood the Secure
Transport documentation. I’ve fixed it now similarly to how the OpenSSL code
handles it. Fixing it client-side remains a TODO.

> In be_tls_get_version and PQsslAttribute, can we add support for kTLSProtocol13? Perhaps with an autoconf test, if the constant is not available on all macOS versions.

Good point, I’ve added an autoconf test for it with an ifdef around using it in
the code.

> In be_tls_get_peerdn_name, wouldn't SecCertificateCopyCommonName() be more appropriate than SecCertificateCopyLongDescription()?

The OpenSSL support use X509_get_subject_name() which afaict returns the full
subject and not just the CN, so SecCertificateCopyLongDescription() should be
the equivalent.

> In be_tls_get_cipher, returning "unknown" would seem better than erroring out, if the cipher isn't recognized.

Fixed.

> Check error message style. eg.:
>
>> + ereport(COMMERROR,
>> + (errmsg("could not load server certificate \"%s\": \"%s\"",
>> + ssl_cert_file, pg_SSLerrmessage(status))));
>
> Why is the result of pg_SSLerrmessage() in quotes? Maybe errdetail?

I really don’t know why I had put it in quotes, but it was clearly not a good
idea. Fixed all occurrences to not use quotes, but kept them in errmsg() for
now.

>> + * Any consumers of the Keychain array should always call this to ensure that
>> + * it is set up in a manner that reflect the configuration. If it not, then
>
> s/reflect/reflects/

Fixed.

>> + else if (keychain_count == 2)
>> + {
>> + if (ssl_keychain_file[0] == '\0' || !ssl_keychain_use_default)
>> + goto error;
>> +
>> + return;
>> + }
>> + else
>> + /* Fallthrough to erroring out */
>> +
>> +error:
>> + ereport(FATAL,
>> + (errmsg("Incorrect loading of Keychains detected")));
>> +}
>
> That fallthrough looks dangerous.

Fixed and refactored the function to be clearer while at it. The default for
the ssl_keychain_use_default GUC is set to false, which seems a safe bet.
During hacking I’ve temporarily set it to ‘on’ in the testsuite though, the
keychain setup for the tests needs to be figured out (as in how and what do we
want to test).

>> --- a/src/backend/utils/misc/postgresql.conf.sample
>> +++ b/src/backend/utils/misc/postgresql.conf.sample
>> @@ -106,6 +106,7 @@
>> #ssl_dh_params_file = ''
>> #ssl_passphrase_command = ''
>> #ssl_passphrase_command_supports_reload = off
>> +#ssl_keychain_file = ''
>
> Do we want to have this Secure Transport-specific option in the sample config file? Comment at least

On second thought I don’t think we do, so removed this.

>> +#elif USE_SECURETRANSPORT
>> + void *ssl;
>> + int ssl_buffered;
>
> Add comments. Mention the actual type of 'ssl' (SSLContextRef), and what 'ssl_buffered' means.

Fixed.

> libpq-be.h and libpq-int.h use a different strategy to avoid clashes between PostgreSQL's and Secure Transport's versions of Size and uint64. Let's be consistent. (I like the libpq-fe.h version more, i.e. using "void *" and avoiding the #include)

Yeah, I’ve been meaning to fix that but clearly forgot. Fixed now.

>> - * SSL implementation (e.g. be-secure-openssl.c)
>> + * SSL implementation (e.g. be-secure-<implementation>.c)
>
> Since this is just an example, I think leaving it as be-secure-openssl.c is fine.

Fixed.

> Also update pg_config.h.win32 with the new USE_SECURETRANSPOT flag being added to pg_config.h.in.

Fixed.

> In the call to SSLSetPeerDomainName(), should check return code.

Fixed.

>> + /*
>> + * In sslmode "require" we accept some certificate verification
>> + * failures when we don't have a rootcert since MITM protection
>> + * isn't enforced. Check the reported failure and trust in case
>> + * the cert is missing, self signed or expired/future.
>> + */
>> + if (strcmp(conn->sslmode, "require") == 0 && !conn->st_rootcert)
>> + {
>
> Not just "require", but "allow" as well, right?
>
> freePGconn should free conn->keychain. Would be good to write a little test that opens & closes millions of connectins, to check for memory leaks.

Fixed (the free part, not the util but I agree it would be interesting).

>> + * Queries the specified Keychain, or the default unless not defined, for a
>
> "unless not defined" is a double-negative. I don't understand that sentence. (there are two copies of the same comment in the patch, in FE and BE. And the FE function is called "load_identity_keychain", but its comment says "import_identity_keychain” )

Fixed.

> PQinitOpenSSL and PQinitSSL are inconsistent in whether to call pgtls_init_library(), when compiled with Secure Transport. pgtls_init_library() is a no-op, so it doesn't matter, but let's be consistent. Perhaps do "#define pgtls_init_library() ((void)true)" in the header?

Fixed.

> s/readiblitity/readability/
> s/dont/don't/
> s/securetransport_common.h/securetransport.h/
> s/f.e/for example/ (at least I'm not familiar with that abbreviation)

Fixed.

cheers ./daniel

On 26/09/2018 23:19, Daniel Gustafsson wrote:
> It’s not clear to me just how common it is to use GCC via homebrew on macOS.

I use that all the time.

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> On 26/09/2018 23:19, Daniel Gustafsson wrote:
>> It’s not clear to me just how common it is to use GCC via homebrew on macOS.

> I use that all the time.

Hm, so did 5e2217131 break anything for you? Does that version of gcc
claim to know -F or -framework switches?

regards, tom lane

On 02/10/2018 15:40, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> On 26/09/2018 23:19, Daniel Gustafsson wrote:
>>> It’s not clear to me just how common it is to use GCC via homebrew on macOS.
>
>> I use that all the time.
>
> Hm, so did 5e2217131 break anything for you? Does that version of gcc
> claim to know -F or -framework switches?

This is not a problem. The Python and Tcl build flags have included
-framework switches since time immemorial. (I suspect the compiler just
treats them as linker switches like -l and -L and passes them on.)

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

> On 26 Sep 2018, at 23:19, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> I’ve rebased these changes on top of your v9 patch as the attached v10.

Attached is a v11 rebased on top of todays HEAD, which had minor conflicts due
to the recent snprintf work. No functional changes are introduced over v10.

cheers ./daniel

> On Sun, Oct 28, 2018 at 11:42 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 26 Sep 2018, at 23:19, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > I’ve rebased these changes on top of your v9 patch as the attached v10.
>
> Attached is a v11 rebased on top of todays HEAD, which had minor conflicts due
> to the recent snprintf work. No functional changes are introduced over v10.

Thank you,

Unfortunately, patch needs to be fixed - it doesn't pass "make -C ssl check"

t/001_ssltests.pl .. 1/65 Bailout called. Further testing stopped:
system pg_ctl failed
FAILED--Further testing stopped: system pg_ctl failed

Could you post an updated version?

On Fri, Nov 30, 2018 at 02:00:00PM +0100, Dmitry Dolgov wrote:
> Unfortunately, patch needs to be fixed - it doesn't pass "make -C ssl check"
>
> t/001_ssltests.pl .. 1/65 Bailout called. Further testing stopped:
> system pg_ctl failed
> FAILED--Further testing stopped: system pg_ctl failed
>
> Could you post an updated version?

This has not been answered yet, and a couple of months have gone by,
so I am marking the patch as returned with feedback.
--
Michael