From: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative |
Date: | 2018-01-22 09:56:31 |
Message-ID: | 511E7956-876F-44A6-8ABC-C6CE3EE862F2@yesql.se |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> 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
Attachment | Content-Type | Size |
---|---|---|
0001-WIP-Add-support-for-Apple-Secure-Transport-SSL-li-v4.patch | application/octet-stream | 119.7 KB |
0002-Allow-running-SSL-tests-against-different-binar-v4.patch | application/octet-stream | 8.8 KB |
0003-Allow-spaces-in-connectionstrings-v4.patch | application/octet-stream | 1.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2018-01-22 09:57:46 | Re: pgbench - add \if support |
Previous Message | Fabien COELHO | 2018-01-22 09:45:19 | Re: pgbench - add \if support |