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

Heikki Linnakangas <hlinnaka(at)iki(dot)fi> 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