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

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>, 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-07-17 14:30:40
Message-ID: 732bde95-a196-eef0-b3ea-536d10ddea25@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 27/06/18 21:57, Daniel Gustafsson wrote:
>> On 27 Jun 2018, at 14:32, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
>> Attached is an updated patch for supporting the native macOS Secure Transport
>> library, rebased on top of current master.
>
> 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.

--- laundry list begins ---

What exactly does 'ssl_is_server_start' mean? I don't understand that
mechanism. ISTM it's only checked in load_key(), via
be_tls_open_server(), which should only be called after be_tls_init(),
in which case it's always 'true' when it's checked. Should there be an
Assertion on it or something?

The "-framework" option, being added to CFLAGS, is clang specific. I
think we need some more autoconf magic, to make this work with gcc.

In be_tls_open_server(), I believe there are several cases of using
variables uninitialized. For example, load_identity_keychain() doesn't
always set the 'identity' output parameter, but there's an "if (identity
== NULL)" check after the call to it. And 'certificates' is left
uninitialized, if load_identity_keychain() is used. Also, 'dh_len' is
used uninitialized here in the "if (!dh_buf || dh_len == 0)" line, if
the 'ssl_dh_params_file' option is not set. Did clang not give warnings
about these?

> + /*
> + * Certificate Revocation List are not supported in the Secure Transport
> + * API
> + */

The corresponding client code has a longer comment on that:

> + /*
> + * There is no API to load CRL lists in Secure Transport, they can however
> + * be imported into a Keychain with the commandline application "certtool".
> + * For libpq to use them, the certificate/key and root certificate needs to
> + * be using an identity in a Keychain into which the CRL have been
> + * imported. That needs to be documented.
> + */

Is it possible to programmatically create a temporary keychain, in
memory, and load the CRL to it? (I googled a bit, and couldn't find any
suitable API for it, so I guess not..)

> + if (ssl_crl_file[0])
> + ereport(FATAL,
> + (errmsg("CRL files not supported with Secure Transport")));

I think this should be COMMERROR, like other errors around this. We
don't want to pass that error message to the client. Although I guess it
won't reach the client as we haven't negotiated TLS yet.

> + /*
> + * We use kTryAuthenticate here since we don't know which sslmode the
> + * client is using. If we were to use kAlwaysAuthenticate then sslmode
> + * require won't work as intended.
> + */
> + if (ssl_loaded_verify_locations)
> + SSLSetClientSideAuthenticate((SSLContextRef) port->ssl, kTryAuthenticate);

That comment sounds wrong. This is in backend code, and
SSLSetClientSideAuthenticate() is all about checking the client's
certificate in the server, while libpq 'sslmode' is about checking the
server's certificate in the client.

> + for (;;)
> + {
> + status = SSLHandshake((SSLContextRef) port->ssl);
> + if (status == noErr)
> + break;
> +
> + if (status == errSSLWouldBlock)
> + continue;
> + ...
> + }

Does this busy-wait, if there's not enough data from the client? That
seems bad. In the corresponding client-side code, there's a comment on
that too, but it's still bad.

In be_tls_get_version and PQsslAttribute, can we add support for
kTLSProtocol13? Perhaps with an autoconf test, if the constant is not
available on all macOS versions.

In be_tls_get_peerdn_name, wouldn't SecCertificateCopyCommonName() be
more appropriate than SecCertificateCopyLongDescription()?

In be_tls_get_cipher, returning "unknown" would seem better than
erroring out, if the cipher isn't recognized.

Check error message style. eg.:

> + ereport(COMMERROR,
> + (errmsg("could not load server certificate \"%s\": \"%s\"",
> + ssl_cert_file, pg_SSLerrmessage(status))));
>

Why is the result of pg_SSLerrmessage() in quotes? Maybe errdetail?

> + * Any consumers of the Keychain array should always call this to ensure that
> + * it is set up in a manner that reflect the configuration. If it not, then

s/reflect/reflects/

> + else if (keychain_count == 2)
> + {
> + if (ssl_keychain_file[0] == '\0' || !ssl_keychain_use_default)
> + goto error;
> +
> + return;
> + }
> + else
> + /* Fallthrough to erroring out */
> +
> +error:
> + ereport(FATAL,
> + (errmsg("Incorrect loading of Keychains detected")));
> +}

That fallthrough looks dangerous.

> --- a/src/backend/utils/misc/postgresql.conf.sample
> +++ b/src/backend/utils/misc/postgresql.conf.sample
> @@ -106,6 +106,7 @@
> #ssl_dh_params_file = ''
> #ssl_passphrase_command = ''
> #ssl_passphrase_command_supports_reload = off
> +#ssl_keychain_file = ''

Do we want to have this Secure Transport-specific option in the sample
config file? Comment at least

> +#elif USE_SECURETRANSPORT
> + void *ssl;
> + int ssl_buffered;

Add comments. Mention the actual type of 'ssl' (SSLContextRef), and what
'ssl_buffered' means.

libpq-be.h and libpq-int.h use a different strategy to avoid clashes
between PostgreSQL's and Secure Transport's versions of Size and uint64.
Let's be consistent. (I like the libpq-fe.h version more, i.e. using
"void *" and avoiding the #include)

> - * SSL implementation (e.g. be-secure-openssl.c)
> + * SSL implementation (e.g. be-secure-<implementation>.c)

Since this is just an example, I think leaving it as be-secure-openssl.c
is fine.

Also update pg_config.h.win32 with the new USE_SECURETRANSPOT flag being
added to pg_config.h.in.

In the call to SSLSetPeerDomainName(), should check return code.

> + /*
> + * In sslmode "require" we accept some certificate verification
> + * failures when we don't have a rootcert since MITM protection
> + * isn't enforced. Check the reported failure and trust in case
> + * the cert is missing, self signed or expired/future.
> + */
> + if (strcmp(conn->sslmode, "require") == 0 && !conn->st_rootcert)
> + {

Not just "require", but "allow" as well, right?

freePGconn should free conn->keychain. Would be good to write a little
test that opens & closes millions of connectins, to check for memory leaks.

> + * Queries the specified Keychain, or the default unless not defined, for a

"unless not defined" is a double-negative. I don't understand that
sentence. (there are two copies of the same comment in the patch, in FE
and BE. And the FE function is called "load_identity_keychain", but its
comment says "import_identity_keychain" )

PQinitOpenSSL and PQinitSSL are inconsistent in whether to call
pgtls_init_library(), when compiled with Secure Transport.
pgtls_init_library() is a no-op, so it doesn't matter, but let's be
consistent. Perhaps do "#define pgtls_init_library() ((void)true)" in
the header?

s/readiblitity/readability/
s/dont/don't/
s/securetransport_common.h/securetransport.h/
s/f.e/for example/ (at least I'm not familiar with that abbreviation)

--- end of laundry list ---

- Heikki

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Grigory Smolkin 2018-07-17 14:33:00 Another fun fact about temp tables and wraparound
Previous Message Dean Rasheed 2018-07-17 13:28:14 Re: PG 10: could not generate random cancel key