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)