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-middleattack.")));
 
>             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)