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

[Michael Paquier]

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


signature.asc
Description: PGP signature

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

[Dmitry Dolgov]

> On Sun, Oct 28, 2018 at 11:42 PM Daniel Gustafsson  wrote:
>
> > On 26 Sep 2018, at 23:19, Daniel Gustafsson  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?

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

[Peter Eisentraut]

On 02/10/2018 15:40, Tom Lane wrote:
> Peter Eisentraut  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

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

[Tom Lane]

Peter Eisentraut  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

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

[Peter Eisentraut]

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

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

[Tom Lane]

Heikki Linnakangas  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

diff --git a/configure b/configure
index 23ebfa8..6bdc96a 100755
--- a/configure
+++ b/configure
@@ -708,6 +708,7 @@ UUID_EXTRA_OBJS
 with_uuid
 with_systemd
 with_selinux
+with_securetransport
 with_openssl
 with_ldap
 with_krb_srvnam
@@ -854,6 +855,7 @@ with_bsd_auth
 with_ldap
 with_bonjour
 with_openssl
+with_securetransport
 with_selinux
 with_systemd
 with_readline
@@ -1554,6 +1556,7 @@ Optional Packages:
   --with-ldap build with LDAP support
   --with-bonjour  build with Bonjour support
   --with-openssl  build with OpenSSL support
+  --with-securetransport  build with Apple Secure Transport support
   --with-selinux  build with SELinux support
   --with-systemd  build with systemd support
   --without-readline  do not use GNU Readline nor BSD Libedit for editing
@@ -7968,6 +7971,41 @@ $as_echo "$with_openssl" >&6; }
 
 
 #
+# Apple Secure Transport
+#
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with Apple Secure Transport support" >&5
+$as_echo_n "checking whether to build with Apple Secure Transport support... " >&6; }
+
+
+
+# Check whether --with-securetransport was given.
+if test "${with_securetransport+set}" = set; then :
+  withval=$with_securetransport;
+  case $withval in
+yes)
+
+$as_echo "#define USE_SECURETRANSPORT 1" >>confdefs.h
+
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-securetransport option" "$LINENO" 5
+  ;;
+  esac
+
+else
+  with_securetransport=no
+
+fi
+
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_securetransport" >&5
+$as_echo "$with_securetransport" >&6; }
+
+
+#
 # SELinux
 #
 { $as_echo 

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

[Heikki Linnakangas]

On 27/06/18 21:57, Daniel Gustafsson wrote:

On 27 Jun 2018, at 14:32, Daniel Gustafsson  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,
+   

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

[Daniel Gustafsson]

> On 06 Mar 2018, at 22:08, Tom Lane  wrote:
> 
> Peter Eisentraut  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

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

[Tom Lane]

Peter Eisentraut  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

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

[Peter Eisentraut]

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

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

[Andres Freund]

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

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

[Daniel Gustafsson]

> On 05 Mar 2018, at 02:41, Michael Paquier  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

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

[Michael Paquier]

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


signature.asc
Description: PGP signature

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

[Daniel Gustafsson]

> On 02 Mar 2018, at 03:10, Andres Freund  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

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

[Andres Freund]

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

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

[Peter Eisentraut]

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-8489b3b67...@yesql.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

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

[Peter Eisentraut]

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

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

[Daniel Gustafsson]

> On 23 Jan 2018, at 18:20, Peter Eisentraut  
> 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

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

[Peter Eisentraut]

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

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

[Michael Paquier]

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-4c3c3d0af...@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


signature.asc
Description: PGP signature