Horton

Thread

Commits

  1. Use 'void *' for arbitrary buffers, 'uint8 *' for byte arrays

  2. Use more mundane 'int' type for cancel key lengths in libpq

  3. Fix a few oversights in the longer cancel keys patch

  4. Make cancel request keys longer

  1. pgsql: Make cancel request keys longer

    Heikki Linnakangas <heikki.linnakangas@iki.fi> — 2025-04-02T13:43:17Z

    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(-)

  2. Re: pgsql: Make cancel request keys longer

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-08T17:06:33Z

    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.

  3. Re: pgsql: Make cancel request keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-08T19:41:31Z

    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)

  4. Re: pgsql: Make cancel request keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-09T08:53:20Z

    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)

  5. Re: pgsql: Make cancel request keys longer

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-09T09:39:49Z

    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.

  6. Re: pgsql: Make cancel request keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-09T10:28:03Z

    (moving to pgsql-hackers)

On 09/04/2025 12:39, Peter Eisentraut wrote:
> 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.

Here's a patch to change cancellation keys to "uint8 *". I did the same 
for a few other places, namely the new scram_client_key_binary and 
scram_server_key_binary fields in pg_conn, and a few libpq functions 
that started to give compiler warnings after that. There probably would 
be more code that could be changed to follow this convention, but I 
didn't look hard. What do you think?

I'm on the edge with the pg_b64_encode/decode functions, whether they 
should work on "uint8 *" or "void *". On one hand, you do base64 
encoding on a byte array, which would support "uint8 *". But on the 
other hand, you might use it for encoding things with more structure, 
which would support "void *". I went with "void *", mostly out of 
convenience as many of the SCRAM functions that currently use 
pg_b64_encode/decode, use "char *" to represent byte arrays. But 
arguably those should be changed to use "uint8 *" too.

I committed the other parts of your original patch, thanks!

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

  7. Re: pgsql: Make cancel request keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-09T10:46:57Z

    On 09/04/2025 13:28, Heikki Linnakangas wrote:
> On 09/04/2025 12:39, Peter Eisentraut wrote:
>> 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.
> 
> Here's a patch to change cancellation keys to "uint8 *". I did the same 
> for a few other places, namely the new scram_client_key_binary and 
> scram_server_key_binary fields in pg_conn, and a few libpq functions 
> that started to give compiler warnings after that. There probably would 
> be more code that could be changed to follow this convention, but I 
> didn't look hard. What do you think?
> 
> I'm on the edge with the pg_b64_encode/decode functions, whether they 
> should work on "uint8 *" or "void *". On one hand, you do base64 
> encoding on a byte array, which would support "uint8 *". But on the 
> other hand, you might use it for encoding things with more structure, 
> which would support "void *". I went with "void *", mostly out of 
> convenience as many of the SCRAM functions that currently use 
> pg_b64_encode/decode, use "char *" to represent byte arrays. But 
> arguably those should be changed to use "uint8 *" too.

I went around looking a bit more anyway. Here's a patch to change more 
places to use 'uint8' for byte arrays, in SCRAM and MD5 salts and 
digests and such. It's a bit of code churn, but I think it improves 
readability. Especially the SCRAM code sometimes deals with 
base64-encoded string representations of digests and sometimes with 
decoded byte arrays, and it's helpful to use different datatypes for them.

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

  8. Re: pgsql: Make cancel request keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-05-08T19:11:07Z

    On 09/04/2025 13:46, Heikki Linnakangas wrote:
> On 09/04/2025 13:28, Heikki Linnakangas wrote:
>> On 09/04/2025 12:39, Peter Eisentraut wrote:
>>> 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.
>>
>> Here's a patch to change cancellation keys to "uint8 *". I did the 
>> same for a few other places, namely the new scram_client_key_binary 
>> and scram_server_key_binary fields in pg_conn, and a few libpq 
>> functions that started to give compiler warnings after that. There 
>> probably would be more code that could be changed to follow this 
>> convention, but I didn't look hard. What do you think?
>>
>> I'm on the edge with the pg_b64_encode/decode functions, whether they 
>> should work on "uint8 *" or "void *". On one hand, you do base64 
>> encoding on a byte array, which would support "uint8 *". But on the 
>> other hand, you might use it for encoding things with more structure, 
>> which would support "void *". I went with "void *", mostly out of 
>> convenience as many of the SCRAM functions that currently use 
>> pg_b64_encode/decode, use "char *" to represent byte arrays. But 
>> arguably those should be changed to use "uint8 *" too.
> 
> I went around looking a bit more anyway. Here's a patch to change more 
> places to use 'uint8' for byte arrays, in SCRAM and MD5 salts and 
> digests and such. It's a bit of code churn, but I think it improves 
> readability. Especially the SCRAM code sometimes deals with base64- 
> encoded string representations of digests and sometimes with decoded 
> byte arrays, and it's helpful to use different datatypes for them.

Polished this up a tiny bit, and committed.

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

  9. Re: pgsql: Make cancel request keys longer

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-05-08T22:28:24Z

    On Thu, May 8, 2025 at 12:11 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Polished this up a tiny bit, and committed.

Thanks! I think the uint8->int change for cancel_key_len is more than
just cosmetic; it most likely fixes a bug where a key size of 256
wrapped around to 0. I'll double-check that this fixes that later;
I've gotten side-tracked from the protocol stuff a bit.

While I have you, though, is the following just a really complicated
way to say `msgLength - 4`, or is there some other reason to do the
pointer math?

    cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);

--Jacob

  10. Re: pgsql: Make cancel request keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-05-09T06:37:58Z

    On 09/05/2025 01:28, Jacob Champion wrote:
> On Thu, May 8, 2025 at 12:11 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Polished this up a tiny bit, and committed.
> 
> Thanks! I think the uint8->int change for cancel_key_len is more than
> just cosmetic; it most likely fixes a bug where a key size of 256
> wrapped around to 0. I'll double-check that this fixes that later;
> I've gotten side-tracked from the protocol stuff a bit.

True, although I'm pretty sure you'd fail the later cross-check that the 
whole message was consumed. ("message contents do not agree with length 
in message type"). But it's fixed now in any case.

> While I have you, though, is the following just a really complicated
> way to say `msgLength - 4`, or is there some other reason to do the
> pointer math?
> 
>      cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);

Yes, it amounts to 'msgLength - 4'. I agree it looks pretty obscure. The 
way to read it is:

/* full length of the message, including the type code byte and the 
length field itself */
fullMsgLength = 5 + msgLength;

/* number of bytes consumed from the message so far */
lengthConsumed = (conn->inCursor - conn->inStart);

/* the cancel key consumes all the remaining bytes of the message */
cancel_key_len = fullMsgLength - lengthConsumed;

It didn't occur to me that you could write it simply as 'msgLength - 4'. 
That depends on knowing that the preceding fields are exactly 4 bytes 
long, but that's clear enough if we just add a comment on that, see 
attached.

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

  11. Re: pgsql: Make cancel request keys longer

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-05-23T21:59:44Z

    On Thu, May 8, 2025 at 11:38 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> It didn't occur to me that you could write it simply as 'msgLength - 4'.
> That depends on knowing that the preceding fields are exactly 4 bytes
> long, but that's clear enough if we just add a comment on that, see
> attached.

Sorry for the conference delay; this looks fine to me.

One of the side effects of the uint8 change is that the client now
accepts cancel keys up to roughly 30kb. Is that intended?

--Jacob