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)
