pgwire: also support cancel under TLS#54618
Conversation
The pg protocol spec says that cancel messages should be sent unencrypted, prior to encryption and the TLS handshake. We've seen a report of a user seeing this after TLS in their logs though. So this patch extends the code to handle both situations. Release note (bug fix): CockroachDB now handles PostgreSQL "cancel" messages on TLS connections in the same way as when they are sent without TLS: the connection is closed, but no action takes place. No error is logged. As a reminder, PostgreSQL "cancel" messages are still unsupported in CockroachDB and client should still use CANCEL QUERY instead.
|
This change is |
andreimatei
left a comment
There was a problem hiding this comment.
I think closing a connection when the client opened it just to send a cancel message (so, in the per-TLS case) is make sense (and regardless of what I think, it is mandated by the docs), but is it also fine in the post-TLS case? Are we doing anybody a favor by closing a connection that was being used by other stuff, instead of just ignoring the message? Is that what pg does?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @yuzefovich)
Yes: the pgwire protocol does not even support a response for a cancel request, and specifies the conn is closed immediately after the query is cancelled. |
|
LGTM assuming you've actually tested pg. |
|
I can't really test pg since |
|
Well but that's what I'm saying - the doc only talks about cancel messages sent on brand new connections, not on existing connections. Closing a connection that was opened for this specific purpose is one thing, closing an arbitrary connection is another in my opinion. I think you can test pg with the data-driven pgwire tests that we have, where you can control exactly what messages get sent. I might be wrong... |
You might be misreading my diff? The new condition I added is also on the code path for brand new connections, before authn happens. Does this change your view? |
andreimatei
left a comment
There was a problem hiding this comment.
What new condition? I see that you've extracted a helper function, but otherwise did you change anything about the handling of the cancel message on brand new conns?
Reviewable status:
|
See the green part at the bottom right here: |
andreimatei
left a comment
There was a problem hiding this comment.
Rapahel just explained to me that this patch changes the existing behavior a lot less than I thought - it only applies in the initial version-negotiation phase of a conn.
Reviewable status:
|
thanks bors r=andreimatei |
|
Build succeeded: |
The pg protocol spec says that cancel messages should be sent
unencrypted, prior to authentication and the TLS handshake.
We've seen a report of a user seeing this after TLS in their logs
though. See https://github.com/cockroachlabs/support/issues/600
So this patch extends the code to handle both situations.
Release note (bug fix): CockroachDB now handles PostgreSQL "cancel"
messages on TLS connections in the same way as when they are sent
without TLS: the connection is closed, but no action takes place. No
error is logged. As a reminder, PostgreSQL "cancel" messages are still
unsupported in CockroachDB and client should still use CANCEL QUERY
instead.