Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative - Mailing list pgsql-hackers
From | Daniel Gustafsson |
---|---|
Subject | Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative |
Date | |
Msg-id | 511E7956-876F-44A6-8ABC-C6CE3EE862F2@yesql.se Whole thread Raw |
In response to | Re: [HACKERS] Support for Secure Transport SSL library on macOS asOpenSSL alternative (Michael Paquier <michael.paquier@gmail.com>) |
List | pgsql-hackers |
> On 22 Jan 2018, at 02:46, Michael Paquier <michael.paquier@gmail.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@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? 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
pgsql-hackers by date: