Thread: pgsql: Make cancel request keys longer
Make cancel request keys longer Currently, the cancel request key is a 32-bit token, which isn't very much entropy. If you want to cancel another session's query, you can brute-force it. In most environments, an unauthorized cancellation of a query isn't very serious, but it nevertheless would be nice to have more protection from it. Hence make the key longer, to make it harder to guess. The longer cancellation keys are generated when using the new protocol version 3.2. For connections using version 3.0, short 4-bytes keys are still used. The new longer key length is not hardcoded in the protocol anymore, the client is expected to deal with variable length keys, up to 256 bytes. This flexibility allows e.g. a connection pooler to add more information to the cancel key, which might be useful for finding the connection. Reviewed-by: Jelte Fennema-Nio <postgres@jeltef.nl> Reviewed-by: Robert Haas <robertmhaas@gmail.com> (earlier versions) Discussion: https://www.postgresql.org/message-id/508d0505-8b7a-4864-a681-e7e5edfe32aa@iki.fi Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/a460251f0a1ac987f0225203ff9593704da0b1a9 Modified Files -------------- doc/src/sgml/protocol.sgml | 29 +++++- src/backend/storage/ipc/procsignal.c | 23 ++--- src/backend/tcop/backend_startup.c | 55 ++++++----- src/backend/tcop/postgres.c | 15 ++- src/backend/utils/init/globals.c | 5 +- src/backend/utils/init/postinit.c | 2 +- src/include/libpq/pqcomm.h | 8 +- src/include/miscadmin.h | 4 +- src/include/storage/procsignal.h | 14 ++- src/interfaces/libpq/fe-cancel.c | 102 +++++++++++++++++---- src/interfaces/libpq/fe-connect.c | 15 ++- src/interfaces/libpq/fe-protocol3.c | 45 ++++++++- src/interfaces/libpq/libpq-int.h | 7 +- .../modules/libpq_pipeline/t/001_libpq_pipeline.pl | 12 ++- 14 files changed, 252 insertions(+), 84 deletions(-)
On 02.04.25 15:43, Heikki Linnakangas wrote: > Make cancel request keys longer This patch changed the signature of ProcSignal() -ProcSignalInit(bool cancel_key_valid, int32 cancel_key) +ProcSignalInit(char *cancel_key, int cancel_key_len) but did not update the caller in auxprocess.c: ProcSignalInit(false, 0); This gives a warning with clang. While I was looking at this, I suggest to make the first argument void *. This is consistent for passing binary data. Also, I wonder why MyCancelKeyLength is of type uint8 rather than something more mundane like int. There doesn't seem to be any API reason for this type. See attached patch for possible changes.
Attachment
On 08/04/2025 20:06, Peter Eisentraut wrote: > On 02.04.25 15:43, Heikki Linnakangas wrote: >> Make cancel request keys longer > > This patch changed the signature of ProcSignal() > > -ProcSignalInit(bool cancel_key_valid, int32 cancel_key) > +ProcSignalInit(char *cancel_key, int cancel_key_len) > > but did not update the caller in auxprocess.c: > > ProcSignalInit(false, 0); > > This gives a warning with clang. Good catch. I wonder why the cirrus CI didn't complain, it has a step to check for warnings with clang. > While I was looking at this, I suggest to make the first argument void > *. This is consistent for passing binary data. Ok, sure. > Also, I wonder why MyCancelKeyLength is of type uint8 rather than > something more mundane like int. There doesn't seem to be any API > reason for this type. Agreed. The cancel key length is documented to be at most 256 bytes, but that's more of a coincidence, nothing depends on that variable being uint8. > See attached patch for possible changes. Looks good to me. I can commit these tomorrow, or feel free to do it yourself too. Thank you! -- Heikki Linnakangas Neon (https://neon.tech)
On 08/04/2025 22:41, Heikki Linnakangas wrote: > On 08/04/2025 20:06, Peter Eisentraut wrote: >> While I was looking at this, I suggest to make the first argument void >> *. This is consistent for passing binary data. > > Ok, sure. On second thoughts, -1 on that. 'void *' is appropriate for functions like libc's read() or pq_sendbytes(), where the buffer can point to anything. In other words, the caller is expected to have a pointer like 'foobar *', and it gets cast to 'void *' when you call the function. That's not the case with the cancellation key. The cancellation key is just an array of bytes, the caller is expected to pass an array of bytes, not a struct. The right precedent for that are e.g. SCRAM functions in scram-common.h, for example. They use "const uint8 *" for the hashes. I'll switch to "const uint *" everywhere that deals with cancel keys. There are a few more variables elsewhere in the backend and in libpq. > Looks good to me. I can commit these tomorrow, or feel free to do it > yourself too. I'm on this now. -- Heikki Linnakangas Neon (https://neon.tech)
On 09.04.25 10:53, Heikki Linnakangas wrote: > On 08/04/2025 22:41, Heikki Linnakangas wrote: >> On 08/04/2025 20:06, Peter Eisentraut wrote: >>> While I was looking at this, I suggest to make the first argument >>> void *. This is consistent for passing binary data. >> >> Ok, sure. > > On second thoughts, -1 on that. 'void *' is appropriate for functions > like libc's read() or pq_sendbytes(), where the buffer can point to > anything. In other words, the caller is expected to have a pointer like > 'foobar *', and it gets cast to 'void *' when you call the function. > That's not the case with the cancellation key. The cancellation key is > just an array of bytes, the caller is expected to pass an array of > bytes, not a struct. > > The right precedent for that are e.g. SCRAM functions in scram-common.h, > for example. They use "const uint8 *" for the hashes. > > I'll switch to "const uint *" everywhere that deals with cancel keys. > There are a few more variables elsewhere in the backend and in libpq. I was having the same second thoughts overnight. I agree with your conclusion.