Horton

Thread

Commits

  1. Add timingsafe_bcmp(), for constant-time memory comparison

  2. Add missing declarations to pg_config.h.in

  3. docs: Add a new section and a table listing protocol versions

  4. Make cancel request keys longer

  5. libpq: Add min/max_protocol_version connection options

  6. libpq: Handle NegotiateProtocolVersion message differently

  7. docs: Update phrase on message lengths in the protocol

  8. libpq: Trace all NegotiateProtocolVersion fields

  9. libpq: Add PQfullProtocolVersion to exports.txt

  10. Move cancel key generation to after forking the backend

  1. Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-02-29T21:25:43Z

    Currently, 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. The attached patch makes it longer. It is an 
optional protocol feature, so it's fully backwards-compatible with 
clients that don't support longer keys.

If the client requests the "_pq_.extended_query_cancel" protocol 
feature, the server will generate a longer 256-bit cancellation key. 
However, the new longer key length is no longer hardcoded in the 
protocol. The client is expected to deal with variable length keys, up 
to some reasonable upper limit (TODO: document the maximum). This 
flexibility allows e.g. a connection pooler to add more information to 
the cancel key, which could be useful. If the client doesn't request the 
protocol feature, the server generates a 32-bit key like before.

One complication with this was that because we no longer know how long 
the key should be, 4-bytes or something longer, until the backend has 
performed the protocol negotiation, we cannot generate the key in the 
postmaster before forking the process anymore. The first patch here 
changes things so that the cancellation key is generated later, in the 
backend, and the backend advertises the key in the PMSignalState array. 
This is similar to how this has always worked in EXEC_BACKEND mode with 
the ShmemBackendArray, but instead of having a separate array, I added 
fields to the PMSignalState slots. This removes a bunch of 
EXEC_BACKEND-specific code, which is nice.

Any thoughts on this? Documentation is still missing, and there's one 
TODO on adding a portable time-constant memcmp() function; I'll add 
those if there's agreement on this otherwise.

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

  2. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-01T05:19:53Z

    This is a preliminary review. I'll look at this more closely soon.

On Thu, 29 Feb 2024 at 22:26, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> If the client requests the "_pq_.extended_query_cancel" protocol
> feature, the server will generate a longer 256-bit cancellation key.

Huge +1 for this general idea. This is a problem I ran into with
PgBouncer, and had to make some concessions when fitting the
information I wanted into the available bits. Also from a security
perspective I don't think the current amount of bits have stood the
test of time.

+ ADD_STARTUP_OPTION("_pq_.extended_query_cancel", "");

Since this parameter doesn't actually take a value (just empty
string). I think this behaviour makes more sense as a minor protocol
version bump instead of a parameter.

+ if (strcmp(conn->workBuffer.data, "_pq_.extended_query_cancel") == 0)
+ {
+ /* that's ok */
+ continue;
+ }

Please see this thread[1], which in the first few patches makes
pqGetNegotiateProtocolVersion3 actually usable for extending the
protocol. I started that, because very proposed protocol change that's
proposed on the list has similar changes to
pqGetNegotiateProtocolVersion3 and I think we shouldn't make these
changes ad-hoc hacked into the current code, but actually do them once
in a way that makes sense for all protocol changes.

> Documentation is still missing

I think at least protocol message type documentation would be very
helpful in reviewing, because that is really a core part of this
change. Based on the current code I think it should have a few
changes:

+ int cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);
+
+ conn->be_cancel_key = malloc(cancel_key_len);
+ if (conn->be_cancel_key == NULL)

This is using the message length to determine the length of the cancel
key in BackendKey. That is not something we generally do in the
protocol. It's even documented: "Notice that although each message
includes a byte count at the beginning, the message format is defined
so that the message end can be found without reference to the byte
count." So I think the patch should be changed to include the length
of the cancel key explicitly in the message.

[1]: https://www.postgresql.org/message-id/flat/CAGECzQSr2%3DJPJHNN06E_jTF2%2B0E60K%3DhotyBw5wY%3Dq9Wvmt7DQ%40mail.gmail.com#359e4222eb161da37124be1a384f8d92

  3. Re: Make query cancellation keys longer

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-01T14:19:23Z

    On 29.02.24 22:25, Heikki Linnakangas wrote:
> Currently, 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. The attached patch makes it longer. It is an 
> optional protocol feature, so it's fully backwards-compatible with 
> clients that don't support longer keys.

My intuition would be to make this a protocol version bump, not an 
optional feature.  I think this is something that everyone should 
eventually be using, not a niche feature that you explicitly want to 
opt-in for.

> One complication with this was that because we no longer know how long 
> the key should be, 4-bytes or something longer, until the backend has 
> performed the protocol negotiation, we cannot generate the key in the 
> postmaster before forking the process anymore.

Maybe this would be easier if it's a protocol version number change, 
since that is sent earlier than protocol extensions?

  4. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-03T06:59:15Z

    On Fri, 1 Mar 2024 at 15:19, Peter Eisentraut <peter@eisentraut.org> wrote:
> > One complication with this was that because we no longer know how long
> > the key should be, 4-bytes or something longer, until the backend has
> > performed the protocol negotiation, we cannot generate the key in the
> > postmaster before forking the process anymore.
>
> Maybe this would be easier if it's a protocol version number change,
> since that is sent earlier than protocol extensions?

Protocol version and protocol extensions are both sent in the
StartupMessage, so the same complication applies. (But I do agree that
a protocol version bump is more appropriate for this type of change)

  5. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-03T14:27:35Z

    On Fri, 1 Mar 2024 at 06:19, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> This is a preliminary review. I'll look at this more closely soon.

Some more thoughts after looking some more at the proposed changes

+#define EXTENDED_CANCEL_REQUEST_CODE PG_PROTOCOL(1234,5677)

nit: I think the code should be 1234,5679 because it's the newer
message type, so to me it makes more sense if the code is a larger
number.

+ * FIXME: we used to use signal_child. I believe kill() is
+ * maybe even more correct, but verify that.

signal_child seems to be the correct one, not kill. signal_child has
this relevant comment explaining why it's better than plain kill:

 * On systems that have setsid(), each child process sets itself up as a
 * process group leader.  For signals that are generally interpreted in the
 * appropriate fashion, we signal the entire process group not just the
 * direct child process.  This allows us to, for example, SIGQUIT a blocked
 * archive_recovery script, or SIGINT a script being run by a backend via
 * system().

+SendCancelRequest(int backendPID, int32 cancelAuthCode)

I think this name of the function is quite confusing, it's not sending
a cancel request, it is processing one. It sends a SIGINT.

> While we're at it, switch to using atomics in pmsignal.c for the state
> field. That feels easier to reason about than volatile
> pointers.

I feel like this refactor would benefit from being a separate commit.
That would make it easier to follow which change to pmsignal.c is
necessary for what.

+ MyCancelKeyLength = (MyProcPort != NULL &&
MyProcPort->extended_query_cancel) ? MAX_CANCEL_KEY_LENGTH : 4;

I think we should be doing this check the opposite way, i.e. only fall
back to the smaller key when explicitly requested:

+ MyCancelKeyLength = (MyProcPort != NULL &&
MyProcPort->old_query_cancel) ? 4 : MAX_CANCEL_KEY_LENGTH;

That way we'd get the more secure, longer key length for non-backend
processes such as background workers.

+ case EOF:
+ /* We'll come back when there is more data */
+ return PGRES_POLLING_READING;

Nice catch, I'll go steal this for my patchset which adds all the
necessary changes to be able to do a protocol bump[1].

+ int be_pid; /* PID of backend --- needed for XX cancels */

Seems like you accidentally added XX to the comment in this line.

[1]: https://www.postgresql.org/message-id/flat/CAGECzQSr2%3DJPJHNN06E_jTF2%2B0E60K%3DhotyBw5wY%3Dq9Wvmt7DQ%40mail.gmail.com#359e4222eb161da37124be1a384f8d92

  6. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-03T17:27:56Z

    On Sun, 3 Mar 2024 at 15:27, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> + case EOF:
> + /* We'll come back when there is more data */
> + return PGRES_POLLING_READING;
>
> Nice catch, I'll go steal this for my patchset which adds all the
> necessary changes to be able to do a protocol bump[1].

Actually, it turns out your change to return PGRES_POLLING_READING on
EOF is incorrect (afaict). A little bit above there is this code
comment above a check to see if the whole body was received:

 * Can't process if message body isn't all here yet.
 *
 * After this check passes, any further EOF during parsing
 * implies that the server sent a bad/truncated message.
 * Reading more bytes won't help in that case, so don't return
 * PGRES_POLLING_READING after this point.

So I'll leave my patchset as is.

  7. Re: Make query cancellation keys longer

    Bruce Momjian <bruce@momjian.us> — 2024-03-06T01:12:18Z

    On Fri, Mar  1, 2024 at 03:19:23PM +0100, Peter Eisentraut wrote:
> On 29.02.24 22:25, Heikki Linnakangas wrote:
> > Currently, 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. The attached patch makes it longer. It is an
> > optional protocol feature, so it's fully backwards-compatible with
> > clients that don't support longer keys.
> 
> My intuition would be to make this a protocol version bump, not an optional
> feature.  I think this is something that everyone should eventually be
> using, not a niche feature that you explicitly want to opt-in for.

Agreed.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

  8. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-03-08T22:20:19Z

    On 01/03/2024 07:19, Jelte Fennema-Nio wrote:
> I think this behaviour makes more sense as a minor protocol version
> bump instead of a parameter.
Ok, the consensus is to bump the minor protocol version for this. Works 
for me.

> + if (strcmp(conn->workBuffer.data, "_pq_.extended_query_cancel") == 0)
> + {
> + /* that's ok */
> + continue;
> + }
> 
> Please see this thread[1], which in the first few patches makes
> pqGetNegotiateProtocolVersion3 actually usable for extending the
> protocol. I started that, because very proposed protocol change that's
> proposed on the list has similar changes to
> pqGetNegotiateProtocolVersion3 and I think we shouldn't make these
> changes ad-hoc hacked into the current code, but actually do them once
> in a way that makes sense for all protocol changes.

Thanks, I will take a look and respond on that thread.

>> Documentation is still missing
> 
> I think at least protocol message type documentation would be very
> helpful in reviewing, because that is really a core part of this
> change.

Added some documentation. There's more work to be done there, but at 
least the message type descriptions are now up-to-date.

> Based on the current code I think it should have a few changes:
> 
> + int cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);
> +
> + conn->be_cancel_key = malloc(cancel_key_len);
> + if (conn->be_cancel_key == NULL)
> 
> This is using the message length to determine the length of the cancel
> key in BackendKey. That is not something we generally do in the
> protocol. It's even documented: "Notice that although each message
> includes a byte count at the beginning, the message format is defined
> so that the message end can be found without reference to the byte
> count." So I think the patch should be changed to include the length
> of the cancel key explicitly in the message.

The nice thing about relying on the message length was that we could 
just redefine the CancelRequest message to have a variable length 
secret, and old CancelRequest with 4-byte secret was compatible with the 
new definition too. But it doesn't matter much, so added an explicit 
length field.

FWIW I don't think that restriction makes sense. Any code that parses 
the messages needs to have the message length available, and I don't 
think it helps with sanity checking that much. I think the documentation 
is a little anachronistic. The real reason that all the message types 
include enough information to find the message end is that in protocol 
version 2, there was no message length field. The only exception that 
doesn't have that property is CopyData, and it's no coincidence that it 
was added in protocol version 3.

On 03/03/2024 16:27, Jelte Fennema-Nio wrote:
> On Fri, 1 Mar 2024 at 06:19, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>> This is a preliminary review. I'll look at this more closely soon.
> 
> Some more thoughts after looking some more at the proposed changes
> 
> +#define EXTENDED_CANCEL_REQUEST_CODE PG_PROTOCOL(1234,5677)
> 
> nit: I think the code should be 1234,5679 because it's the newer
> message type, so to me it makes more sense if the code is a larger
> number.

Unfortunately 1234,5679 already in use by NEGOTIATE_SSL_CODE. That's why 
I went the other direction. If we want to add this to "the end", it 
needs to be 1234,5681. But I wanted to keep the cancel requests together.

> + * FIXME: we used to use signal_child. I believe kill() is
> + * maybe even more correct, but verify that.
> 
> signal_child seems to be the correct one, not kill. signal_child has
> this relevant comment explaining why it's better than plain kill:
> 
>   * On systems that have setsid(), each child process sets itself up as a
>   * process group leader.  For signals that are generally interpreted in the
>   * appropriate fashion, we signal the entire process group not just the
>   * direct child process.  This allows us to, for example, SIGQUIT a blocked
>   * archive_recovery script, or SIGINT a script being run by a backend via
>   * system().

I changed it to signal the process group if HAVE_SETSID, like 
pg_signal_backend() does. We don't need to signal both the process group 
and the process itself like signal_child() does, because we don't have 
the race condition with recently-forked children that signal_child() 
talks about.

> +SendCancelRequest(int backendPID, int32 cancelAuthCode)
> 
> I think this name of the function is quite confusing, it's not sending
> a cancel request, it is processing one. It sends a SIGINT.

Heh, well, it's sending the cancel request signal to the right backend, 
but I see your point. Renamed to ProcessCancelRequest.

>> While we're at it, switch to using atomics in pmsignal.c for the state
>> field. That feels easier to reason about than volatile
>> pointers.
> 
> I feel like this refactor would benefit from being a separate commit.
> That would make it easier to follow which change to pmsignal.c is
> necessary for what.

Point taken. I didn't do that yet, but it makes sense.

> + MyCancelKeyLength = (MyProcPort != NULL &&
> MyProcPort->extended_query_cancel) ? MAX_CANCEL_KEY_LENGTH : 4;
> 
> I think we should be doing this check the opposite way, i.e. only fall
> back to the smaller key when explicitly requested:
> 
> + MyCancelKeyLength = (MyProcPort != NULL &&
> MyProcPort->old_query_cancel) ? 4 : MAX_CANCEL_KEY_LENGTH;
> 
> That way we'd get the more secure, longer key length for non-backend
> processes such as background workers.

+1, fixed.

On 03/03/2024 19:27, Jelte Fennema-Nio wrote:
> On Sun, 3 Mar 2024 at 15:27, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>> + case EOF:
>> + /* We'll come back when there is more data */
>> + return PGRES_POLLING_READING;
>>
>> Nice catch, I'll go steal this for my patchset which adds all the
>> necessary changes to be able to do a protocol bump[1].
> 
> Actually, it turns out your change to return PGRES_POLLING_READING on
> EOF is incorrect (afaict). A little bit above there is this code
> comment above a check to see if the whole body was received:
> 
>   * Can't process if message body isn't all here yet.
>   *
>   * After this check passes, any further EOF during parsing
>   * implies that the server sent a bad/truncated message.
>   * Reading more bytes won't help in that case, so don't return
>   * PGRES_POLLING_READING after this point.
> 
> So I'll leave my patchset as is.

Yep, thanks.

One consequence of this patch that I didn't mention earlier is that it 
makes libpq incompatible with server version 9.2 and below, because the 
minor version negotiation was introduced in version 9.3. 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. 9.3 was released in 
2013. We dropped pg_dump support for versions older than 9.2 a few years 
ago, this raises the bar for pg_dump to 9.3 as well.

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

  9. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-09T12:32:38Z

    On Fri, 8 Mar 2024 at 23:20, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Added some documentation. There's more work to be done there, but at
> least the message type descriptions are now up-to-date.

Thanks, that's very helpful.

> The nice thing about relying on the message length was that we could
> just redefine the CancelRequest message to have a variable length
> secret, and old CancelRequest with 4-byte secret was compatible with the
> new definition too. But it doesn't matter much, so I added an explicit
> length field.
>
> FWIW I don't think that restriction makes sense. Any code that parses
> the messages needs to have the message length available, and I don't
> think it helps with sanity checking that much. I think the documentation
> is a little anachronistic. The real reason that all the message types
> include enough information to find the message end is that in protocol
> version 2, there was no message length field. The only exception that
> doesn't have that property is CopyData, and it's no coincidence that it
> was added in protocol version 3.

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.

> Unfortunately 1234,5679 already in use by NEGOTIATE_SSL_CODE. That's why
> I went the other direction. If we want to add this to "the end", it
> needs to be 1234,5681. But I wanted to keep the cancel requests together.

Fair enough, I didn't realise that. This whole point is moot anyway if
we're not introducing CancelRequestExtended

> 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. 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.

I'll try to add such a fallback connection option to my patchset soon.

[1]: https://github.com/pgbouncer/pgbouncer/pull/1007
[2]: https://github.com/postgresml/pgcat/issues/706

  10. Re: Make query cancellation keys longer

    Robert Haas <robertmhaas@gmail.com> — 2024-06-05T15:25:48Z

    On Fri, Mar 8, 2024 at 5:20 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> The nice thing about relying on the message length was that we could
> just redefine the CancelRequest message to have a variable length
> secret, and old CancelRequest with 4-byte secret was compatible with the
> new definition too. But it doesn't matter much, so added an explicit
> length field.

I think I liked your original idea better than this one.

> One consequence of this patch that I didn't mention earlier is that it
> makes libpq incompatible with server version 9.2 and below, because the
> minor version negotiation was introduced in version 9.3. 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. 9.3 was released in
> 2013. We dropped pg_dump support for versions older than 9.2 a few years
> ago, this raises the bar for pg_dump to 9.3 as well.

I think we shouldn't underestimate the impact of bumping the minor
protocol version. Minor version negotiation is probably not supported
by all drivers and Jelte says that it's not supported by any poolers,
so for anybody but direct libpq users, there will be some breakage.
Now, on the one hand, as Jelte has said, there's little value in
having a protocol version if we're too afraid to make use of it. But
on the other hand, is this problem serious enough to justify the
breakage we'll cause? I'm not sure. It seems pretty silly to be using
a 32-bit value for this in 2024, but I'm sure some people aren't going
to like it, and they may not all have noticed this thread. On the
third hand, if we do this, it may help to unblock a bunch of other
pending patches that also want to do protocol-related things.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  11. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-04T10:32:37Z

    Here's a new version of the first patch. In the previous version, I 
added the pid cancellation key to pmsignal.c, but on second thoughts, I 
think procsignal.c is a better place. The ProcSignal array already 
contains the pid, we just need to add the cancellation key there.

This first patch just refactors the current code, without changing the 
protocol or length of the cancellation key. I'd like to get this 
reviewed and committed first, and get back to the protocol changes after 
that.

We currently don't do any locking on the ProcSignal array. For query 
cancellations, that's good because a query cancel packet is processed 
without having a PGPROC entry, so we cannot take LWLocks. We could use 
spinlocks though. In this patch, I used memory barriers to ensure that 
we load/store the pid and the cancellation key in a sensible order, so 
that you cannot e.g. send a cancellation signal to a backend that's just 
starting up and hasn't advertised its cancellation key in ProcSignal 
yet. But I think this might be simpler and less error-prone by just 
adding a spinlock to each ProcSignal slot. That would also fix the 
existing race condition where we might set the pss_signalFlags flag for 
a slot, when the process concurrently terminates and the slot is reused 
for a different process. Because of that, we currently have this caveat: 
"... all the signals are such that no harm is done if they're mistakenly 
fired". With a spinlock, we could eliminate that race.

Thoughts?

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

  12. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-04T10:35:11Z

    On 04/07/2024 13:32, Heikki Linnakangas wrote:
> Here's a new version of the first patch. 

Sorry, forgot attachment.

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

  13. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-07-04T10:50:20Z

    On Thu, 4 Jul 2024 at 12:35, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 04/07/2024 13:32, Heikki Linnakangas wrote:
> > Here's a new version of the first patch.
>
> Sorry, forgot attachment.

It seems you undid the following earlier change. Was that on purpose?
If not, did you undo any other earlier changes by accident?

> > +SendCancelRequest(int backendPID, int32 cancelAuthCode)
> >
> > I think this name of the function is quite confusing, it's not sending
> > a cancel request, it is processing one. It sends a SIGINT.
>
> Heh, well, it's sending the cancel request signal to the right backend,
> but I see your point. Renamed to ProcessCancelRequest.

  14. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-04T11:35:00Z

    On 04/07/2024 13:50, Jelte Fennema-Nio wrote:
> On Thu, 4 Jul 2024 at 12:35, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>
>> On 04/07/2024 13:32, Heikki Linnakangas wrote:
>>> Here's a new version of the first patch.
>>
>> Sorry, forgot attachment.
> 
> It seems you undid the following earlier change. Was that on purpose?
> If not, did you undo any other earlier changes by accident?
> 
>>> +SendCancelRequest(int backendPID, int32 cancelAuthCode)
>>>
>>> I think this name of the function is quite confusing, it's not sending
>>> a cancel request, it is processing one. It sends a SIGINT.
>>
>> Heh, well, it's sending the cancel request signal to the right backend,
>> but I see your point. Renamed to ProcessCancelRequest.

Ah, I made that change as part of the second patch earlier, so I didn't 
consider it now.

I don't feel strongly about it, but I think SendCancelRequest() actually 
feels a little better, in procsignal.c. It's more consistent with the 
existing SendProcSignal() function.

There was indeed another change in the second patch that I missed:

> +				/* If we have setsid(), signal the backend's whole process group */
> +#ifdef HAVE_SETSID
> +				kill(-backendPID, SIGINT);
> +#else
>  				kill(backendPID, SIGINT);
> +#endif

I'm not sure that's really required, when sending SIGINT to a backend 
process. A backend process shouldn't have any child processes, and if it 
did, it's not clear what good SIGINT will do them. But I guess it makes 
sense to do it anyway, for consistency with pg_cancel_backend(), which 
also signals the whole process group.

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

  15. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-07-04T12:20:39Z

    On Thu, 4 Jul 2024 at 12:32, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> We currently don't do any locking on the ProcSignal array. For query
> cancellations, that's good because a query cancel packet is processed
> without having a PGPROC entry, so we cannot take LWLocks. We could use
> spinlocks though. In this patch, I used memory barriers to ensure that
> we load/store the pid and the cancellation key in a sensible order, so
> that you cannot e.g. send a cancellation signal to a backend that's just
> starting up and hasn't advertised its cancellation key in ProcSignal
> yet. But I think this might be simpler and less error-prone by just
> adding a spinlock to each ProcSignal slot. That would also fix the
> existing race condition where we might set the pss_signalFlags flag for
> a slot, when the process concurrently terminates and the slot is reused
> for a different process. Because of that, we currently have this caveat:
> "... all the signals are such that no harm is done if they're mistakenly
> fired". With a spinlock, we could eliminate that race.

I think a spinlock would make this thing a whole concurrency stuff a
lot easier to reason about.

+   slot->pss_cancel_key_valid = false;
+   slot->pss_cancel_key = 0;

If no spinlock is added, I think these accesses should still be made
atomic writes. Otherwise data-races on those fields are still
possible, resulting in undefined behaviour. The memory barriers you
added don't prevent that afaict. With atomic operations there are
still race conditions, but no data-races.

Actually it seems like that same argument applies to the already
existing reading/writing of pss_pid: it's written/read using
non-atomic operations so data-races can occur and thus undefined
behaviour too.

-    volatile pid_t pss_pid;
+    pid_t        pss_pid;

Why remove the volatile modifier?

  16. Re: Make query cancellation keys longer

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-04T12:43:20Z

    Hi,

I don't have any immediate feedback regarding this patch, but I'm
wondering about one thing related to cancellations - we talk cancelling
a query, but we really target a PID (or a particular backend, no matter
how we identify it).

I occasionally want to only cancel a particular query, but I don't think
that's really possible - the query may complete meanwhile, and the
backend may even get used for a different user connection (e.g. with a
connection pool)? Or am I missing something important?

Anyway, I wonder if making the cancellation key longer (or variable
length) might be useful for this too - it would allow including some
sort of optional "query ID" in the request, not just the PID. (Maybe
st_xact_start_timestamp would work?)

Obviously, that's not up to this patch, but it's somewhat related.



regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  17. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-07-04T13:31:49Z

    On Thu, 4 Jul 2024 at 14:43, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
> I don't have any immediate feedback regarding this patch, but I'm
> wondering about one thing related to cancellations - we talk cancelling
> a query, but we really target a PID (or a particular backend, no matter
> how we identify it).
>
> I occasionally want to only cancel a particular query, but I don't think
> that's really possible - the query may complete meanwhile, and the
> backend may even get used for a different user connection (e.g. with a
> connection pool)? Or am I missing something important?

No, you're not missing anything. Having the target of the cancel
request be the backend instead of a specific query is really annoying
and can cause all kinds of race conditions. I had to redesign and
complicate the cancellation logic in PgBouncer significantly, to make
sure that one client could not cancel a connection from another client
anymore: https://github.com/pgbouncer/pgbouncer/pull/717

> Anyway, I wonder if making the cancellation key longer (or variable
> length) might be useful for this too - it would allow including some
> sort of optional "query ID" in the request, not just the PID. (Maybe
> st_xact_start_timestamp would work?)

Yeah, some query ID would be necessary. I think we'd want a dedicated
field for it though. Instead of encoding it in the secret. Getting the
xact_start_timestamp would require communication with the server to
get it, which you would like to avoid since the server might be
unresponsive. So I think a command counter that both sides keep track
of would be better. This counter could then be incremented after every
Query and Sync message.

> Obviously, that's not up to this patch, but it's somewhat related.

Yeah, let's postpone more discussion on this until we have the
currently proposed much simpler change in, which only changes the
secret length.

  18. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-24T16:12:21Z

    On 04/07/2024 15:20, Jelte Fennema-Nio wrote:
> On Thu, 4 Jul 2024 at 12:32, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> We currently don't do any locking on the ProcSignal array. For query
>> cancellations, that's good because a query cancel packet is processed
>> without having a PGPROC entry, so we cannot take LWLocks. We could use
>> spinlocks though. In this patch, I used memory barriers to ensure that
>> we load/store the pid and the cancellation key in a sensible order, so
>> that you cannot e.g. send a cancellation signal to a backend that's just
>> starting up and hasn't advertised its cancellation key in ProcSignal
>> yet. But I think this might be simpler and less error-prone by just
>> adding a spinlock to each ProcSignal slot. That would also fix the
>> existing race condition where we might set the pss_signalFlags flag for
>> a slot, when the process concurrently terminates and the slot is reused
>> for a different process. Because of that, we currently have this caveat:
>> "... all the signals are such that no harm is done if they're mistakenly
>> fired". With a spinlock, we could eliminate that race.
> 
> I think a spinlock would make this thing a whole concurrency stuff a
> lot easier to reason about.
> 
> +   slot->pss_cancel_key_valid = false;
> +   slot->pss_cancel_key = 0;
> 
> If no spinlock is added, I think these accesses should still be made
> atomic writes. Otherwise data-races on those fields are still
> possible, resulting in undefined behaviour. The memory barriers you
> added don't prevent that afaict. With atomic operations there are
> still race conditions, but no data-races.
> 
> Actually it seems like that same argument applies to the already
> existing reading/writing of pss_pid: it's written/read using
> non-atomic operations so data-races can occur and thus undefined
> behaviour too.

Ok, here's a version with spinlocks.

I went back and forth on what exactly is protected by the spinlock. I 
kept the "volatile sig_atomic_t" type for pss_signalFlags, so that it 
can still be safely read without holding the spinlock in 
CheckProcSignal, but all the functions that set the flags now hold the 
spinlock. That removes the race condition that you might set the flag 
for wrong slot, if the target backend exits and the slot is recycled. 
The race condition was harmless and there were comments to note it, but 
it doesn't occur anymore with the spinlock.

(Note: Thomas's "Interrupts vs signals" patch will remove ps_signalFlags 
altogether. I'm looking forward to that.)

> -    volatile pid_t pss_pid;
> +    pid_t        pss_pid;
> 
> Why remove the volatile modifier?

Because I introduced a memory barrier to ensure the reads/writes of 
pss_pid become visible to other processes in right order. That makes the 
'volatile' unnecessary IIUC. With the spinlock, the point is moot however.

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

  19. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-29T13:19:15Z

    On 24/07/2024 19:12, Heikki Linnakangas wrote:
> On 04/07/2024 15:20, Jelte Fennema-Nio wrote:
>> On Thu, 4 Jul 2024 at 12:32, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> We currently don't do any locking on the ProcSignal array. For query
>>> cancellations, that's good because a query cancel packet is processed
>>> without having a PGPROC entry, so we cannot take LWLocks. We could use
>>> spinlocks though. In this patch, I used memory barriers to ensure that
>>> we load/store the pid and the cancellation key in a sensible order, so
>>> that you cannot e.g. send a cancellation signal to a backend that's just
>>> starting up and hasn't advertised its cancellation key in ProcSignal
>>> yet. But I think this might be simpler and less error-prone by just
>>> adding a spinlock to each ProcSignal slot. That would also fix the
>>> existing race condition where we might set the pss_signalFlags flag for
>>> a slot, when the process concurrently terminates and the slot is reused
>>> for a different process. Because of that, we currently have this caveat:
>>> "... all the signals are such that no harm is done if they're mistakenly
>>> fired". With a spinlock, we could eliminate that race.
>>
>> I think a spinlock would make this thing a whole concurrency stuff a
>> lot easier to reason about.
>>
>> +   slot->pss_cancel_key_valid = false;
>> +   slot->pss_cancel_key = 0;
>>
>> If no spinlock is added, I think these accesses should still be made
>> atomic writes. Otherwise data-races on those fields are still
>> possible, resulting in undefined behaviour. The memory barriers you
>> added don't prevent that afaict. With atomic operations there are
>> still race conditions, but no data-races.
>>
>> Actually it seems like that same argument applies to the already
>> existing reading/writing of pss_pid: it's written/read using
>> non-atomic operations so data-races can occur and thus undefined
>> behaviour too.
> 
> Ok, here's a version with spinlocks.
> 
> I went back and forth on what exactly is protected by the spinlock. I
> kept the "volatile sig_atomic_t" type for pss_signalFlags, so that it
> can still be safely read without holding the spinlock in
> CheckProcSignal, but all the functions that set the flags now hold the
> spinlock. That removes the race condition that you might set the flag
> for wrong slot, if the target backend exits and the slot is recycled.
> The race condition was harmless and there were comments to note it, but
> it doesn't occur anymore with the spinlock.
> 
> (Note: Thomas's "Interrupts vs signals" patch will remove ps_signalFlags
> altogether. I'm looking forward to that.)
> 
>> -    volatile pid_t pss_pid;
>> +    pid_t        pss_pid;
>>
>> Why remove the volatile modifier?
> 
> Because I introduced a memory barrier to ensure the reads/writes of
> pss_pid become visible to other processes in right order. That makes the
> 'volatile' unnecessary IIUC. With the spinlock, the point is moot however.

I:
- fixed a few comments,
- fixed a straightforward failure with EXEC_BACKEND (ProcSignal needs to 
be passed down in BackendParameters now),
- put back the snippet to signal the whole process group if supported, 
which you pointed out earlier

and committed this refactoring patch.

Thanks for the review!

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

  20. Re: Make query cancellation keys longer

    Thomas Munro <thomas.munro@gmail.com> — 2024-08-12T11:45:44Z

    +     * See if we have a matching backend. Reading the pss_pid and
+     * pss_cancel_key fields is racy, a backend might die and remove itself
+     * from the array at any time.  The probability of the cancellation key
+     * matching wrong process is miniscule, however, so we can live with that.
+     * PIDs are reused too, so sending the signal based on PID is inherently
+     * racy anyway, although OS's avoid reusing PIDs too soon.

Just BTW, we know that Windows sometimes recycles PIDs very soon,
sometimes even immediately, to the surprise of us Unix hackers.  It can
make for some very confusing build farm animal logs.  My patch will
propose to change that particular thing to proc numbers anyway so I'm
not asking for a change here, I just didn't want that assumption to go
un-footnoted.  I suppose that's actually (another) good reason to want
to widen the cancellation key, so that we don't have to worry about
proc number allocation order being less protective than traditional
Unix PID allocation...

  21. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-15T17:13:49Z

    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)

  22. Re: Make query cancellation keys longer

    Robert Haas <robertmhaas@gmail.com> — 2024-08-15T20:20:54Z

    On Thu, Aug 15, 2024 at 1:13 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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.

Over on the "Add new protocol message to change GUCs for usage with
future protocol-only GUCs" there is a lot of relevant discussion about
how bumping the protocol version should work. This thread shouldn't
ignore all that discussion. Just to take one example, Jelte wants to
bump the protocol version to 3.2, not 3.1, for some reasons that are
in the commit message for the relevant patch over there.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  23. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-15T22:07:25Z

    On 15/08/2024 23:20, Robert Haas wrote:
> On Thu, Aug 15, 2024 at 1:13 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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.
> 
> Over on the "Add new protocol message to change GUCs for usage with
> future protocol-only GUCs" there is a lot of relevant discussion about
> how bumping the protocol version should work. This thread shouldn't
> ignore all that discussion. Just to take one example, Jelte wants to
> bump the protocol version to 3.2, not 3.1, for some reasons that are
> in the commit message for the relevant patch over there.

Ok, I've read through that thread now, and opined there too. One 
difference is with libpq option name: My patch adds "protocol_version", 
while Jelte proposes "max_protocol_version". I don't have strong 
opinions on that. I hope the ecosystem catches up to support 
NegotiateProtocolVersion quickly, so that only few people will need to 
set this option. In particular, I hope that there will never be need to 
use "max_protocol_version=3.2", because by the time we introduce version 
3.3, all the connection poolers that support 3.2 will also implement 
NegotiateProtocolVersion.

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

  24. Re: Make query cancellation keys longer

    Robert Haas <robertmhaas@gmail.com> — 2024-08-16T12:31:27Z

    On Thu, Aug 15, 2024 at 6:07 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Ok, I've read through that thread now, and opined there too. One
> difference is with libpq option name: My patch adds "protocol_version",
> while Jelte proposes "max_protocol_version". I don't have strong
> opinions on that. I hope the ecosystem catches up to support
> NegotiateProtocolVersion quickly, so that only few people will need to
> set this option. In particular, I hope that there will never be need to
> use "max_protocol_version=3.2", because by the time we introduce version
> 3.3, all the connection poolers that support 3.2 will also implement
> NegotiateProtocolVersion.

In Jelte's design, there end up being two connection parameters. We
tell the server we want max_protocol_version, but we accept that it
might give us something older. If, however, it tries to degrade us to
something lower than min_protocol_version, we bail out. I see you've
gone for a simpler design: you ask the server for protocol_version and
you get that or you die. To be honest, right up until exactly now, I
was assuming we wanted a two-parameter system like that, just because
being able to tolerate a range of protocol versions seems useful.
However, maybe we don't need it. Alternatively, we could do this for
now, and then later we could adjust the parameter so that you can say
protocol_version=3.2-3.7 and the client will ask for 3.7 but tolerate
anything >= 3.2. Hmm, I kind of like that idea.

I think it's likely that the ecosystem will catch up with
NegotiateProtocolVersion once things start breaking. However, I feel
pretty confident that there are going to be glitches. Clients are
going to want to force newer protocol versions to make sure they get
new features, or to make sure that security features that they want to
have (like this one) are enabled. Some users are going to be running
old poolers that can't handle 3.2, or there will be weirder things
where the pooler says it supports it but it doesn't actually work
properly in all cases. There are also non-PG servers that reimplement
the PG wire protocol. I can't really enumerate all the things that go
wrong, but I think there are a number of wire protocol changes that
various people have been wanting for a long while now, and when we
start to get the infrastructure in place to make that practical,
people are going to take advantage of it. So I think we can expect a
number of protocol enhancements and changes -- Peter's transparent
column encryption stuff is another example -- and there will be
mistakes by us and mistakes by others along the way. Allowing users to
specify what protocol version they want is probably an important part
of coping with that.

The documentation in the patch you attached still seems to think
there's an explicit length field for the cancel key. Also, I think it
would be good to split this into two patches, one to bump the protocol
version and a second to change the cancel key stuff. It would
facilitate review, and I also think that bumping the protocol version
is a big enough deal that it should have its own entry in the commit
log.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  25. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-16T14:37:06Z

    On 16/08/2024 15:31, Robert Haas wrote:
> On Thu, Aug 15, 2024 at 6:07 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Ok, I've read through that thread now, and opined there too. One
>> difference is with libpq option name: My patch adds "protocol_version",
>> while Jelte proposes "max_protocol_version". I don't have strong
>> opinions on that. I hope the ecosystem catches up to support
>> NegotiateProtocolVersion quickly, so that only few people will need to
>> set this option. In particular, I hope that there will never be need to
>> use "max_protocol_version=3.2", because by the time we introduce version
>> 3.3, all the connection poolers that support 3.2 will also implement
>> NegotiateProtocolVersion.
> 
> In Jelte's design, there end up being two connection parameters. We
> tell the server we want max_protocol_version, but we accept that it
> might give us something older. If, however, it tries to degrade us to
> something lower than min_protocol_version, we bail out. I see you've
> gone for a simpler design: you ask the server for protocol_version and
> you get that or you die. To be honest, right up until exactly now, I
> was assuming we wanted a two-parameter system like that, just because
> being able to tolerate a range of protocol versions seems useful.
> However, maybe we don't need it. Alternatively, we could do this for
> now, and then later we could adjust the parameter so that you can say
> protocol_version=3.2-3.7 and the client will ask for 3.7 but tolerate
> anything >= 3.2. Hmm, I kind of like that idea.

Works for me.

If we envision accepting ranges like that in the future, it would be 
good to do now rather than later. Otherwise, if someone wants to require 
features from protocol 3.2 today, they will have to put 
"protocol_version=3.2" in the connection string, and later when 3.3 
version is released, their connection string will continue to force the 
then-old 3.2 version.

> I think it's likely that the ecosystem will catch up with
> NegotiateProtocolVersion once things start breaking. However, I feel
> pretty confident that there are going to be glitches. Clients are
> going to want to force newer protocol versions to make sure they get
> new features, or to make sure that security features that they want to
> have (like this one) are enabled. Some users are going to be running
> old poolers that can't handle 3.2, or there will be weirder things
> where the pooler says it supports it but it doesn't actually work
> properly in all cases. There are also non-PG servers that reimplement
> the PG wire protocol. I can't really enumerate all the things that go
> wrong, but I think there are a number of wire protocol changes that
> various people have been wanting for a long while now, and when we
> start to get the infrastructure in place to make that practical,
> people are going to take advantage of it. So I think we can expect a
> number of protocol enhancements and changes -- Peter's transparent
> column encryption stuff is another example -- and there will be
> mistakes by us and mistakes by others along the way. Allowing users to
> specify what protocol version they want is probably an important part
> of coping with that.

Yes, it's a good escape hatch to have.

> The documentation in the patch you attached still seems to think
> there's an explicit length field for the cancel key.

ok thanks

> Also, I think it
> would be good to split this into two patches, one to bump the protocol
> version and a second to change the cancel key stuff. It would
> facilitate review, and I also think that bumping the protocol version
> is a big enough deal that it should have its own entry in the commit
> log.

Right. That's what Jelte's first patches did too. Those changes are more 
or less the same between this patch and his. These clearly need to be 
merged into one "introduce protocol version 3.2" patch.

I'll split this patch like that, to make it easier to compare and merge 
with Jelte's corresponding patches.

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

  26. Re: Make query cancellation keys longer

    Robert Haas <robertmhaas@gmail.com> — 2024-08-16T15:29:17Z

    On Fri, Aug 16, 2024 at 10:37 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> If we envision accepting ranges like that in the future, it would be
> good to do now rather than later. Otherwise, if someone wants to require
> features from protocol 3.2 today, they will have to put
> "protocol_version=3.2" in the connection string, and later when 3.3
> version is released, their connection string will continue to force the
> then-old 3.2 version.

I'm totally cool with doing it now rather than later if you or someone
else is willing to do the work. But I don't see why we'd need a
protocol bump to change it later. If you write protocol_version=3.7 or
protocol_version=3.2-3.7 we send the same thing to the server either
way. It's only a difference in whether we slam the connection shut if
the server comes back and say it can only do 3.0.

> I'll split this patch like that, to make it easier to compare and merge
> with Jelte's corresponding patches.

That sounds great. IMHO, comparing and merging the patches is the next
step here and would be great to see.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  27. Re: Make query cancellation keys longer

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-09-05T15:43:31Z

    On Thu, Aug 15, 2024 at 10:14 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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.

I have a couple of questions/comments separate from the protocol changes:

Has there been any work/discussion around not sending the cancel key
in plaintext from psql? It's not a prerequisite or anything (the
longer length is a clear improvement either way), but it seems odd
that this longer "secret" is still just going to be exposed on the
wire when you press Ctrl+C.

> 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.

One advantage to using other implementations is that _they're_ on the
hook for keeping constant-time guarantees, which is getting trickier
due to weird architectural optimizations [1]. CRYPTO_memcmp() has
almost the same implementation as 0001 here, except they made the
decision to mark the pointers volatile, and they also provide
hand-crafted assembly versions. This patch has OpenBSD's version, but
they've also turned on data-independent timing by default across their
ARM64 processors [2]. And Intel may require the same tweak, but it
doesn't look like userspace has access to that setting yet, and the
kernel thread [3] appears to have just withered...

For the cancel key implementation in particular, I agree with you that
it's probably not a serious problem. But if other security code starts
using timingsafe_bcmp() then it might be something to be concerned
about. Are there any platform/architecture combos that don't provide a
native timingsafe_bcmp() *and* need a DIT bit for safety?

Thanks,
--Jacob

[1] https://github.com/golang/go/issues/66450
[2] https://github.com/openbsd/src/commit/cf1440f11c
[3] https://lore.kernel.org/lkml/851920c5-31c9-ddd9-3e2d-57d379aa0671@intel.com/

  28. Re: Make query cancellation keys longer

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-05T16:21:29Z

    Jacob Champion <jacob.champion@enterprisedb.com> writes:
> Has there been any work/discussion around not sending the cancel key
> in plaintext from psql? It's not a prerequisite or anything (the
> longer length is a clear improvement either way), but it seems odd
> that this longer "secret" is still just going to be exposed on the
> wire when you press Ctrl+C.

Wasn't this already addressed in v17, by

Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-03-12 [61461a300] libpq: Add encrypted and non-blocking query cancellation

?  Perhaps we need to run around and make sure none of our standard
clients use the old API anymore, but the libpq infrastructure is
there already.

			regards, tom lane

  29. Re: Make query cancellation keys longer

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-09-05T16:33:52Z

    On Thu, Sep 5, 2024 at 9:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Wasn't this already addressed in v17, by
>
> Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
> 2024-03-12 [61461a300] libpq: Add encrypted and non-blocking query cancellation
>
> ?  Perhaps we need to run around and make sure none of our standard
> clients use the old API anymore, but the libpq infrastructure is
> there already.

Right. From a quick grep, it looks like we have seven binaries using
the signal-based cancel handler.

(For programs that only send a cancel request right before they break
the connection, it's probably not worth a huge amount of effort to
change it right away, but for psql in particular I think the status
quo is a little weird.)

Thanks,
--Jacob

  30. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-09-05T16:36:10Z

    On Thu, 5 Sept 2024 at 17:43, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Has there been any work/discussion around not sending the cancel key
> in plaintext from psql? It's not a prerequisite or anything (the
> longer length is a clear improvement either way), but it seems odd
> that this longer "secret" is still just going to be exposed on the
> wire when you press Ctrl+C.

Totally agreed that it would be good to update psql to use the new
much more secure libpq function introduced in PG17[1]. This is not a
trivial change though because it requires refactoring the way we
handle signals (which is why I didn't do it as part of introducing
these new APIs). I had hoped that the work in [2] would either do that
or at least make it a lot easier, but that thread seems to have
stalled. So +1 for doing this, but I think it's a totally separate
change and so should be discussed on a separate thread.

[1]: https://www.postgresql.org/docs/17/libpq-cancel.html#LIBPQ-CANCEL-FUNCTIONS
[2]: https://www.postgresql.org/message-id/flat/20240331222502.03b5354bc6356bc5c388919d%40sraoss.co.jp#1450c8fee45408acaa5b5a1b9a6f70fc

> For the cancel key implementation in particular, I agree with you that
> it's probably not a serious problem. But if other security code starts
> using timingsafe_bcmp() then it might be something to be concerned
> about. Are there any platform/architecture combos that don't provide a
> native timingsafe_bcmp() *and* need a DIT bit for safety?

It sounds to me like we should at least use OpenSSL's CRYPTO_memcmp if
we linked against it and the OS doesn't provide a timingsafe_bcmp.
Would that remove your concerns? I expect anyone that cares about
security to link against some TLS library. That way our "fallback"
implementation is only used on the rare systems where that's not the
case.

  31. Re: Make query cancellation keys longer

    Jacob Champion <jacob.champion@enterprisedb.com> — 2024-09-05T17:08:23Z

    On Thu, Sep 5, 2024 at 9:36 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> Totally agreed that it would be good to update psql to use the new
> much more secure libpq function introduced in PG17[1]. This is not a
> trivial change though because it requires refactoring the way we
> handle signals (which is why I didn't do it as part of introducing
> these new APIs).

Yeah, I figured it wasn't a quick fix.

> I had hoped that the work in [2] would either do that
> or at least make it a lot easier, but that thread seems to have
> stalled. So +1 for doing this, but I think it's a totally separate
> change and so should be discussed on a separate thread.

As long as the new thread doesn't also stall out/get forgotten, I'm happy.

> It sounds to me like we should at least use OpenSSL's CRYPTO_memcmp if
> we linked against it and the OS doesn't provide a timingsafe_bcmp.
> Would that remove your concerns?

If we go that direction, I'd still like to know which platforms we
expect to have a suboptimal port, if for no other reason than
documenting that those users should try to get OpenSSL into their
builds. (I agree that they probably will already, if they care.)

And if I'm being really picky, I'm not sure we should call our port
"timingsafe_bcmp" (vs. pg_secure_bcmp or something) if we know it's
not actually timing-safe for some. But I won't die on that hill.

Thanks,
--Jacob

  32. Re: Make query cancellation keys longer

    Robert Haas <robertmhaas@gmail.com> — 2024-09-09T15:58:34Z

    On Fri, Aug 16, 2024 at 11:29 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > I'll split this patch like that, to make it easier to compare and merge
> > with Jelte's corresponding patches.
>
> That sounds great. IMHO, comparing and merging the patches is the next
> step here and would be great to see.

Heikki, do you have any update on this work?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  33. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-22T14:58:26Z

    On Mon, 9 Sept 2024 at 17:58, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Aug 16, 2024 at 11:29 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > I'll split this patch like that, to make it easier to compare and merge
> > > with Jelte's corresponding patches.
> >
> > That sounds great. IMHO, comparing and merging the patches is the next
> > step here and would be great to see.
>
> Heikki, do you have any update on this work?

My patchset in the other protocol thread needed a rebase. So I took
that as an opportunity to rebase this patchset on top of it, because
this seems to be the protocol change that we can most easily push over
the finish line.

1. I changed the last patch from Heikki to only contain the changes
for the cancel lengthening. The general protocol change related things
I merged with mine (I only kept some error handling and docs).
2. I also removed the length field in the new BackendKey definition,
eventhough I asked for that addition previously. I agree with Heikki
that it's actually easier to parse and create without, because you can
use the same code for both versions.
3. I made our timingsafe_bcmp implementation call into OpenSSL's CRYPTO_memcmp.

One open question on the last patch is: Document what the maximum size
of the cancel key is that the client can expect? I think Jacob might
have some ideas on that.

  34. Re: Make query cancellation keys longer

    Robert Haas <robertmhaas@gmail.com> — 2025-02-24T17:24:01Z

    On Sat, Feb 22, 2025 at 9:58 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> My patchset in the other protocol thread needed a rebase. So I took
> that as an opportunity to rebase this patchset on top of it, because
> this seems to be the protocol change that we can most easily push over
> the finish line.

I'm still not really convinced about that -- see comments upthread --
but I committed 0001 and 0002, which appear to be straightforward bug
fixes.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  35. Re: Make query cancellation keys longer

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-25T14:28:20Z

    On Mon, 24 Feb 2025 at 18:24, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sat, Feb 22, 2025 at 9:58 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > My patchset in the other protocol thread needed a rebase. So I took
> > that as an opportunity to rebase this patchset on top of it, because
> > this seems to be the protocol change that we can most easily push over
> > the finish line.
>
> I'm still not really convinced about that -- see comments upthread --

You've said quite a few things upthread, so I'm going to assume you
are referring to the following. If that's not the case could you
please quote the part of the thread that you think is important:

On Wed, 5 Jun 2024 at 17:26, Robert Haas <robertmhaas@gmail.com> wrote:
> I think we shouldn't underestimate the impact of bumping the minor
> protocol version. Minor version negotiation is probably not supported
> by all drivers and Jelte says that it's not supported by any poolers,
> so for anybody but direct libpq users, there will be some breakage.
> Now, on the one hand, as Jelte has said, there's little value in
> having a protocol version if we're too afraid to make use of it. But
> on the other hand, is this problem serious enough to justify the
> breakage we'll cause? I'm not sure. It seems pretty silly to be using
> a 32-bit value for this in 2024, but I'm sure some people aren't going
> to like it, and they may not all have noticed this thread. On the
> third hand, if we do this, it may help to unblock a bunch of other
> pending patches that also want to do protocol-related things.

So to be clear: the current patchset *does not* change any client
defaults. libpq will still connect to a server using the protocol
version 3.0 by default. It will only connect with 3.2 unless
instructed explicitly in the connection string using
min/max_protocol_version. So no breakage by default is going to
happen, and very few users will hit any breakage at all, because they
won't change the default.

The reason I think that this longer cancel key change is the easiest
protocol change to get through, is precisely because it's a really
minor change that almost no-one actually cares about. Don't get me
wrong, it's useful feature from a security perspective, and for people
implementing poolers, but for both of those it's a relatively minor
benefit.

To me the main feature of doing this change, is to nudge poolers and
other servers to implement the NegotiateProtocolVersion feature. I
think there are two reasons why no server except Postgres has
implemented it:
1. There was no immediate usecase, because there was no new protocol
version in sight that would be useful to them.
2. There was no easy way to test this. They would need to recompile
libpq/psql to be able to send a different protocol version.

Both of those would be addressed by getting this really minor change
in. 1 would be addressed, because this change is a signal that we
intend to do more useful protocol changes in the somewhat near future.
And 2 is addressed because you can now send 2 different protocol
versions with a single psql binary. So by releasing this the ecosystem
is able to catch up to Postgres its NegotiateProtocolVersion support,
which will mean much less breakage in the future when we make a
protocol change that users will actively want to use (e.g. because
it's a feature they care about or makes their queries faster).

> but I committed 0001 and 0002, which appear to be straightforward bug
> fixes.

Nice.

  36. Re: Make query cancellation keys longer

    Robert Haas <robertmhaas@gmail.com> — 2025-02-26T20:18:45Z

    On Tue, Feb 25, 2025 at 9:28 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> So to be clear: the current patchset *does not* change any client
> defaults. libpq will still connect to a server using the protocol
> version 3.0 by default. It will only connect with 3.2 unless
> instructed explicitly in the connection string using
> min/max_protocol_version. So no breakage by default is going to
> happen, and very few users will hit any breakage at all, because they
> won't change the default.

Ah, interesting. I didn't realize that you had done this in a way
where it wasn't enabled by default. I think that's different from what
was proposed before, but maybe I'm not remembering correctly.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  37. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-01T14:08:12Z

    On 22/02/2025 16:58, Jelte Fennema-Nio wrote:
> On Mon, 9 Sept 2024 at 17:58, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Fri, Aug 16, 2024 at 11:29 AM Robert Haas <robertmhaas@gmail.com> wrote:
>>>> I'll split this patch like that, to make it easier to compare and merge
>>>> with Jelte's corresponding patches.
>>>
>>> That sounds great. IMHO, comparing and merging the patches is the next
>>> step here and would be great to see.
>>
>> Heikki, do you have any update on this work?
> 
> My patchset in the other protocol thread needed a rebase. So I took
> that as an opportunity to rebase this patchset on top of it, because
> this seems to be the protocol change that we can most easily push over
> the finish line.
> 
> 1. I changed the last patch from Heikki to only contain the changes
> for the cancel lengthening. The general protocol change related things
> I merged with mine (I only kept some error handling and docs).
> 2. I also removed the length field in the new BackendKey definition,
> eventhough I asked for that addition previously. I agree with Heikki
> that it's actually easier to parse and create without, because you can
> use the same code for both versions.
> 3. I made our timingsafe_bcmp implementation call into OpenSSL's CRYPTO_memcmp.

Great, thanks!

I spent some time going over this again, making tiny fixes here and 
there. A couple of somewhat notable fixes:

- fixed the "server only supports protocol version %d.%d, but 
min_protocol_version was set to %d.%d" message to print the server's 
protocol version correctly
- error out if the server sends two NegotiateProtocolVersion messages 
for some reason
- added a new "protocol versions" subsection to the docs, moving the 
existing paragraphs related to version negotiation there. I also added a 
table to list the different protocol versions. See the last patch in the 
series.

I'm a bit disappointed that we're not changing the default protocol 
version to 3.2 yet. It will receive very little usage in practice until 
we do. But that seems to be the least objectionable path forward.

I think this is pretty much ready for commit. I will go over it one more 
time, and plan to push tomorrow.

> One open question on the last patch is: Document what the maximum size
> of the cancel key is that the client can expect? I think Jacob might
> have some ideas on that.

It's documented as 256 bytes now. But that still leaves one remaining 
question: the idea is that the maximum size is long enough that 
middleware can add their data to it, and still include the original 
server's key. As the patch stands, the server creates 32 byte keys, 
leaving 224 for middleware. But that's not documented anywhere. I'm 
inclined to document that the server only uses up to 32 bytes, and 
middleware should likewise try to "add" only 32 bytes or so to the key, 
so that you can stack multiple such middleware without exceeding the 
total length. And perhaps 256 bytes is still too small, if you consider 
such stacking.

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

  38. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-02T13:48:46Z

    On 01/04/2025 17:08, Heikki Linnakangas wrote:
> I think this is pretty much ready for commit. I will go over it one more 
> time, and plan to push tomorrow.

And committed, with minor a bunch more little cleanups.

Thank you to everyone involved, in particular Jelte, Robert and Jacob! I 
read through all the discussions on this and the other thread again last 
night to make sure I didn't miss anything. And what a long thread that 
was! But full of good, genuine discussions on pros and cons of various 
details. Changing the protocol is permanent in a way many other changes 
are not. I feel good about where we arrived in the end.

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

  39. Re: Make query cancellation keys longer

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-03T11:29:21Z

    > On 2 Apr 2025, at 15:48, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> And committed, with minor a bunch more little cleanups.

0001 in commit 09be39112654 seems to have missed (omitted?) adding the HAVE_
macros to pg_config.h.in, which doesn't matter as of now since they're not used
but it will be in the way for anyone running autoheader.  Any objection to me
pushing the attached?

--
Daniel Gustafsson

  40. Re: Make query cancellation keys longer

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-03T11:33:13Z

    On 03/04/2025 14:29, Daniel Gustafsson wrote:
>> On 2 Apr 2025, at 15:48, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
>> And committed, with minor a bunch more little cleanups.
> 
> 0001 in commit 09be39112654 seems to have missed (omitted?) adding the HAVE_
> macros to pg_config.h.in, which doesn't matter as of now since they're not used
> but it will be in the way for anyone running autoheader.  Any objection to me
> pushing the attached?

Sounds good, thanks!

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