Обсуждение: libpq compression (part 3)

Hello PG developers!

I would like to introduce an updated take on libpq protocol-level
compression, building off off the work in
https://www.postgresql.org/message-id/aad16e41-b3f9-e89d-fa57-fb4c694bec25@postgrespro.ru
and the followon work in
https://www.postgresql.org/message-id/ABAA09C6-BB95-47A5-890D-90353533F9AC@yandex-team.ru
along with all of the (nice and detailed) feedback and discussion therein.

The first patch in the stack replaces `secure_read` and `secure_write`
(and their frontend counterparts) with an "IO stream" abstraction,
which should address a lot of the concerns from all parties around
secure_read.  The fundamental idea of an IO stream is that it is a
linked list of "IoStreamProcessor"s.  The "base" processor is the
actual socket-backed API, and then SSL, GSSAPI, and compression can
all add layers on top of that base that add functionality and rely on
the layer below to read/write data.  This structure makes it easy to
add compression on top of either plain or encrypted sockets through a
unified, unconditional API, and also makes it easy for callers to use
plain, plain-compressed, secure, and secure-compressed communication
channels equivalently.

The second patch is the refactored implementation of compression
itself, with ZSTD support merged into the main patch because the
configuration-level work is now already merged in master.  There was a
good bit of rebasing, housekeeping, and bugfixing (including fixing
lz4 by making it now be explicitly buffered inside ZStream), along
with taking into account a lot of the feedback from this mailing list.
I reworked the API to use the general compression processing types and
methods  from `common/compression`.  This change also refactors the
protocol to require the minimum amount of new message types and
exchanges possible, while also enabling one-directional compression.
The compression "handshaking" process now looks as follows:
1. Client sends startup packet with `_pq_.libpq_compression = alg1;alg2`
2. At this point, the server can immediately begin compressing packets
to the client with any of the specified algorithms it supports if it
so chooses
3. Server includes `libpq_compression` in the automatically sent
`ParameterStatus` messages before handshaking
4. At this point, the client can immediately begin compressing packets
to the server with any of the supported algorithms
Both the server and client will prefer to compress using the first
algorithm in their list that the other side supports, and we
explicitly support `none` in the algorithm list.  This allows e.g. a
client to use `none;gzip` and a server to use `zstd;gzip;lz4`, and
then the client will not compress its data but the server will send
its data using gzip.  Each side uses its own configured compression
level (if set), since compression levels affect compression effort
much more than decompression effort. This change also allows
connections to succeed if compression was requested but not available
(most of the time, I imagine that a client would prefer to just not
use compression if the server doesn't support it; unlike SSL, it's a
nice to have not an essential.  If a client application actually
really *needs* compression, that can still be facilitated by
explicitly checking the negotiated compression methods.)

The third patch adds the traffic monitoring statistics that had been
in the main patch of the previous series.  I've renamed them and
changed slightly where to measure the actual raw network bytes and the
"logical" protocol bytes, which also means this view can measure
SSL/GSSAPI overhead (not that that's a particularly *important* thing
to measure, but it's worth nothing what the view will actually
measure.

The fourth patch adds a TAP test that validates all of the compression
methods and compression negotiation.  Ideally it would probably be
part of patch #2, but it uses the monitoring from #3 to be able to
validate that compression is actually working.

The fifth patch is just a placeholder to allow running the test suite
with compression maximally enabled to work out any kinks.

I believe this patch series is ready for detailed review/testing, with
one caveat: as can be seen here
https://cirrus-ci.com/build/6732518292979712 , the build is passing on
all platforms and all tests except for the primary SSL test on
Windows.  After some network-level debugging, it appears that we are
bumping into a version of the issues seen here

https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BOeoETZQ%3DQw5Ub5h3tmwQhBmDA%3DnuNO3KG%3DzWfUypFAw%40mail.gmail.com
, where on Windows some SSL error messages end up getting swallowed up
by the the process exiting and closing the socket with a RST rather
than a nice clean shutdown.  I may have the cause/effect wrong here,
but the issues appear before the compression is actually fully set up
in the client used and would appear to be a side effect of timing
differences and/or possibly size differences in the startup packet.
Any pointers on how to resolve this would be appreciated.  It does
reproduce on Windows fairly readily, though any one particular test
still sometimes succeeds, and the relevant SSL connection failure
message reliably shows up in Wireshark.

Also please let me know if I have made any notable mailing list/patch
etiquette/format/structure errors.  This is my first time submitting a
patch to a mailing-list driven open source project and while I have
tried to carefully review the various wiki guides I'm sure I didn't
take everything in perfectly.

Thanks,
Jacob Burroughs

> I believe this patch series is ready for detailed review/testing, with one caveat: as can be seen here
https://cirrus-ci.com/build/6732518292979712, the build is passing on all platforms and all tests except for the
primarySSL test on Windows.
 

One correction: I apparently missed a kerberos timeout failure on
freebsd with compression enabled (being color blind the checkmark and
still running colors are awfully similar, and I misread what I saw).
I haven't yet successfully reproduced that one, so I may or may not
need some pointers to sort it out, but I think whatever it is the fix
will be small enough that the patch overall is still reviewable.

> One correction: I apparently missed a kerberos timeout failure on
> freebsd with compression enabled (being color blind the checkmark and
> still running colors are awfully similar, and I misread what I saw).
> I haven't yet successfully reproduced that one, so I may or may not
> need some pointers to sort it out, but I think whatever it is the fix
> will be small enough that the patch overall is still reviewable.

I have now sorted out all of the non-Windows build issues (and removed
my stray misguided attempt at fixing the Windows issue that I hadn't
intended to post the first time around).  The build that is *actually*
passing every platform except for the one Windows SSL test mentioned
in my original message can be seen here:
https://cirrus-ci.com/build/5924321042890752

On Tue, Dec 19, 2023 at 11:41 AM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> The compression "handshaking" process now looks as follows:
> 1. Client sends startup packet with `_pq_.libpq_compression = alg1;alg2`
> 2. At this point, the server can immediately begin compressing packets
> to the client with any of the specified algorithms it supports if it
> so chooses
> 3. Server includes `libpq_compression` in the automatically sent
> `ParameterStatus` messages before handshaking
> 4. At this point, the client can immediately begin compressing packets
> to the server with any of the supported algorithms
> Both the server and client will prefer to compress using the first
> algorithm in their list that the other side supports, and we
> explicitly support `none` in the algorithm list.  This allows e.g. a
> client to use `none;gzip` and a server to use `zstd;gzip;lz4`, and
> then the client will not compress its data but the server will send
> its data using gzip.

I'm having difficulty understanding the details of this handshaking
algorithm from this description. It seems good that the handshake
proceeds in each direction somewhat separately from the other, but I
don't quite understand how the whole thing fits together. If the
client tells the server that 'none,gzip' is supported, and I elect to
start using gzip, how does the client know that I picked gzip rather
than none? Are the compressed packets self-identifying?

It's also slightly odd to me that the same parameter seems to specify
both what we want to send, and what we're able to receive. I'm not
really sure we should have separate parameters for those things, but I
don't quite understand how this works without it. The "none" thing
seems like a bit of a hack. It lets you say "I'd like to receive
compressed data but send uncompressed data" ... but what about the
reverse? How do you say "don't bother compressing what you receive
from the server, but please lz4 everything you send to the server"? Or
how do you say "use zstd from server to client, but lz4 from client to
server"? It seems like you can't really say that kind of thing.

What if we had, on the server side, a GUC saying what compression to
accept and a GUC saying what compression to be willing to do? And then
let the client request whatever it wants for each direction.

> Also please let me know if I have made any notable mailing list/patch
> etiquette/format/structure errors.  This is my first time submitting a
> patch to a mailing-list driven open source project and while I have
> tried to carefully review the various wiki guides I'm sure I didn't
> take everything in perfectly.

Seems fine to me.

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

> I'm having difficulty understanding the details of this handshaking
> algorithm from this description. It seems good that the handshake
> proceeds in each direction somewhat separately from the other, but I
> don't quite understand how the whole thing fits together. If the
> client tells the server that 'none,gzip' is supported, and I elect to
> start using gzip, how does the client know that I picked gzip rather
> than none? Are the compressed packets self-identifying?

I agree I could have spelled this out more clearly.  I forgot to
mention that I added a byte to the CompressedMessage message type that
specifies the chosen algorithm.  So if the server receives
'none,gzip', it can either keep sending uncompressed regular messages,
or it can compress them in CompressedMessage packets which now look
like "z{len}{format}{data}" (format just being a member of the
pg_compress_algorithm enum, so `1` in the case of gzip).  Overall the
intention is that both the client and the server can just start
sending CompressedMessages once they receive the list of ones other
party supports without any negotiation or agreement needed and without
an extra message type to first specify the compression algorithm. (One
byte per message seemed to me like a reasonable overhead for the
simplicity, but it wouldn't be hard to bring back SetCompressionMethod
if we prefer.)

> It's also slightly odd to me that the same parameter seems to specify
> both what we want to send, and what we're able to receive. I'm not
> really sure we should have separate parameters for those things, but I
> don't quite understand how this works without it. The "none" thing
> seems like a bit of a hack. It lets you say "I'd like to receive
> compressed data but send uncompressed data" ... but what about the
> reverse? How do you say "don't bother compressing what you receive
> from the server, but please lz4 everything you send to the server"? Or
> how do you say "use zstd from server to client, but lz4 from client to
> server"? It seems like you can't really say that kind of thing.

When I came up with the protocol I was imagining that basically both
server admins and clients might want a decent bit more control over
the compression they do rather than the decompression they do, since
compression is generally much more computationally expensive than
decompression.  Now that you point it out though, I don't think that
actually makes that much sense.

> What if we had, on the server side, a GUC saying what compression to
> accept and a GUC saying what compression to be willing to do? And then
> let the client request whatever it wants for each direction.

Here's two proposals:
Option 1:
GUCs:
libpq_compression (default "off")
libpq_decompression (default "auto", which is defined to be equal to
libpq_compression)
Connection parameters:
compression (default "off")
decompression (default "auto", which is defined to be equal to compression)

I think we would only send the decompression fields over the wire to
the other side, to be used to filter for the first chosen compression
field.  We would send the `_pq_.libpq_decompression` protocol
extension even if only compression was enabled and not decompression
so that the server knows to enable compression processing for the
connection (I think this would be the only place we would still use
`none`, and not as part of a list in this case.)  I think we also
would want to add libpq functions to allow a client to check the
last-used compression algorithm in each direction for any
monitoring/inspection purposes (actually that's probably a good idea
regardless, so a client application that cares doesn't need to/try to
implement the intersection and assumption around choosing the first
algorithm in common).  Also I'm open to better names than "auto", I
just would like it to avoid unnecessary verbosity for the common case
of "I just want to enable bidirectional compression with whatever
algorithms are available with default parameters".

Option 2:
This one is even cleaner in the common case but a bit worse in the
uncommon case: just use one parameter and have
compression/decompression enabling be part of the compression detail
(e.g. "libpq_compression='gzip:no_decompress;lz4:level=2,no_compress;zstd'"
or something like that, in which case the "none,gzip" case would
become "'libpq_compression=gzip:no_compress'").  See
https://www.postgresql.org/docs/current/app-pgbasebackup.html ,
specifically the `--compress` flag, for how specifying compression
algorithms and details works.

I'm actually not sure which of the two I prefer; opinions are welcome :)

Thanks,
Jacob

> On 21 Dec 2023, at 05:30, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> One thing I'm wondering: should it be possible for the client to change the compression it wants mid-connection?

This patchset allows sending CompressionMethod message, which allows to set another codec\level picked from the set of
negotiatedcodec sets (during startup). 


Best regards, Andrey Borodin.

On Fri, 29 Dec 2023 at 11:02, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> This patchset allows sending CompressionMethod message, which allows to set another codec\level picked from the set
ofnegotiated codec sets (during startup).
 

Did you mean to attach a patchset? I don't see the CompressionMethod
message in the v2 patchset. Only a CompressedData one.

> One thing I'm wondering: should it be possible for the client to change the compression it wants mid-connection? I
canthink of some scenarios where that would be useful to connection poolers: if a pooler does plain forwarding of the
compressedmessages, then it would need to be able to disable/enable compression if it wants to multiplex client
connectionswith different compression settings over the same server connection. 

I have reworked this patch series to make it easier to extend to
restart compression mid-connection once something in the vein of the
discussion in "Add new protocol message to change GUCs for usage with
future protocol-only GUCs" [1] happens.  In particular, I have changed
the `CompressedMessage` protocol message to signal the current
compression algorithm any time the client should restart its streaming
decompressor and otherwise implicitly use whatever compression
algorithm and decompressor was used for previous `CompressedMessage` ,
which future work can leverage to trigger such a restart on update of
the client-supported compression algorithms.

> Option 2:
> This one is even cleaner in the common case but a bit worse in the
> uncommon case: just use one parameter and have
> compression/decompression enabling be part of the compression detail
> (e.g. "libpq_compression='gzip:no_de
> compress;lz4:level=2,no_compress;zstd'"
> or something like that, in which case the "none,gzip" case would
> become "'libpq_compression=gzip:no_compress'").  See
> https://www.postgresql.org/docs/current/app-pgbasebackup.html ,
> specifically the `--compress` flag, for how specifying compression
> algorithms and details works.

I ended up reworking this to use a version of this option in place of
the `none` hackery, but naming the parameters `compress` and
`decompress, so to disable compression but allow decompression you
would specify `libpq_compression=gzip:compress=off`.

Also my windows SSL test failures seem to have resolved themselves
with either these changes or a rebase, so I think things are truly in
a reviewable state now.

On Sun, Dec 31, 2023 at 2:32 AM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> I ended up reworking this to use a version of this option in place of
> the `none` hackery, but naming the parameters `compress` and
> `decompress, so to disable compression but allow decompression you
> would specify `libpq_compression=gzip:compress=off`.

I'm still a bit befuddled by this interface.
libpq_compression=gzip:compress=off looks a lot like it's saying "do
it, except don't". I guess that you're using "compress" and
"decompress" to distinguish the two directions - i.e. server to client
and client to server - but of course in both directions the sender
compresses and the receiver decompresses, so I don't find that very
clear.

I wonder if we could use "upstream" and "downstream" to be clearer? Or
some other terminology?

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

> I wonder if we could use "upstream" and "downstream" to be clearer? Or
> some other terminology?

What about `send` and `receive`?

On Fri, Jan 12, 2024 at 4:02 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> > I wonder if we could use "upstream" and "downstream" to be clearer? Or
> > some other terminology?
>
> What about `send` and `receive`?

I think that would definitely be better than "compress" and
"decompress," but I was worried that it might be unclear to the user
whether the parameter that they specified was from the point of view
of the client or the server. Perhaps that's a dumb thing to worry
about, though.

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

On Fri, Jan 12, 2024 at 4:11 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I think that would definitely be better than "compress" and
> "decompress," but I was worried that it might be unclear to the user
> whether the parameter that they specified was from the point of view
> of the client or the server. Perhaps that's a dumb thing to worry
> about, though.

According to https://commitfest.postgresql.org/48/4746/ this patch set
needs review, but:

1. Considering that there have been no updates for 5 months, maybe
it's actually dead?

and

2. I still think it needs to be more clear how the interface is
supposed to work. I do not want to spend time reviewing a patch to see
whether it works without understanding how it is intended to work --
and I also think that reviewing the patch in detail before we've got
the user interface right makes a whole lot of sense.

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

On Tue, May 14, 2024 at 11:08 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> According to https://commitfest.postgresql.org/48/4746/ this patch set
> needs review, but:
>
> 1. Considering that there have been no updates for 5 months, maybe
> it's actually dead?

I've withdrawn this patch from the commitfest.  I had been waiting for
some resolution on "Add new protocol message to change GUCs for usage
with future protocol-only GUCs" before I rebased/refactored this one,
because this would be introducing the first protocol extension so far,
and that discussion appeared to be working out some meaningful issues
on how GUCs and protocol parameters should interact.  If you think it
is worthwhile to proceed here though, I am very happy to do so. (I
would love to see this feature actually make it into postgres; it
would almost certainly be a big efficiency and cost savings win for
how my company deploys postgres internally :) )

> 2. I still think it needs to be more clear how the interface is
> supposed to work. I do not want to spend time reviewing a patch to see
> whether it works without understanding how it is intended to work --
> and I also think that reviewing the patch in detail before we've got
> the user interface right makes a whole lot of sense.

Regarding the interface, what I had originally gone for was the idea
that the naming of the options was from the perspective of the side
you were setting them on.  Therefore, when setting `libpq_compression`
as a server-level GUC, `compress` would control if the server would
compress (send compressed data) with the given algorithm, and
`decompress` would control if the the server would decompress (receive
compressed data) with the given algorithm.  And likewise on the client
side, when setting `compression` as a connection config option,
`compress` would control if the *client* would compress (send
compressed data) with the given algorithm, and `decompress` would
control if the the *client* would decompress (receive compressed data)
with the given algorithm.  So for a client to pick what to send, it
would choose from the intersection of its own `compress=true` and the
server's `decompress=true` algorithms sent in the `ParameterStatus`
message with `libpq_compression`.  And likewise on the server side, it
would choose from the intersection of the server's `compress=true`
algorithms and the client's `decompress=true` algorithms sent in the
`_pq_.libpq_compression` startup option.  If you have a better
suggestion I am very open to it though.



--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Tue, May 14, 2024 at 12:30 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> I've withdrawn this patch from the commitfest.  I had been waiting for
> some resolution on "Add new protocol message to change GUCs for usage
> with future protocol-only GUCs" before I rebased/refactored this one,
> because this would be introducing the first protocol extension so far,
> and that discussion appeared to be working out some meaningful issues
> on how GUCs and protocol parameters should interact.  If you think it
> is worthwhile to proceed here though, I am very happy to do so. (I
> would love to see this feature actually make it into postgres; it
> would almost certainly be a big efficiency and cost savings win for
> how my company deploys postgres internally :) )

I don't think you should wait for that to be resolved; IMHO, this
patch needs to inform that discussion more than the other way around.

> > 2. I still think it needs to be more clear how the interface is
> > supposed to work. I do not want to spend time reviewing a patch to see
> > whether it works without understanding how it is intended to work --
> > and I also think that reviewing the patch in detail before we've got
> > the user interface right makes a whole lot of sense.
>
> Regarding the interface, what I had originally gone for was the idea
> that the naming of the options was from the perspective of the side
> you were setting them on.  Therefore, when setting `libpq_compression`
> as a server-level GUC, `compress` would control if the server would
> compress (send compressed data) with the given algorithm, and
> `decompress` would control if the the server would decompress (receive
> compressed data) with the given algorithm.  And likewise on the client
> side, when setting `compression` as a connection config option,
> `compress` would control if the *client* would compress (send
> compressed data) with the given algorithm, and `decompress` would
> control if the the *client* would decompress (receive compressed data)
> with the given algorithm.  So for a client to pick what to send, it
> would choose from the intersection of its own `compress=true` and the
> server's `decompress=true` algorithms sent in the `ParameterStatus`
> message with `libpq_compression`.  And likewise on the server side, it
> would choose from the intersection of the server's `compress=true`
> algorithms and the client's `decompress=true` algorithms sent in the
> `_pq_.libpq_compression` startup option.  If you have a better
> suggestion I am very open to it though.

Well, in my last response before the thread died, I complained that
libpq_compression=gzip:compress=off was confusing, and I stand by
that, because "compress" is used both in the name of the parameter and
as an option within the value of that parameter. I think there's more
than one acceptable way to resolve that problem, but I think leaving
it like that is unacceptable.

Even more broadly, I think there have been a couple of versions of
this patch now where I read the documentation and couldn't understand
how the feature was supposed to work, and I'm not really willing to
spend time trying to review a complex patch for conformity with a
design that I can't understand in the first place. I don't want to
pretend like I'm the smartest person on this mailing list, and in fact
I know that I'm definitely not, but I think I'm smart enough and
experienced enough with PostgreSQL that if I look at the description
of a parameter and say "I don't understand how the heck this is
supposed to work", probably a lot of users are going to have the same
reaction. That lack of understanding on my part my come either from
the explanation of the parameter not being as good as it needs to be,
or from the design itself not being as good as it needs to be, or from
some combination of the two, but whichever is the case, IMHO you or
somebody else has got to figure out how to fix it.

I do also admit that there is a possibility that everything is totally
fine and I've just been kinda dumb on the days when I've looked at the
patch. If a chorus of other hackers shows up and gives me a few whacks
with the cluestick and after that I look at the proposed options and
go "oh, yeah, these totally make sense, I was just being stupid," fair
enough! But right now that's not where I'm at. I don't want you to
explain to me how it works; I want you to change it in some way so
that when I or some end user looks at it, they go "I don't need an
explanation of how that works because it's extremely clear to me
already," or at least "hmm, this is a bit complicated but after a
quick glance at the documentation it makes sense".

I would really like to see this patch go forward, but IMHO these UI
questions are blockers.

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

On Tue, May 14, 2024 at 1:35 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> Well, in my last response before the thread died, I complained that
> libpq_compression=gzip:compress=off was confusing, and I stand by
> that, because "compress" is used both in the name of the parameter and
> as an option within the value of that parameter. I think there's more
> than one acceptable way to resolve that problem, but I think leaving
> it like that is unacceptable.

What if we went with:
Server side:
* `libpq_compression=on` (I just want everything the server supports
available; probably the most common case)
* `libpq_compression=off` (I don't want any compression ever with this server)
* `libpq_compression=lzma;gzip` (I only want these algorithms for
whatever reason)
* `libpq_compression=lzma:client_to_server=off;gzip:server_to_client=off`
(I only want to send data with lzma and receive data with gzip)
Client side:
*`compression=on` (I just want compression; pick sane defaults
automatically for me; probably the most common case)
* `compression=off` (I don't want any compression)
* `compression=lzma;gzip` (I only want these algorithms for whatever reason)
* `compression=lzma:client_to_server=off;gzip:server_to_client=off` (I
only want to receive data with lzma and send data with gzip)

`client_to_server`/`server_to_client` is a bit verbose, but it's very
explicit in what it means, so you don't need to reason about who is
sending/receiving/etc in a given context, and a given config string
applied to the server or the client side has the same effect on the
connection.

> Even more broadly, I think there have been a couple of versions of
> this patch now where I read the documentation and couldn't understand
> how the feature was supposed to work, and I'm not really willing to
> spend time trying to review a complex patch for conformity with a
> design that I can't understand in the first place. I don't want to
> pretend like I'm the smartest person on this mailing list, and in fact
> I know that I'm definitely not, but I think I'm smart enough and
> experienced enough with PostgreSQL that if I look at the description
> of a parameter and say "I don't understand how the heck this is
> supposed to work", probably a lot of users are going to have the same
> reaction. That lack of understanding on my part my come either from
> the explanation of the parameter not being as good as it needs to be,
> or from the design itself not being as good as it needs to be, or from
> some combination of the two, but whichever is the case, IMHO you or
> somebody else has got to figure out how to fix it.

If the above proposal seems better to you I'll both rework the patch
and then also try to rewrite the relevant bits of documentation to
separate out "what knobs are there" and "how do I specify the flags to
turn the knobs", because I think those two being integrated is making
the parameter documentation less readable/followable.


--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Tue, May 14, 2024 at 3:22 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> What if we went with:
> Server side:
> * `libpq_compression=on` (I just want everything the server supports
> available; probably the most common case)
> * `libpq_compression=off` (I don't want any compression ever with this server)
> * `libpq_compression=lzma;gzip` (I only want these algorithms for
> whatever reason)
> * `libpq_compression=lzma:client_to_server=off;gzip:server_to_client=off`
> (I only want to send data with lzma and receive data with gzip)
> Client side:
> *`compression=on` (I just want compression; pick sane defaults
> automatically for me; probably the most common case)
> * `compression=off` (I don't want any compression)
> * `compression=lzma;gzip` (I only want these algorithms for whatever reason)
> * `compression=lzma:client_to_server=off;gzip:server_to_client=off` (I
> only want to receive data with lzma and send data with gzip)
>
> `client_to_server`/`server_to_client` is a bit verbose, but it's very
> explicit in what it means, so you don't need to reason about who is
> sending/receiving/etc in a given context, and a given config string
> applied to the server or the client side has the same effect on the
> connection.

IMHO, that's a HUGE improvement. But:

* I would probably change is the name "libpq_compression", because
even though we have src/backend/libpq, we typically use libpq to refer
to the client library, not the server's implementation of the wire
protocol. I think we could call it connection_encryption or
wire_protocol_encryption or something like that, but I'm not a huge
fan of libpq_compression.

* I would use commas, not semicolons, to separate items in a list,
i.e. lzma,gzip not lzma;gzip. I think that convention is nearly
universal in PostgreSQL, but feel free to point out counterexamples if
you were modelling this on something.

* libpq_compression=lzma:client_to_server=off;gzip:server_to_client=off
reads strangely to me. How about making it so that the syntax is like
this:


libpq_compression=DEFAULT_VALUE_FOR_BOTH_DIRECTIONS:client_to_server=OVERRIDE_FOR_THIS_DIRECTION:servert_to_client=OVERRIDE_FOR_THIS_DIRECTION

With all components being optional. So this example could be written
in any of these ways:

libpq_compression=lzma;server_to_client=gzip
libpq_compression=gzip;client_to_server=lzma
libpq_compression=server_to_client=gzip;client_to_server=lzma
libpq_compression=client_to_server=lzma;client_to_server=gzip

And if I wrote libpq_compression=server_to_client=gzip that would mean
send data to the client using gzip and in the other direction use
whatever the default is.

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

On Tue, May 14, 2024 at 3:24 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> IMHO, that's a HUGE improvement. But:
>
> * I would probably change is the name "libpq_compression", because
> even though we have src/backend/libpq, we typically use libpq to refer
> to the client library, not the server's implementation of the wire
> protocol. I think we could call it connection_encryption or
> wire_protocol_encryption or something like that, but I'm not a huge
> fan of libpq_compression.
>
I think connection_compression would seem like a good name to me.

> * I would use commas, not semicolons, to separate items in a list,
> i.e. lzma,gzip not lzma;gzip. I think that convention is nearly
> universal in PostgreSQL, but feel free to point out counterexamples if
> you were modelling this on something.
>
> * libpq_compression=lzma:client_to_server=off;gzip:server_to_client=off
> reads strangely to me. How about making it so that the syntax is like
> this:
>
>
libpq_compression=DEFAULT_VALUE_FOR_BOTH_DIRECTIONS:client_to_server=OVERRIDE_FOR_THIS_DIRECTION:servert_to_client=OVERRIDE_FOR_THIS_DIRECTION
>
> With all components being optional. So this example could be written
> in any of these ways:
>
> libpq_compression=lzma;server_to_client=gzip
> libpq_compression=gzip;client_to_server=lzma
> libpq_compression=server_to_client=gzip;client_to_server=lzma
> libpq_compression=client_to_server=lzma;client_to_server=gzip
>
> And if I wrote libpq_compression=server_to_client=gzip that would mean
> send data to the client using gzip and in the other direction use
> whatever the default is.

The reason for both the semicolons and for not doing this is related
to using the same specification structure as here:
https://www.postgresql.org/docs/current/app-pgbasebackup.html
(specifically the --compress argument). Reusing that specification
requires that we use commas to separate the flags for a compression
method, and therefore left me with semicolons as the leftover
separator character. I think we could go with something like your
proposal, and in a lot of ways I like it, but there's still the
possibility of e.g.
`libpq_compression=client_to_server=zstd:level=10,long=true,gzip;client_to_server=gzip`
and I'm not quite sure how to make the separator characters work
coherently if we need to treat `zstd:level=10,long=true` as a unit.
Alternatively, we could have `connection_compression`,
`connection_compression_server_to_client`, and
`connection_compression_client_to_server` as three separate GUCs (and
on the client side `compression`, `compression_server_to_client`, and
`compression_client_to_server` as three separate connection
parameters), where we would treat `connection_compression` as a
default that could be overridden by an explicit
client_to_server/server_to_client.  That creates the slightly funky
case where if you specify all three then the base one ends up unused
because the two more specific ones are being used instead, but that
isn't necessarily terrible.  On the server side we *could* go with
just the server_to_client and client_to_server ones, but I think we
want it to be easy to use this feature in the simple case with a
single libpq parameter.

--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Tue, May 14, 2024 at 5:21 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> The reason for both the semicolons and for not doing this is related
> to using the same specification structure as here:
> https://www.postgresql.org/docs/current/app-pgbasebackup.html
> (specifically the --compress argument).

I agree with that goal, but I'm somewhat confused by how your proposal
achieves it. You had
libpq_compression=lzma:client_to_server=off;gzip:server_to_client=off,
so how do we parse that? Is that two completely separate
specifications, one for lzma and one for gzip, and each of those has
one option which is set to off? And then they are separated from each
other by a semicolon? That actually does make sense, and I think it
may do a better job allowing for compression options than my proposal,
but it also seems a bit weird, because client_to_server and
server_to_client are not really compression options at all. They're
framing when this compression specification applies, rather than what
it does when it applies. In a way it's a bit like the fact that you
can prefix a pg_basebackup's --compress option with client- or server-
to specify where the compression should happen. But we can't quite
reuse that idea here, because in that case there's no question of
doing it in both places, whereas here, you might want one thing for
upstream and another thing for downstream.

> Alternatively, we could have `connection_compression`,
> `connection_compression_server_to_client`, and
> `connection_compression_client_to_server` as three separate GUCs (and
> on the client side `compression`, `compression_server_to_client`, and
> `compression_client_to_server` as three separate connection
> parameters), where we would treat `connection_compression` as a
> default that could be overridden by an explicit
> client_to_server/server_to_client.  That creates the slightly funky
> case where if you specify all three then the base one ends up unused
> because the two more specific ones are being used instead, but that
> isn't necessarily terrible.  On the server side we *could* go with
> just the server_to_client and client_to_server ones, but I think we
> want it to be easy to use this feature in the simple case with a
> single libpq parameter.

I'm not a fan of three settings; I could go with two settings, one for
each direction, and if you want both you have to set both. Or, another
idea, what if we just separated the two directions with a slash,
SEND/RECEIVE, and if there's no slash, then it applies to both
directions. So you could say
connection_compression='gzip:level=9/lzma' or whatever.

But now I'm wondering whether these options should really be symmetric
on the client and server sides? Isn't it for the server just to
specify a list of acceptable algorithms, and the client to set the
compression options? If both sides are trying to set the compression
level, for example, who wins?

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

On Wed, May 15, 2024 at 8:38 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> I agree with that goal, but I'm somewhat confused by how your proposal
> achieves it. You had
> libpq_compression=lzma:client_to_server=off;gzip:server_to_client=off,
> so how do we parse that? Is that two completely separate
> specifications, one for lzma and one for gzip, and each of those has
> one option which is set to off? And then they are separated from each
> other by a semicolon? That actually does make sense, and I think it
> may do a better job allowing for compression options than my proposal,
> but it also seems a bit weird, because client_to_server and
> server_to_client are not really compression options at all. They're
> framing when this compression specification applies, rather than what
> it does when it applies. In a way it's a bit like the fact that you
> can prefix a pg_basebackup's --compress option with client- or server-
> to specify where the compression should happen. But we can't quite
> reuse that idea here, because in that case there's no question of
> doing it in both places, whereas here, you might want one thing for
> upstream and another thing for downstream.

Your interpretation is correct, but I don't disagree that it ends up
feeling confusing.

> I'm not a fan of three settings; I could go with two settings, one for
> each direction, and if you want both you have to set both. Or, another
> idea, what if we just separated the two directions with a slash,
> SEND/RECEIVE, and if there's no slash, then it applies to both
> directions. So you could say
> connection_compression='gzip:level=9/lzma' or whatever.
>
> But now I'm wondering whether these options should really be symmetric
> on the client and server sides? Isn't it for the server just to
> specify a list of acceptable algorithms, and the client to set the
> compression options? If both sides are trying to set the compression
> level, for example, who wins?

Compression options really only ever apply to the side doing the
compressing, and at least as I had imagined things each party
(client/server) only used its own level/other compression params.
That leaves me thinking, maybe we really want two independent GUCs,
one for "what algorithms are enabled/negotiable" and one for "how
should I configure my compressors" and then we reduce the dimensions
we are trying to shove into one GUC and each one ends up with a very
clear purpose:
connection_compression=(yes|no|alg1,alg2:server_to_client=alg1,alg2:client_to_server=alg3)
connection_compression_opts=gzip:level=2


--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Wed, May 15, 2024 at 12:24 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> > But now I'm wondering whether these options should really be symmetric
> > on the client and server sides? Isn't it for the server just to
> > specify a list of acceptable algorithms, and the client to set the
> > compression options? If both sides are trying to set the compression
> > level, for example, who wins?
>
> Compression options really only ever apply to the side doing the
> compressing, and at least as I had imagined things each party
> (client/server) only used its own level/other compression params.
> That leaves me thinking, maybe we really want two independent GUCs,
> one for "what algorithms are enabled/negotiable" and one for "how
> should I configure my compressors" and then we reduce the dimensions
> we are trying to shove into one GUC and each one ends up with a very
> clear purpose:
> connection_compression=(yes|no|alg1,alg2:server_to_client=alg1,alg2:client_to_server=alg3)
> connection_compression_opts=gzip:level=2

From my point of view, it's the client who knows what it wants to do
with the connection. If the client plans to read a lot of data, it
might want the server to compress that data, especially if it knows
that it's on a slow link. If the client plans to send a lot of data --
basically COPY, I'm not thinking this is going to matter much
otherwise -- then it might want to compress that data before sending
it, again, especially if it knows that it's on a slow link.

But what does the server know, really? If some client connects and
sends a SELECT query, the server can't guess whether that query is
going to return 1 row or 100 million rows, so it has no idea of
whether compression is likely to make sense or not. It is entitled to
decide, as a matter of policy, that it's not willing to perform
compression, either because of CPU consumption or security concerns or
whatever, but it has no knowledge of what the purpose of this
particular connection is.

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

On Wed, May 15, 2024 at 11:31 AM Robert Haas <robertmhaas@gmail.com> wrote:
> From my point of view, it's the client who knows what it wants to do
> with the connection. If the client plans to read a lot of data, it
> might want the server to compress that data, especially if it knows
> that it's on a slow link. If the client plans to send a lot of data --
> basically COPY, I'm not thinking this is going to matter much
> otherwise -- then it might want to compress that data before sending
> it, again, especially if it knows that it's on a slow link.
>
> But what does the server know, really? If some client connects and
> sends a SELECT query, the server can't guess whether that query is
> going to return 1 row or 100 million rows, so it has no idea of
> whether compression is likely to make sense or not. It is entitled to
> decide, as a matter of policy, that it's not willing to perform
> compression, either because of CPU consumption or security concerns or
> whatever, but it has no knowledge of what the purpose of this
> particular connection is.

I think I would agree with that.  That said, I don't think the client
should be in the business of specifying what configuration of the
compression algorithm the server should use.  The server administrator
(or really most of the time, the compression library developer's
defaults) gets to pick the compression/compute tradeoff for
compression that runs on the server (which I would imagine would be
the vast majority of it), and the client gets to pick those same
parameters for any compression that runs on the client machine
(probably indeed in practice only for large COPYs).  The *algorithm*
needs to actually be communicated/negotiated since different
client/server pairs may be built with support for different
compression libraries, but I think it is reasonable to say that the
side that actually has to do the computationally expensive part owns
the configuration of that part too.  Maybe I'm missing a good reason
that we want to allow clients to choose compression levels for the
server though?


--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Wed, May 15, 2024 at 12:50 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> I think I would agree with that.  That said, I don't think the client
> should be in the business of specifying what configuration of the
> compression algorithm the server should use.  The server administrator
> (or really most of the time, the compression library developer's
> defaults) gets to pick the compression/compute tradeoff for
> compression that runs on the server (which I would imagine would be
> the vast majority of it), and the client gets to pick those same
> parameters for any compression that runs on the client machine
> (probably indeed in practice only for large COPYs).  The *algorithm*
> needs to actually be communicated/negotiated since different
> client/server pairs may be built with support for different
> compression libraries, but I think it is reasonable to say that the
> side that actually has to do the computationally expensive part owns
> the configuration of that part too.  Maybe I'm missing a good reason
> that we want to allow clients to choose compression levels for the
> server though?

Well, I mean, I don't really know what the right answer is here, but
right now I can say pg_dump --compress=gzip to compress the dump with
gzip, or pg_dump --compress=gzip:9 to compress with gzip level 9. Now,
say that instead of compressing the output, I want to compress the
data sent to me over the connection. So I figure I should be able to
say pg_dump 'compress=gzip' or pg_dump 'compress=gzip:9'. I think you
want to let me do the first of those but not the second. But, to turn
your question on its head, what would be the reasoning behind such a
restriction?

Note also the precedent of pg_basebackup. I can say pg_basebackup
--compress=server-gzip:9 to ask the server to compress the backup with
gzip at level 9. In that case, what I request from the server changes
the actual output that I get, which is not the case here. Even so, I
don't really understand what the justification would be for refusing
to let the client ask for a specific compression level.

And on the flip side, I also don't understand why the server would
want to mandate a certain compression level. If compression is very
expensive for a certain algorithm when the level is above some
threshold X, we could have a GUC to limit the maximum level that the
client can request. But, given that the gzip compression level
defaults to 6 in every other context, why would the administrator of a
particular server want to say, well, the default for my server is 3 or
9 or whatever?

(This is of course all presuming you want to use gzip at all, which
you probably don't, because gzip is crazy slow. Use lz4 or zstd! But
it makes the point.)

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

On Thu, May 16, 2024 at 3:28 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> Well, I mean, I don't really know what the right answer is here, but
> right now I can say pg_dump --compress=gzip to compress the dump with
> gzip, or pg_dump --compress=gzip:9 to compress with gzip level 9. Now,
> say that instead of compressing the output, I want to compress the
> data sent to me over the connection. So I figure I should be able to
> say pg_dump 'compress=gzip' or pg_dump 'compress=gzip:9'. I think you
> want to let me do the first of those but not the second. But, to turn
> your question on its head, what would be the reasoning behind such a
> restriction?

I think I was more thinking that trying to let both parties control
the parameter seemed like a recipe for confusion and sadness, and so
the choice that felt most natural to me was to let the sender control
it, but I'm definitely open to changing that the other way around.

> Note also the precedent of pg_basebackup. I can say pg_basebackup
> --compress=server-gzip:9 to ask the server to compress the backup with
> gzip at level 9. In that case, what I request from the server changes
> the actual output that I get, which is not the case here. Even so, I
> don't really understand what the justification would be for refusing
> to let the client ask for a specific compression level.
>
> And on the flip side, I also don't understand why the server would
> want to mandate a certain compression level. If compression is very
> expensive for a certain algorithm when the level is above some
> threshold X, we could have a GUC to limit the maximum level that the
> client can request. But, given that the gzip compression level
> defaults to 6 in every other context, why would the administrator of a
> particular server want to say, well, the default for my server is 3 or
> 9 or whatever?
>
> (This is of course all presuming you want to use gzip at all, which
> you probably don't, because gzip is crazy slow. Use lz4 or zstd! But
> it makes the point.)

New proposal, predicated on the assumption that if you enable
compression you are ok with the client picking whatever level they
want.  At least with the currently enabled algorithms I don't think
any of them are so insane that they would knock over a server or
anything, and in general postgres servers are usually connected to by
clients that the server admin has some channel to talk to (after all
they somehow had to get access to log in to the server in the first
place) if they are doing something wasteful, given that a client can
do a lot worse things than enable aggressive compression by writing
bad queries.

On the server side, we use slash separated sets of options

connection_compression=DEFAULT_VALUE_FOR_BOTH_DIRECTIONS/client_to_server=OVERRIDE_FOR_THIS_DIRECTION/server_to_client=OVERRIDE_FOR_THIS_DIRECTION
with the values being semicolon separated compression algorithms.
On the client side, you can specify
compression=<same_specification_as_above>,
but on the client side you can actually specify compression options,
which the server will use if provided, and otherwise it will fall back
to defaults.

If we think we need to, we could let the server specify defaults for
server-side compression.  My overall thought though is that having an
excessive number of knobs increases the surface area for testing and
bugs while also increasing potential user confusion and that allowing
configuration on *both* sides doesn't seem sufficiently useful to be
worth adding that complexity.

--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Fri, May 17, 2024 at 1:53 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> New proposal, predicated on the assumption that if you enable
> compression you are ok with the client picking whatever level they
> want.  At least with the currently enabled algorithms I don't think
> any of them are so insane that they would knock over a server or
> anything, and in general postgres servers are usually connected to by
> clients that the server admin has some channel to talk to (after all
> they somehow had to get access to log in to the server in the first
> place) if they are doing something wasteful, given that a client can
> do a lot worse things than enable aggressive compression by writing
> bad queries.

We're talking about a transport-level option, though -- I thought the
proposal enabled compression before authentication completed? Or has
that changed?

(I'm suspicious of arguments that begin "well you can already do bad
things", anyway... It seems like there's a meaningful difference
between consuming resources running a parsed query and consuming
resources trying to figure out what the parsed query is. I don't know
if the solution is locking in a compression level, or something else;
maybe they're both reasonably mitigated in the same way. I haven't
really looked into zip bombs much.)

--Jacob

On Fri, May 17, 2024 at 4:53 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> I think I was more thinking that trying to let both parties control
> the parameter seemed like a recipe for confusion and sadness, and so
> the choice that felt most natural to me was to let the sender control
> it, but I'm definitely open to changing that the other way around.

To be clear, I am not arguing that it should be the receiver's choice.
I'm arguing it should be the client's choice, which means the client
decides what it sends and also tells the server what to send to it.
I'm open to counter-arguments, but as I've thought about this more,
I've come to the conclusion that letting the client control the
behavior is the most likely to be useful and the most consistent with
existing facilities. I think we're on the same page based on the rest
of your email: I'm just clarifying.

> On the server side, we use slash separated sets of options
>
connection_compression=DEFAULT_VALUE_FOR_BOTH_DIRECTIONS/client_to_server=OVERRIDE_FOR_THIS_DIRECTION/server_to_client=OVERRIDE_FOR_THIS_DIRECTION
> with the values being semicolon separated compression algorithms.
> On the client side, you can specify
> compression=<same_specification_as_above>,
> but on the client side you can actually specify compression options,
> which the server will use if provided, and otherwise it will fall back
> to defaults.

I have some quibbles with the syntax but I agree with the concept.
What I'd probably do is separate the server side thing into two GUCs,
each with a list of algorithms, comma-separated, like we do for other
lists in postgresql.conf. Maybe make the default 'all' meaning
"everything this build of the server supports". On the client side,
I'd allow both things to be specified using a single option, because
wanting to do the same thing in both directions will be common, and
you actually have to type in connection strings sometimes, so
verbosity matters more.

As far as the format of the value for that keyword, what do you think
about either compression=DO_THIS_BOTH_WAYS or
compression=DO_THIS_WHEN_SENDING/DO_THIS_WHEN_RECEIVING, with each "do
this" being a specification of the same form already accepted for
server-side compression e.g. gzip or gzip:level=9? If you don't like
that, why do you think the proposal you made above is better, and why
is that one now punctuated with slashes instead of semicolons?

> If we think we need to, we could let the server specify defaults for
> server-side compression.  My overall thought though is that having an
> excessive number of knobs increases the surface area for testing and
> bugs while also increasing potential user confusion and that allowing
> configuration on *both* sides doesn't seem sufficiently useful to be
> worth adding that complexity.

I agree.

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

On Fri, 17 May 2024 at 23:40, Robert Haas <robertmhaas@gmail.com> wrote:
> To be clear, I am not arguing that it should be the receiver's choice.
> I'm arguing it should be the client's choice, which means the client
> decides what it sends and also tells the server what to send to it.
> I'm open to counter-arguments, but as I've thought about this more,
> I've come to the conclusion that letting the client control the
> behavior is the most likely to be useful and the most consistent with
> existing facilities. I think we're on the same page based on the rest
> of your email: I'm just clarifying.

+1

> I have some quibbles with the syntax but I agree with the concept.
> What I'd probably do is separate the server side thing into two GUCs,
> each with a list of algorithms, comma-separated, like we do for other
> lists in postgresql.conf. Maybe make the default 'all' meaning
> "everything this build of the server supports". On the client side,
> I'd allow both things to be specified using a single option, because
> wanting to do the same thing in both directions will be common, and
> you actually have to type in connection strings sometimes, so
> verbosity matters more.
>
> As far as the format of the value for that keyword, what do you think
> about either compression=DO_THIS_BOTH_WAYS or
> compression=DO_THIS_WHEN_SENDING/DO_THIS_WHEN_RECEIVING, with each "do
> this" being a specification of the same form already accepted for
> server-side compression e.g. gzip or gzip:level=9? If you don't like
> that, why do you think the proposal you made above is better, and why
> is that one now punctuated with slashes instead of semicolons?

+1

On Fri, 17 May 2024 at 23:10, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> We're talking about a transport-level option, though -- I thought the
> proposal enabled compression before authentication completed? Or has
> that changed?

I think it would make sense to only compress messages after
authentication has completed. The gain of compressing authentication
related packets seems pretty limited.

> (I'm suspicious of arguments that begin "well you can already do bad
> things"

Once logged in it's really easy to max out a core of the backend
you're connected as. There's many trivial queries you can use to do
that. An example would be:
SELECT sum(i) from generate_series(1, 1000000000) i;

So I don't think it makes sense to worry about an attacker using a
high compression level as a means to DoS the server. Sending a few of
the above queries seems much easier.

On Fri, May 17, 2024 at 11:40 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> To be clear, I am not arguing that it should be the receiver's choice.
> I'm arguing it should be the client's choice, which means the client
> decides what it sends and also tells the server what to send to it.
> I'm open to counter-arguments, but as I've thought about this more,
> I've come to the conclusion that letting the client control the
> behavior is the most likely to be useful and the most consistent with
> existing facilities. I think we're on the same page based on the rest
> of your email: I'm just clarifying.

This is what I am imagining too

> I have some quibbles with the syntax but I agree with the concept.
> What I'd probably do is separate the server side thing into two GUCs,
> each with a list of algorithms, comma-separated, like we do for other
> lists in postgresql.conf. Maybe make the default 'all' meaning
> "everything this build of the server supports". On the client side,
> I'd allow both things to be specified using a single option, because
> wanting to do the same thing in both directions will be common, and
> you actually have to type in connection strings sometimes, so
> verbosity matters more.
>
> As far as the format of the value for that keyword, what do you think
> about either compression=DO_THIS_BOTH_WAYS or
> compression=DO_THIS_WHEN_SENDING/DO_THIS_WHEN_RECEIVING, with each "do
> this" being a specification of the same form already accepted for
> server-side compression e.g. gzip or gzip:level=9? If you don't like
> that, why do you think the proposal you made above is better, and why
> is that one now punctuated with slashes instead of semicolons?

I like this more than what I proposed, and will update the patches to
reflect this proposal. (I've gotten them locally back into a state of
applying cleanly and dealing with the changes needed to support direct
SSL connections, so refactoring the protocol layer shouldn't be too
hard now.)

On Fri, May 17, 2024 at 11:10 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> We're talking about a transport-level option, though -- I thought the
> proposal enabled compression before authentication completed? Or has
> that changed?
On Fri, May 17, 2024 at 1:03 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> I think it would make sense to only compress messages after
> authentication has completed. The gain of compressing authentication
> related packets seems pretty limited.

At the protocol level, compressed data is a message type that can be
used to wrap arbitrary data as soon as the startup packet is
processed.  However, as an implementation detail that clients should
not rely on but that we can rely on in thinking about the
implications, the only message types that are compressed (except in
the 0005 CI patch for test running only) are PqMsg_CopyData,
PqMsg_DataRow, and PqMsg_Query, all of which aren't sent before
authentication.

--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Fri, May 17, 2024 at 4:03 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Fri, 17 May 2024 at 23:10, Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
> > We're talking about a transport-level option, though -- I thought the
> > proposal enabled compression before authentication completed? Or has
> > that changed?
>
> I think it would make sense to only compress messages after
> authentication has completed. The gain of compressing authentication
> related packets seems pretty limited.

Okay. But if we're relying on that for its security properties, it
needs to be enforced by the server.

> > (I'm suspicious of arguments that begin "well you can already do bad
> > things"
>
> Once logged in it's really easy to max out a core of the backend
> you're connected as. There's many trivial queries you can use to do
> that. An example would be:
> SELECT sum(i) from generate_series(1, 1000000000) i;

This is just restating the "you can already do bad things" argument. I
understand that if your query gets executed, it's going to consume
resources on the thing that's executing it (for the record, though,
there are people working on constraining that). But introducing
disproportionate resource consumption into all traffic-inspecting
software, like pools and bouncers, seems like a different thing to me.
Many use cases are going to be fine with it, of course, but I don't
think it should be hand-waved.

--Jacob

On Sat, May 18, 2024 at 1:18 AM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> I like this more than what I proposed, and will update the patches to
> reflect this proposal. (I've gotten them locally back into a state of
> applying cleanly and dealing with the changes needed to support direct
> SSL connections, so refactoring the protocol layer shouldn't be too
> hard now.)

Sounds good!

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

On Mon, May 20, 2024 at 10:15 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> This is just restating the "you can already do bad things" argument. I
> understand that if your query gets executed, it's going to consume
> resources on the thing that's executing it (for the record, though,
> there are people working on constraining that). But introducing
> disproportionate resource consumption into all traffic-inspecting
> software, like pools and bouncers, seems like a different thing to me.
> Many use cases are going to be fine with it, of course, but I don't
> think it should be hand-waved.

I can't follow this argument.

I think it's important that the startup message is always sent
uncompressed, because it's a strange exception to our usual
message-formatting rules, and because it's so security-critical. I
don't think we should do anything to allow more variation there,
because any benefit will be small and the chances of introducing
security vulnerabilities seems non-trivial.

But if the client says in the startup message that it would like to
send and receive compressed data and the server is happy with that
request, I don't see why we need to postpone implementing that request
until after the authentication exchange is completed. I think that
will make the code more complicated and I don't see a security
benefit. If the use of a particular compression algorithm is going to
impose too much load, the server, or the pooler, is free to refuse it,
and should. Deferring the use of the compression method until after
authentication doesn't really solve any problem here, at least not
that I can see.

It does occur to me that if some compression algorithm has a buffer
overrun bug, restricting its use until after authentication might
reduce the score of the resulting CVE, because now you have to be able
to authenticate to make an exploit work. Perhaps that's an argument
for imposing a restriction here, but it doesn't seem to be the
argument that you're making.

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

On Mon, May 20, 2024 at 8:29 AM Robert Haas <robertmhaas@gmail.com> wrote:
> It does occur to me that if some compression algorithm has a buffer
> overrun bug, restricting its use until after authentication might
> reduce the score of the resulting CVE, because now you have to be able
> to authenticate to make an exploit work. Perhaps that's an argument
> for imposing a restriction here, but it doesn't seem to be the
> argument that you're making.

It wasn't my argument; Jacob B said above:

> in general postgres servers are usually connected to by
> clients that the server admin has some channel to talk to (after all
> they somehow had to get access to log in to the server in the first
> place) if they are doing something wasteful, given that a client can
> do a lot worse things than enable aggressive compression by writing
> bad queries.

...and my response was that, no, the proposal doesn't seem to be
requiring that authentication take place before compression is done.
(As evidenced by your email. :D) If the claim is that there are no
security problems with letting unauthenticated clients force
decompression, then I can try to poke holes in that; or if the claim
is that we don't need to worry about that at all because we'll wait
until after authentication, then I can poke holes in that too. My
request is just that we choose one.

> But if the client says in the startup message that it would like to
> send and receive compressed data and the server is happy with that
> request, I don't see why we need to postpone implementing that request
> until after the authentication exchange is completed. I think that
> will make the code more complicated and I don't see a security
> benefit.

I haven't implemented compression bombs before to know lots of
details, but I think the general idea is to take up resources that are
vastly disproportionate to the effort expended by the client. The
systemic risk is then more or less multiplied by the number of
intermediaries that need to do the decompression. Maybe all three of
our algorithms are hardened against malicious compression techniques;
that'd be great. But if we've never had a situation where a completely
untrusted peer can hand a blob to the server and say "here, decompress
this for me", maybe we'd better check?

--Jacob

On Mon, May 20, 2024 at 12:49 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> ...and my response was that, no, the proposal doesn't seem to be
> requiring that authentication take place before compression is done.
> (As evidenced by your email. :D) If the claim is that there are no
> security problems with letting unauthenticated clients force
> decompression, then I can try to poke holes in that;

I would prefer this approach, so I suggest trying to poke holes here
first. If you find big enough holes then...

> or if the claim
> is that we don't need to worry about that at all because we'll wait
> until after authentication, then I can poke holes in that too. My
> request is just that we choose one.

...we can fall back to this and you can try to poke holes here.

I really hope that you can't poke big enough holes to kill the feature
entirely, though. Because that sounds sad.

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

On Mon, May 20, 2024 at 10:01 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I really hope that you can't poke big enough holes to kill the feature
> entirely, though. Because that sounds sad.

Even if there are holes, I don't think the situation's going to be bad
enough to tank everything; otherwise no one would be able to use
decompression on the Internet. :D And I expect the authors of the
newer compression methods to have thought about these things [1].

I hesitate to ask as part of the same email, but what were the plans
for compression in combination with transport encryption? (Especially
if you plan to compress the authentication exchange, since mixing your
LDAP password into the compression context seems like it might be a
bad idea if you don't want to leak it.)

--Jacob

[1] https://datatracker.ietf.org/doc/html/rfc8878#name-security-considerations

On Mon, May 20, 2024 at 1:23 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> On Mon, May 20, 2024 at 10:01 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > I really hope that you can't poke big enough holes to kill the feature
> > entirely, though. Because that sounds sad.
>
> Even if there are holes, I don't think the situation's going to be bad
> enough to tank everything; otherwise no one would be able to use
> decompression on the Internet. :D And I expect the authors of the
> newer compression methods to have thought about these things [1].
>
> I hesitate to ask as part of the same email, but what were the plans
> for compression in combination with transport encryption? (Especially
> if you plan to compress the authentication exchange, since mixing your
> LDAP password into the compression context seems like it might be a
> bad idea if you don't want to leak it.)

So, the data would be compressed first, with framing around that, and
then transport encryption would happen afterwards. I don't see how
that would leak your password, but I have a feeling that might be a
sign that I'm about to learn some unpleasant truths.

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

> On 20 May 2024, at 22:48, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 1:23 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>> On Mon, May 20, 2024 at 10:01 AM Robert Haas <robertmhaas@gmail.com> wrote:
>>> I really hope that you can't poke big enough holes to kill the feature
>>> entirely, though. Because that sounds sad.
>>
>> Even if there are holes, I don't think the situation's going to be bad
>> enough to tank everything; otherwise no one would be able to use
>> decompression on the Internet. :D And I expect the authors of the
>> newer compression methods to have thought about these things [1].
>>
>> I hesitate to ask as part of the same email, but what were the plans
>> for compression in combination with transport encryption? (Especially
>> if you plan to compress the authentication exchange, since mixing your
>> LDAP password into the compression context seems like it might be a
>> bad idea if you don't want to leak it.)
>
> So, the data would be compressed first, with framing around that, and
> then transport encryption would happen afterwards. I don't see how
> that would leak your password, but I have a feeling that might be a
> sign that I'm about to learn some unpleasant truths.

Compression defeats encryption. That's why it's not in TLS anymore.
The thing is compression codecs use data self correlation. And if you mix secret data with user's data, user might
guesshow correlated they are. 


Best regards, Andrey Borodin.

On Mon, May 20, 2024 at 2:05 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> Compression defeats encryption. That's why it's not in TLS anymore.
> The thing is compression codecs use data self correlation. And if you mix secret data with user's data, user might
guesshow correlated they are. 

Yeah, I'm aware that there are some problems like this. For example,
suppose the bad guy can both supply some of the data sent over the
connection (e.g. by typing search queries into a web page) and also
observe the traffic between the web application and the database. Then
they could supply data and try to guess how correlated that is with
other data sent over the same connection. But if that's a practical
attack, preventing compression prior to the authentication exchange
probably isn't good enough: the user could also try to guess what
queries are being sent on behalf of other users through the same
pooled connection, or they could try to use the bits of the query that
they can control to guess what the other bits of the query that they
can't see look like.

But, does this mean that we should just refuse to offer compression as
a feature? This kind of attack isn't a threat in every environment,
and in some environments, compression could be pretty useful. For
instance, you might need to pull down a lot of data from the database
over a slow connection. Perhaps you're the only user of the database,
and you wrote all of the queries yourself in a locked vault, accepting
no untrusted inputs. In that case, these kinds of attacks aren't
possible, or at least I don't see how, but you might want both
compression and encryption. I guess I don't understand why TLS removed
support for encryption entirely instead of disclaiming its use in some
appropriate way.

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

> On 20 May 2024, at 23:37, Robert Haas <robertmhaas@gmail.com> wrote:
>
>  But if that's a practical
> attack, preventing compression prior to the authentication exchange
> probably isn't good enough: the user could also try to guess what
> queries are being sent on behalf of other users through the same
> pooled connection, or they could try to use the bits of the query that
> they can control to guess what the other bits of the query that they
> can't see look like.

All these attacks can be practically exploited in a controlled environment.
That's why previous incarnation of this patchset [0] contained a way to reset compression context. And Odyssey AFAIR
didit (Dan, coauthor of that patch, implemented the compression in Odyssey). 
But attacking authentication is much more straightforward and viable.

> On 20 May 2024, at 23:37, Robert Haas <robertmhaas@gmail.com> wrote:
>
> But, does this mean that we should just refuse to offer compression as
> a feature?

No, absolutely, we need the feature.

> I guess I don't understand why TLS removed
> support for encryption entirely instead of disclaiming its use in some
> appropriate way.

I think, the scope of TLS is too broad. HTTPS in turn has a compression. But AFAIK it never compress headers.
IMO we should try to avoid compressing authentication information.


Best regards, Andrey Borodin.

[0] https://commitfest.postgresql.org/38/3499/

On Mon, May 20, 2024 at 11:05 AM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> > So, the data would be compressed first, with framing around that, and
> > then transport encryption would happen afterwards. I don't see how
> > that would leak your password, but I have a feeling that might be a
> > sign that I'm about to learn some unpleasant truths.
>
> Compression defeats encryption. That's why it's not in TLS anymore.
> The thing is compression codecs use data self correlation. And if you mix secret data with user's data, user might
guesshow correlated they are. 

I'm slow on the draw, but I hacked up a sample client to generate
traffic against the compression-enabled server, to try to illustrate.

If my client sends an LDAP password of "hello", followed by the query
`SELECT 'world'`, as part of the same gzip stream, I get two encrypted
packets on the wire: lengths 42 and 49 bytes. If the client instead
sends the query `SELECT 'hello'`, I get lengths 42 and 46. We lost
three bytes, and there's only been one packet on the stream before the
query; if the observer controlled the query, it's pretty obvious that
the self-similarity has to have come from the PasswordMessage. Rinse
and repeat.

That doesn't cover the case where the password itself is low-entropy,
either. "hellohellohellohello" at least has length, but once you
compress it that collapses. So an attacker can passively monitor for
shorter password packets and know which user to target first.

--Jacob

On Mon, May 20, 2024 at 11:37 AM Robert Haas <robertmhaas@gmail.com> wrote:
> But if that's a practical
> attack, preventing compression prior to the authentication exchange
> probably isn't good enough

I mean... you said it, not me. I'm trying not to rain on the parade
too much, because compression is clearly very valuable. But it makes
me really uncomfortable that we're reintroducing the compression
oracle (especially over the authentication exchange, which is
generally more secret than the rest of the traffic).

> But, does this mean that we should just refuse to offer compression as
> a feature? This kind of attack isn't a threat in every environment,
> and in some environments, compression could be pretty useful. For
> instance, you might need to pull down a lot of data from the database
> over a slow connection. Perhaps you're the only user of the database,
> and you wrote all of the queries yourself in a locked vault, accepting
> no untrusted inputs. In that case, these kinds of attacks aren't
> possible, or at least I don't see how, but you might want both
> compression and encryption.

Right, I think it's reasonable to let a sufficiently
determined/informed user lift the guardrails, but first we have to
choose to put guardrails in place... and then we have to somehow
sufficiently inform the users when it's okay to lift them.

> I guess I don't understand why TLS removed
> support for encryption entirely instead of disclaiming its use in some
> appropriate way.

One of the IETF conversations was at [1] (there were dissenters on the
list, as you might expect). My favorite summary is this one from
Alyssa Rowan:

> Compression is usually best performed as "high" as possible; transport layer is blind to what's being compressed,
whichis (as we now know) was definitely too low and was in retrospect a mistake. 
>
> Any application layer protocol needs to know - if compression is supported - to separate compression contexts for
attacker-chosenplaintext and attacker-sought unknown secrets. (As others have stated, HTTPbis covers this.) 

But for SQL, where's the dividing line between attacker-chosen and
attacker-sought? To me, it seems like only the user knows; the server
has no clue. I think that puts us "lower" in Alyssa's model than HTTP
is.

As Andrey points out, there was prior work done that started to take
this into account. I haven't reviewed it to see how good it is -- and
I think there are probably many use cases in which queries and tables
contain both private and attacker-controlled information -- but if we
agree that they have to be separated, then the strategy can at least
be improved upon.

--Jacob

[1] https://mailarchive.ietf.org/arch/msg/tls/xhMLf8j4pq8W_ZGXUUU1G_m6r1c/

On Mon, May 20, 2024 at 4:12 PM Magnus Hagander <magnus@hagander.net> wrote:
> That used to be the case in HTTP/1. But header compression was one of the headline features of HTTP/2, which isn't
exactlynew anymore. But there's a special algorithm, HPACK, for it. And then http/3 uses QPACK. Cloudflare has a pretty
decentblog post explaining why and how: https://blog.cloudflare.com/hpack-the-silent-killer-feature-of-http-2/, or
rfc7541for all the details. 
>
> tl;dr; is yes, let's be careful not to expose headers to a CRIME-style attack. And I doubt our connections has as
muchto gain by compressing "header style" fields as http, so we are probably better off just not compressing >  Work:
https://www.redpill-linpro.com/

What do you think constitutes a header in the context of the
PostgreSQL wire protocol?

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

On Mon, May 20, 2024 at 2:42 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> I mean... you said it, not me. I'm trying not to rain on the parade
> too much, because compression is clearly very valuable. But it makes
> me really uncomfortable that we're reintroducing the compression
> oracle (especially over the authentication exchange, which is
> generally more secret than the rest of the traffic).

As currently implemented, the compression only applies to
CopyData/DataRow/Query messages, none of which should be involved in
authentication, unless I've really missed something in my
understanding.

> Right, I think it's reasonable to let a sufficiently
> determined/informed user lift the guardrails, but first we have to
> choose to put guardrails in place... and then we have to somehow
> sufficiently inform the users when it's okay to lift them.

My thought would be that compression should be opt-in on the client
side, with documentation around the potential security pitfalls. (I
could be convinced it should be opt-in on the server side, but overall
I think opt-in on the client side generally protects against footguns
without excessively getting in the way and if an attacker controls the
client, they can just get the information they want directly-they
don't need compression sidechannels to get that information.)

> But for SQL, where's the dividing line between attacker-chosen and
> attacker-sought? To me, it seems like only the user knows; the server
> has no clue. I think that puts us "lower" in Alyssa's model than HTTP
> is.
>
> As Andrey points out, there was prior work done that started to take
> this into account. I haven't reviewed it to see how good it is -- and
> I think there are probably many use cases in which queries and tables
> contain both private and attacker-controlled information -- but if we
> agree that they have to be separated, then the strategy can at least
> be improved upon.

Within SQL-level things, I don't think we can reasonably differentiate
between private and attacker-controlled information at the
libpq/server level.  We can reasonably differentiate between message
types that *definitely* are private and ones that could have
either/both data in them, but that's not nearly as useful.  I think
not compressing auth-related packets plus giving a mechanism to reset
the compression stream for clients (plus guidance on the tradeoffs
involved in turning on compression) is about as good as we can get.
That said, I *think* the feature is reasonable to be
reviewed/committed without the reset functionality as long as the
compressed data already has the mechanism built in (as it does) to
signal when a decompressor should restart its streaming.  The actual
signaling protocol mechanism/necessary libpq API can happen in
followon work.


--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Mon, 20 May 2024 at 21:42, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> As Andrey points out, there was prior work done that started to take
> this into account. I haven't reviewed it to see how good it is -- and
> I think there are probably many use cases in which queries and tables
> contain both private and attacker-controlled information -- but if we
> agree that they have to be separated, then the strategy can at least
> be improved upon.


To help get everyone on the same page I wanted to list all the
security concerns in one place:

1. Triggering excessive CPU usage before authentication, by asking for
very high compression levels
2. Triggering excessive memory/CPU usage before authentication, by
sending a client sending a zipbomb
3. Triggering excessive CPU after authentication, by asking for a very
high compression level
4. Triggering excessive memory/CPU after authentication due to
zipbombs (i.e. small amount of data extracting to lots of data)
5. CRIME style leakage of information about encrypted data

1 & 2 can easily be solved by not allowing any authentication packets
to be compressed. This also has benefits for 5.

3 & 4 are less of a concern than 1&2 imho. Once authenticated a client
deserves some level of trust. But having knobs to limit impact
definitely seems useful.

3 can be solved in two ways afaict:
a. Allow the server to choose the maximum compression level for each
compression method (using some GUC), and downgrade the level
transparently when a higher level is requested
b. Don't allow the client to choose the compression level that the server uses.

I'd prefer option a

4 would require some safety limits on the amount of data that a
(small) compressed message can be decompressed to, and stop
decompression of that message once that limit is hit. What that limit
should be seems hard to choose though. A few ideas:
a. The size of the message reported by the uncompressed header. This
would mean that at most the 4GB will be uncompressed, since maximum
message length is 4GB (limited by 32bit message length field)
b. Allow servers to specify maximum client decompressed message length
lower than this 4GB, e.g. messages of more than 100MB of uncompressed
size should not be allowed.

I think 5 is the most complicated to deal with, especially as it
depends on the actual usage to know what is safe. I believe we should
let users have the freedom to make their own security tradeoffs, but
we should protect them against some of the most glaring issues
(especially ones that benefit little from compression anyway). As
already shown by Andrey, sending LDAP passwords in a compressed way
seems extremely dangerous. So I think we should disallow compressing
any authentication related packets. To reduce similar risks further we
can choose to compress only the message types that we expect to
benefit most from compression. IMHO those are the following (marked
with (B)ackend or (F)rontend to show who sends them):
- Query (F)
- Parse (F)
- Describe (F)
- Bind (F)
- RowDescription (B)
- DataRow (B)
- CopyData (B/F)

Then I think we should let users choose how they want to compress and
where they want their compression stream to restart. Something like
this:
a. compression_restart=query: Restart the stream after every query.
Recommended if queries across the same connection are triggered by
different end-users. I think this would be a sane default
b. compression_restart=message: Restart the stream for every message.
Recommended if the amount of correlation between rows of the same
query is a security concern.
c. compression_restart=manual: Don't restart the stream automatically,
but only when the client user calls a specific function. Recommended
only if the user can make trade-offs, or if no encryption is used
anyway.

On Tue, May 21, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> To help get everyone on the same page I wanted to list all the
> security concerns in one place:
>
> 1. Triggering excessive CPU usage before authentication, by asking for
> very high compression levels
> 2. Triggering excessive memory/CPU usage before authentication, by
> sending a client sending a zipbomb
> 3. Triggering excessive CPU after authentication, by asking for a very
> high compression level
> 4. Triggering excessive memory/CPU after authentication due to
> zipbombs (i.e. small amount of data extracting to lots of data)
> 5. CRIME style leakage of information about encrypted data
>
> 1 & 2 can easily be solved by not allowing any authentication packets
> to be compressed. This also has benefits for 5.

This is already addressed by only compressing certain message types.
If we think it is important that the server reject compressed packets
of other types I can add that, but it seemed reasonable to just make
the client never send such packets compressed.

> 3 & 4 are less of a concern than 1&2 imho. Once authenticated a client
> deserves some level of trust. But having knobs to limit impact
> definitely seems useful.
>
> 3 can be solved in two ways afaict:
> a. Allow the server to choose the maximum compression level for each
> compression method (using some GUC), and downgrade the level
> transparently when a higher level is requested
> b. Don't allow the client to choose the compression level that the server uses.
>
> I'd prefer option a

3a would seem preferable given discussion upthread. It would probably
be worth doing some measurement to check how much of an actual
difference in compute effort the max vs the default for all 3
algorithms adds, because I would really prefer to avoid needing to add
even more configuration knobs if the max compression level for the
streaming data usecase is sufficiently performant.

> 4 would require some safety limits on the amount of data that a
> (small) compressed message can be decompressed to, and stop
> decompression of that message once that limit is hit. What that limit
> should be seems hard to choose though. A few ideas:
> a. The size of the message reported by the uncompressed header. This
> would mean that at most the 4GB will be uncompressed, since maximum
> message length is 4GB (limited by 32bit message length field)
> b. Allow servers to specify maximum client decompressed message length
> lower than this 4GB, e.g. messages of more than 100MB of uncompressed
> size should not be allowed.

Because we are using streaming decompression, this is much less of an
issue than for things that decompress wholesale onto disk/into memory.
We only read PQ_RECV_BUFFER_SIZE (8k) bytes off the stream at once,
and when reading a packet we already have a `maxmsglen` that is
PQ_LARGE_MESSAGE_LIMIT (1gb) already, and "We abort the connection (by
returning EOF) if client tries to send more than that.)".  Therefore,
we effectively already have a limit of 1gb that applies to regular
messages too, and I think we should rely on this mechanism for
compressed data too (if we really think we need to make that number
configurable we probably could, but again the fewer new knobs we need
to add the better.


> I think 5 is the most complicated to deal with, especially as it
> depends on the actual usage to know what is safe. I believe we should
> let users have the freedom to make their own security tradeoffs, but
> we should protect them against some of the most glaring issues
> (especially ones that benefit little from compression anyway). As
> already shown by Andrey, sending LDAP passwords in a compressed way
> seems extremely dangerous. So I think we should disallow compressing
> any authentication related packets. To reduce similar risks further we
> can choose to compress only the message types that we expect to
> benefit most from compression. IMHO those are the following (marked
> with (B)ackend or (F)rontend to show who sends them):
> - Query (F)
> - Parse (F)
> - Describe (F)
> - Bind (F)
> - RowDescription (B)
> - DataRow (B)
> - CopyData (B/F)

That seems like a reasonable list (current implementation is just
CopyData/DataRow/Query, but I really just copied that fairly blindly
from the previous incarnation of this effort.) See also my comment
below 1&2 for if we think we need to block decompressing them too.

> Then I think we should let users choose how they want to compress and
> where they want their compression stream to restart. Something like
> this:
> a. compression_restart=query: Restart the stream after every query.
> Recommended if queries across the same connection are triggered by
> different end-users. I think this would be a sane default
> b. compression_restart=message: Restart the stream for every message.
> Recommended if the amount of correlation between rows of the same
> query is a security concern.
> c. compression_restart=manual: Don't restart the stream automatically,
> but only when the client user calls a specific function. Recommended
> only if the user can make trade-offs, or if no encryption is used
> anyway.

I reasonably like this idea, though I think maybe we should also
(instead of query?) add per-transaction on the backend side.  I'm
curious what other people think of this.


--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Tue, May 21, 2024 at 8:23 AM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> As currently implemented, the compression only applies to
> CopyData/DataRow/Query messages, none of which should be involved in
> authentication, unless I've really missed something in my
> understanding.

Right, but Robert has argued that we should compress it all, and I'm
responding to that proposal.

Sorry for introducing threads within threads. But I think it's
valuable to pin down both 1) the desired behavior, and 2) how the
current proposal behaves, as two separate things. I'll try to do a
better job of communicating which I'm talking about.

> > Right, I think it's reasonable to let a sufficiently
> > determined/informed user lift the guardrails, but first we have to
> > choose to put guardrails in place... and then we have to somehow
> > sufficiently inform the users when it's okay to lift them.
>
> My thought would be that compression should be opt-in on the client
> side, with documentation around the potential security pitfalls. (I
> could be convinced it should be opt-in on the server side, but overall
> I think opt-in on the client side generally protects against footguns
> without excessively getting in the way

We absolutely have to document the risks and allow clients to be
written safely. But I think server-side controls on risky behavior
have proven to be generally more valuable, because the server
administrator is often in a better spot to see the overall risks to
the system. ("No, you will not use deprecated ciphersuites. No, you
will not access this URL over plaintext. No, I will not compress this
response containing customer credit card numbers, no matter how nicely
you ask.") There are many more clients than servers, so it's less
risky for the server to enforce safety than to hope that every client
is safe.

Does your database and access pattern regularly mingle secrets with
public data? Would auditing correct client use of compression be a
logistical nightmare? Do your app developers keep indicating in
conversations that they don't understand the risks at all? Cool, just
set `encrypted_compression = nope_nope_nope` on the server and sleep
soundly at night. (Ideally we would default to that.)

> and if an attacker controls the
> client, they can just get the information they want directly-they
> don't need compression sidechannels to get that information.)

Sure, but I don't think that's relevant to the threats being discussed.

> Within SQL-level things, I don't think we can reasonably differentiate
> between private and attacker-controlled information at the
> libpq/server level.

And by the IETF line of argument -- or at least the argument I quoted
above -- that implies that we really have no business introducing
compression when confidentiality is requested. A stronger approach
would require us to prove, or the user to indicate, safety before
compressing.

Take a look at the security notes for QPACK [1] -- keeping in mind
that they know _more_ about what's going on at the protocol level than
we do, due to the header design. And they still say things like "an
encoder might choose not to index values with low entropy" and "these
criteria ... will evolve over time as new attacks are discovered." A
huge amount is left as an exercise for the reader. This stuff is
really hard.

> We can reasonably differentiate between message
> types that *definitely* are private and ones that could have
> either/both data in them, but that's not nearly as useful.  I think
> not compressing auth-related packets plus giving a mechanism to reset
> the compression stream for clients (plus guidance on the tradeoffs
> involved in turning on compression) is about as good as we can get.

The concept of stream reset seems necessary but insufficient at the
application level, which bleeds over into Jelte's compression_restart
proposal. (At the protocol level, I think it may be sufficient?)

If I write a query where one of the WHERE clauses is
attacker-controlled and the other is a secret, I would really like to
not compress that query on the client side. If I join a table of user
IDs against a table of user-provided addresses and a table of
application tokens for that user, compressing even a single row leaks
information about those tokens -- at a _very_ granular level -- and I
would really like the server not to do that.

So if I'm building sand castles... I think maybe it'd be nice to mark
tables (and/or individual columns?) as safe for compression under
encryption, whether by row or in aggregate. And maybe libpq and psql
should be able to turn outgoing compression on and off at will.

And I understand those would balloon the scope of the feature. I'm
worried I'm doing the security-person thing and sucking all the air
out of the room. I know not everybody uses transport encryption; for
those people, compress-it-all is probably a pretty winning strategy,
and there's no need to reset the compression context ever. And the
pg_dump-style, "give me everything" use case seems like it could maybe
be okay, but I really don't know how to assess the risk there, at all.

> That said, I *think* the feature is reasonable to be
> reviewed/committed without the reset functionality as long as the
> compressed data already has the mechanism built in (as it does) to
> signal when a decompressor should restart its streaming.  The actual
> signaling protocol mechanism/necessary libpq API can happen in
> followon work.

Well... working out the security minutiae _after_ changing the
protocol is not historically a winning strategy, I think. Better to do
it as a vertical stack.

Thanks,
--Jacob

[1] https://www.rfc-editor.org/rfc/rfc9204.html#name-security-considerations

On Tue, May 21, 2024 at 9:14 AM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> On Tue, May 21, 2024 at 10:43 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > To help get everyone on the same page I wanted to list all the
> > security concerns in one place:
> >
> > 1. Triggering excessive CPU usage before authentication, by asking for
> > very high compression levels
> > 2. Triggering excessive memory/CPU usage before authentication, by
> > sending a client sending a zipbomb
> > 3. Triggering excessive CPU after authentication, by asking for a very
> > high compression level
> > 4. Triggering excessive memory/CPU after authentication due to
> > zipbombs (i.e. small amount of data extracting to lots of data)
> > 5. CRIME style leakage of information about encrypted data
> >
> > 1 & 2 can easily be solved by not allowing any authentication packets
> > to be compressed. This also has benefits for 5.
>
> This is already addressed by only compressing certain message types.
> If we think it is important that the server reject compressed packets
> of other types I can add that, but it seemed reasonable to just make
> the client never send such packets compressed.

If the server doesn't reject compressed packets pre-authentication,
then case 2 isn't mitigated. (I haven't proven how risky that case is
yet, to be clear.) In other words: if the threat model is that a
client can attack us, we shouldn't assume that it will attack us
politely.

> > 4 would require some safety limits on the amount of data that a
> > (small) compressed message can be decompressed to, and stop
> > decompression of that message once that limit is hit. What that limit
> > should be seems hard to choose though. A few ideas:
> > a. The size of the message reported by the uncompressed header. This
> > would mean that at most the 4GB will be uncompressed, since maximum
> > message length is 4GB (limited by 32bit message length field)
> > b. Allow servers to specify maximum client decompressed message length
> > lower than this 4GB, e.g. messages of more than 100MB of uncompressed
> > size should not be allowed.
>
> Because we are using streaming decompression, this is much less of an
> issue than for things that decompress wholesale onto disk/into memory.

(I agree in general, but since you're designing a protocol extension,
IMO it's not enough that your implementation happens to mitigate
risks. We more or less have to bake those mitigations into the
specification of the extension, because things that aren't servers
have to decompress now. Similar to RFC security considerations.)

--Jacob

On Tue, May 21, 2024 at 1:39 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> If the server doesn't reject compressed packets pre-authentication,
> then case 2 isn't mitigated. (I haven't proven how risky that case is
> yet, to be clear.) In other words: if the threat model is that a
> client can attack us, we shouldn't assume that it will attack us
> politely.

I think I thought I was writing about something else when I wrote that
:sigh:.  I think what I really should have written was a version of
the part below, which is that we use streaming decompression, only
decompress 8kb at a time, and for pre-auth messages limit them to
`PG_MAX_AUTH_TOKEN_LENGTH` (65535 bytes), which isn't really enough
data to actually cause any real-world pain by needing to decompress vs
the equivalent pain of sending invalid uncompressed auth packets.

> > Because we are using streaming decompression, this is much less of an
> > issue than for things that decompress wholesale onto disk/into memory.
>
> (I agree in general, but since you're designing a protocol extension,
> IMO it's not enough that your implementation happens to mitigate
> risks. We more or less have to bake those mitigations into the
> specification of the extension, because things that aren't servers
> have to decompress now. Similar to RFC security considerations.)

We own both the canonical client and server, so those are both covered
here.  I would think it would be the responsibility of any other
system that maintains its own implementation of the postgres protocol
and chooses to support the compression protocol to perform its own
mitigations against potential compression security issues.  Should we
put the fixed message size limits (that have de facto been part of the
protocol since 2021, even if they weren't documented as such) into the
protocol documentation?  That would give implementers of the protocol
numbers that they could actually rely on when implementing the
appropriate safeguards because they would be able to actually have
explicit guarantees about the size of messages. I think it would make
more sense to put the limits on the underlying messages rather than
adding an additional limit that only applies to compressed messages.
( I don't really see how one could implement other tooling that used
pg compression without using streaming compression, as the protocol
never hands over a standalone blob of compressed data: all compressed
data is always part of a stream, but even with streaming decompression
you still need some kind of limits or you will just chew up memory.)

--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Tue, May 21, 2024 at 1:24 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> We absolutely have to document the risks and allow clients to be
> written safely. But I think server-side controls on risky behavior
> have proven to be generally more valuable, because the server
> administrator is often in a better spot to see the overall risks to
> the system. ("No, you will not use deprecated ciphersuites. No, you
> will not access this URL over plaintext. No, I will not compress this
> response containing customer credit card numbers, no matter how nicely
> you ask.") There are many more clients than servers, so it's less
> risky for the server to enforce safety than to hope that every client
> is safe.
>
> Does your database and access pattern regularly mingle secrets with
> public data? Would auditing correct client use of compression be a
> logistical nightmare? Do your app developers keep indicating in
> conversations that they don't understand the risks at all? Cool, just
> set `encrypted_compression = nope_nope_nope` on the server and sleep
> soundly at night. (Ideally we would default to that.)

Thinking about this more (and adding a encrypted_compression GUC or
whatever), I think my inclination would on the server-side default
enable compression for insecure connections but default disable for
encrypted connections, but both would be config parameters that can be
changed as desired.

> The concept of stream reset seems necessary but insufficient at the
> application level, which bleeds over into Jelte's compression_restart
> proposal. (At the protocol level, I think it may be sufficient?)
>
> If I write a query where one of the WHERE clauses is
> attacker-controlled and the other is a secret, I would really like to
> not compress that query on the client side. If I join a table of user
> IDs against a table of user-provided addresses and a table of
> application tokens for that user, compressing even a single row leaks
> information about those tokens -- at a _very_ granular level -- and I
> would really like the server not to do that.
>
> So if I'm building sand castles... I think maybe it'd be nice to mark
> tables (and/or individual columns?) as safe for compression under
> encryption, whether by row or in aggregate. And maybe libpq and psql
> should be able to turn outgoing compression on and off at will.
>
> And I understand those would balloon the scope of the feature. I'm
> worried I'm doing the security-person thing and sucking all the air
> out of the room. I know not everybody uses transport encryption; for
> those people, compress-it-all is probably a pretty winning strategy,
> and there's no need to reset the compression context ever. And the
> pg_dump-style, "give me everything" use case seems like it could maybe
> be okay, but I really don't know how to assess the risk there, at all.

I would imagine that a large volume of uses of postgres are in
contexts (e.g. internal networks) where either no encryption is used
or even when encryption is used the benefit of compression vs the risk
of someone being a position to perform a BREACH-style sidechannel
attack against DB traffic is sufficiently high that compress-it-all
would be be quite useful in many cases.  Would some sort of
per-table/column marking be useful for some cases?  Probably, but that
doesn't seem to me like it needs to be in v1 of this feature as long
as the protocol layer itself is designed such that parties can
arbitrarily alternate between transmitting compressed and uncompressed
data.  Then if we build such a feature down the road we just add logic
around *when* we compress but the protocol layer doesn't change.

> Well... working out the security minutiae _after_ changing the
> protocol is not historically a winning strategy, I think. Better to do
> it as a vertical stack.

Thinking about it more, I agree that we probably should work out the
protocol level mechanism for resetting compression
context/enabling/disabling/reconfiguring compression as part of this
work.  I don't think that we need to have all the ways that the
application layer might choose to use such things done here, but we
should have all the necessary primitives worked out.

--
Jacob Burroughs | Staff Software Engineer
E: jburroughs@instructure.com

On Tue, May 21, 2024 at 12:08 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> I think I thought I was writing about something else when I wrote that
> :sigh:.  I think what I really should have written was a version of
> the part below, which is that we use streaming decompression, only
> decompress 8kb at a time, and for pre-auth messages limit them to
> `PG_MAX_AUTH_TOKEN_LENGTH` (65535 bytes), which isn't really enough
> data to actually cause any real-world pain by needing to decompress vs
> the equivalent pain of sending invalid uncompressed auth packets.

Okay. So it sounds like your position is similar to Robert's from
earlier: prefer allowing unauthenticated compressed packets for
simplicity, as long as we think it's safe for the server. (Personally
I still think a client that compresses its password packets is doing
it wrong, and we could help them out by refusing that.)

> We own both the canonical client and server, so those are both covered
> here.  I would think it would be the responsibility of any other
> system that maintains its own implementation of the postgres protocol
> and chooses to support the compression protocol to perform its own
> mitigations against potential compression security issues.

Sure, but if our official documentation is "here's an extremely
security-sensitive feature, figure it out!" then we've done a
disservice to the community.

> Should we
> put the fixed message size limits (that have de facto been part of the
> protocol since 2021, even if they weren't documented as such) into the
> protocol documentation?

Possibly? I don't know if the other PG-compatible implementations use
the same limits. It might be better to say "limits must exist".

> ( I don't really see how one could implement other tooling that used
> pg compression without using streaming compression, as the protocol
> never hands over a standalone blob of compressed data: all compressed
> data is always part of a stream, but even with streaming decompression
> you still need some kind of limits or you will just chew up memory.)

Well, that's a good point; I wasn't thinking about the streaming APIs
themselves. If the easiest way to implement decompression requires the
use of an API that shouts "hey, give me guardrails!", then that helps
quite a bit. I really need to look into the attack surface of the
three algorithms.

--Jacob