Re: Experiments with Postgres and SSL

On 31/03/2023 10:59, Greg Stark wrote:
> IIRC I put a variable labeled a "GUC" but forgot to actually make it a
> GUC. But I'm thinking of maybe removing that variable since I don't
> see much of a use case for controlling this manually. I *think* ALPN
> is supported by all the versions of OpenSSL we support.

+1 on removing the variable. Let's make ALPN mandatory for direct SSL
connections, like Jacob suggested. And for old-style handshakes, accept
and check ALPN if it's given.

I don't see the point of the libpq 'sslalpn' option either. Let's send
ALPN always.

Admittedly having the options make testing different of combinations of
old and new clients and servers a little easier. But I don't think we
should add options for the sake of backwards compatibility tests.

> --- a/src/backend/libpq/pqcomm.c
> +++ b/src/backend/libpq/pqcomm.c
> @@ -1126,13 +1126,16 @@ pq_discardbytes(size_t len)
> /* --------------------------------
> * pq_buffer_has_data - is any buffered data available to read?
> *
> - * This will *not* attempt to read more data.
> + * Actually returns the number of bytes in the buffer...
> + *
> + * This will *not* attempt to read more data. And reading up to that number of
> + * bytes should not cause reading any more data either.
> * --------------------------------
> */
> -bool
> +size_t
> pq_buffer_has_data(void)
> {
> - return (PqRecvPointer < PqRecvLength);
> + return (PqRecvLength - PqRecvPointer);
> }

Let's rename the function.

> /* push unencrypted buffered data back through SSL setup */
> len = pq_buffer_has_data();
> if (len > 0)
> {
> buf = palloc(len);
> if (pq_getbytes(buf, len) == EOF)
> return STATUS_ERROR; /* shouldn't be possible */
> port->raw_buf = buf;
> port->raw_buf_remaining = len;
> port->raw_buf_consumed = 0;
> }
>
> Assert(pq_buffer_has_data() == 0);
> if (secure_open_server(port) == -1)
> {
> ereport(COMMERROR,
> (errcode(ERRCODE_PROTOCOL_VIOLATION),
> errmsg("SSL Protocol Error during direct SSL connection initiation")));
> return STATUS_ERROR;
> }
>
> if (port->raw_buf_remaining > 0)
> {
> ereport(COMMERROR,
> (errcode(ERRCODE_PROTOCOL_VIOLATION),
> errmsg("received unencrypted data after SSL request"),
> errdetail("This could be either a client-software bug or evidence of an attempted man-in-the-middle attack.")));
> return STATUS_ERROR;
> }
> if (port->raw_buf)
> pfree(port->raw_buf);

This pattern is repeated in both callers of secure_open_server(). Could
we move this into secure_open_server() itself? That would feel pretty
natural, be-secure.c already contains the secure_raw_read() function
that reads the 'raw_buf' field.

> const char *
> PQsslAttribute(PGconn *conn, const char *attribute_name)
> {
> ...
>
> if (strcmp(attribute_name, "alpn") == 0)
> {
> const unsigned char *data;
> unsigned int len;
> static char alpn_str[256]; /* alpn doesn't support longer than 255 bytes */
> SSL_get0_alpn_selected(conn->ssl, &data, &len);
> if (data == NULL || len==0 || len > sizeof(alpn_str)-1)
> return NULL;
> memcpy(alpn_str, data, len);
> alpn_str[len] = 0;
> return alpn_str;
> }

Using a static buffer doesn't look right. If you call PQsslAttribute on
two different connections from two different threads concurrently, they
will write to the same buffer. I see that you copied it from the
"key_bits" handlng, but it has the same issue.

--
Heikki Linnakangas
Neon (https://neon.tech)