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

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