Re: Experiments with Postgres and SSL - Mailing list pgsql-hackers
From | Heikki Linnakangas |
---|---|
Subject | Re: Experiments with Postgres and SSL |
Date | |
Msg-id | 40d2853c-7754-6444-739e-e68bb3339d03@iki.fi Whole thread Raw |
In response to | Re: Experiments with Postgres and SSL (Greg Stark <stark@mit.edu>) |
Responses |
Re: Experiments with Postgres and SSL
|
List | pgsql-hackers |
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)
pgsql-hackers by date: