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

> 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