Re: Make query cancellation keys longer

I'm back to working on the main patch here, to make cancellation keys 
longer. New rebased version attached, with all the FIXMEs and TODOs from 
the earlier version fixed. There was a lot of bitrot, too.

The first patch now introduces timingsafe_bcmp(), a function borrowed 
from OpenBSD to perform a constant-time comparison. There's a configure 
check to use the function from the OS if it's available, and includes a 
copy of OpenBSD's implementation otherwise. Similar functions exist with 
different names in OpenSSL (CRYPTO_memcmp) and NetBSD 
(consttime_memequal), but it's a pretty simple function so I don't think 
we need to work too hard to pick up those other native implementations.

I used it for checking if the cancellation key matches, now that it's 
not a single word anymore. It feels paranoid to worry about timing 
attacks here, a few instructions is unlikely to give enough signal to an 
attacker and query cancellation is not a very interesting target anyway. 
But better safe than sorry. You can still get information about whether 
a backend with the given PID exists at all, the constant-time comparison 
only applies to comparing the key. We probably should be using this in 
some other places in the backend, but I haven't gone around looking for 
them.

> Hmm, looking at the current code, I do agree that not introducing a
> new message would simplify both client and server implementation. Now
> clients need to do different things depending on if the server
> supports 3.1 or 3.0 (sending CancelRequestExtended instead of
> CancelRequest and having to parse BackendKeyData differently). And I
> also agree that the extra length field doesn't add much in regards to
> sanity checking (for the CancelRequest and BackendKeyData message
> types at least). So, sorry for the back and forth on this, but I now
> agree with you that we should not add the length field. I think one
> reason I didn't see the benefit before was because the initial patch
> 0002 was still introducing a CancelRequestExtended message type. If we
> can get rid of this message type by not adding a length, then I think
> that's worth losing the sanity checking.

Ok, I went back to the original scheme that just redefines the secret 
key in the CancelRequest message to be variable length, with the length 
deduced from the message length.

>> We could teach
>> libpq to disconnect and reconnect with minor protocol version 3.0, if
>> connecting with 3.1 fails, but IMHO it's better to not complicate this
>> and accept the break in backwards-compatibility.
> 
> Yeah, implementing automatic reconnection seems a bit overkill to me
> too. But it might be nice to add a connection option that causes libpq
> to use protocol 3.0. Having to install an old libpq to connect to an
> old server seems quite annoying.

Added a "protocol_version" libpq option for that. It defaults to "auto", 
but you can set it to "3.1" or "3.0" to force the version. It makes it 
easier to test that the backwards-compatibility works, too.

> Especially since I think that many other types of servers that
> implement the postgres protocol have not implemented the minor
> version negotiation.
> 
> I at least know PgBouncer[1] and pgcat[2] have not, but probably
> other server implementations like CockroachDB and Google Spanner have
> this problem too.
Good point.

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