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

On 27/06/18 21:57, Daniel Gustafsson wrote:
>> On 27 Jun 2018, at 14:32, Daniel Gustafsson <daniel@yesql.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