Re: Disallow SSL compression?
On Sat, Mar 06, 2021 at 10:39:52AM +0900, Michael Paquier wrote: > Okay, cool. I'd rather wait more for Peter before doing anything, so > if there are no objections, I'll look at that stuff again at the > beginning of next week and perhaps apply it. If you wish to take care > of that yourself, please feel free to do so, of course. So, I have looked at the proposed patch in details, fixed the documentation of pg_stat_ssl where compression was still listed, checked a couple of things with and without OpenSSL, across past major PG versions with OpenSSL 1.0.2 to see if compression was getting disabled correctly. And things look all good, so applied. -- Michael signature.asc Description: PGP signature
Re: Disallow SSL compression?
On Fri, Mar 05, 2021 at 05:44:20PM +0100, Magnus Hagander wrote: > We've broken stats views before. While it'd be nice if we could group > multiple breakages at the same time, I don't think it's that > important. Better to get rid of it once and for all from as many > places as possible. Okay, cool. I'd rather wait more for Peter before doing anything, so if there are no objections, I'll look at that stuff again at the beginning of next week and perhaps apply it. If you wish to take care of that yourself, please feel free to do so, of course. -- Michael signature.asc Description: PGP signature
Re: Disallow SSL compression?
On Fri, Mar 5, 2021 at 1:37 PM Michael Paquier wrote: > > On Fri, Mar 05, 2021 at 01:21:01PM +0100, Daniel Gustafsson wrote: > > On 5 Mar 2021, at 08:04, Michael Paquier wrote: > >> FWIW, I would vote to nuke it from all those places, reducing a bit > >> pg_stat_get_activity() while on it. Keeping it around in the system > >> catalogs may cause confusion IMHO, by making people think that it is > >> still possible to get into configurations where sslcompression could > >> be really enabled. The rest of the patch looks fine to me. > > > > Attached is a version which removes that as well. > > Peter, Magnus, any comments about this point? We've broken stats views before. While it'd be nice if we could group multiple breakages at the same time, I don't think it's that important. Better to get rid of it once and for all from as many places as possible. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: Disallow SSL compression?
On Fri, Mar 05, 2021 at 01:21:01PM +0100, Daniel Gustafsson wrote: > On 5 Mar 2021, at 08:04, Michael Paquier wrote: >> FWIW, I would vote to nuke it from all those places, reducing a bit >> pg_stat_get_activity() while on it. Keeping it around in the system >> catalogs may cause confusion IMHO, by making people think that it is >> still possible to get into configurations where sslcompression could >> be really enabled. The rest of the patch looks fine to me. > > Attached is a version which removes that as well. Peter, Magnus, any comments about this point? > I left the compression > keyword in PQsslAttribute on purpose, not really for backwards compatibility > (PQsslAttributeNames takes care of that) but rather since it's a more generic > connection-info function. Makes sense. -- Michael signature.asc Description: PGP signature
Re: Disallow SSL compression?
> On 5 Mar 2021, at 08:04, Michael Paquier wrote: > > On Thu, Mar 04, 2021 at 11:52:56PM +0100, Daniel Gustafsson wrote: >> The attached version takes a step further and removes sslcompression from >> pg_conn and just eats the value as there is no use in setting a dummy alue. >> It >> also removes compression from PgBackendSSLStatus and be_tls_get_compression >> as >> raised by Michael downthread. I opted for keeping the column in pg_stat_ssl >> with a note in the documentation that it will be removed, for the same >> backwards compatibility reason of eating the connection param without acting >> on >> it. This might be overthinking it however. > > FWIW, I would vote to nuke it from all those places, reducing a bit > pg_stat_get_activity() while on it. Keeping it around in the system > catalogs may cause confusion IMHO, by making people think that it is > still possible to get into configurations where sslcompression could > be really enabled. The rest of the patch looks fine to me. Attached is a version which removes that as well. I left the compression keyword in PQsslAttribute on purpose, not really for backwards compatibility (PQsslAttributeNames takes care of that) but rather since it's a more generic connection-info function. -- Daniel Gustafsson https://vmware.com/ v5-0001-Disallow-SSL-compression.patch Description: Binary data
Re: Disallow SSL compression?
On Thu, Mar 04, 2021 at 11:52:56PM +0100, Daniel Gustafsson wrote: > The attached version takes a step further and removes sslcompression from > pg_conn and just eats the value as there is no use in setting a dummy alue. > It > also removes compression from PgBackendSSLStatus and be_tls_get_compression as > raised by Michael downthread. I opted for keeping the column in pg_stat_ssl > with a note in the documentation that it will be removed, for the same > backwards compatibility reason of eating the connection param without acting > on > it. This might be overthinking it however. FWIW, I would vote to nuke it from all those places, reducing a bit pg_stat_get_activity() while on it. Keeping it around in the system catalogs may cause confusion IMHO, by making people think that it is still possible to get into configurations where sslcompression could be really enabled. The rest of the patch looks fine to me. -- Michael signature.asc Description: PGP signature
Re: Disallow SSL compression?
> On 4 Mar 2021, at 11:59, Michael Paquier wrote: > > On Wed, Mar 03, 2021 at 03:14:01PM +0100, Peter Eisentraut wrote: >> Per your other thread, you should also remove the environment variable. >> >> In postgres_fdw, I think commenting it out is not the right change. The >> other commented out values are still valid settings but are omitted from the >> test for other reasons. It's not entirely all clear, but we don't have to >> keep obsolete stuff in there forever. > > Agreed on both points. Also, it seems a bit weird to keep around > pg_stat_ssl.compression while we know that it will always be false. > Wouldn't it be better to get rid of that in PgBackendSSLStatus and > pg_stat_ssl then? Fixed in the v4 posted just now, although I opted for keeping the column in pg_stat_ssl for backwards compatibility with a doc note. -- Daniel Gustafsson https://vmware.com/
Re: Disallow SSL compression?
> On 3 Mar 2021, at 15:14, Peter Eisentraut > wrote: > > On 03.03.21 11:31, Daniel Gustafsson wrote: >>> On 26 Feb 2021, at 20:34, Daniel Gustafsson wrote: >>> Attached is a v2 which retains the sslcompression parameter for backwards >>> compatibility. >> And now a v3 which fixes an oversight in postgres_fdw as well as adds an SSL >> TAP test to cover deprecated parameters. > > Per your other thread, you should also remove the environment variable. Fixed. > In postgres_fdw, I think commenting it out is not the right change. The > other commented out values are still valid settings but are omitted from the > test for other reasons. It's not entirely all clear, but we don't have to > keep obsolete stuff in there forever. Ah, I didn't get that distinction but that makes sense. Fixed. The attached version takes a step further and removes sslcompression from pg_conn and just eats the value as there is no use in setting a dummy alue. It also removes compression from PgBackendSSLStatus and be_tls_get_compression as raised by Michael downthread. I opted for keeping the column in pg_stat_ssl with a note in the documentation that it will be removed, for the same backwards compatibility reason of eating the connection param without acting on it. This might be overthinking it however. -- Daniel Gustafsson https://vmware.com/ v4-0001-Disallow-SSL-compression.patch Description: Binary data
Re: Disallow SSL compression?
On Wed, Mar 03, 2021 at 03:14:01PM +0100, Peter Eisentraut wrote: > Per your other thread, you should also remove the environment variable. > > In postgres_fdw, I think commenting it out is not the right change. The > other commented out values are still valid settings but are omitted from the > test for other reasons. It's not entirely all clear, but we don't have to > keep obsolete stuff in there forever. Agreed on both points. Also, it seems a bit weird to keep around pg_stat_ssl.compression while we know that it will always be false. Wouldn't it be better to get rid of that in PgBackendSSLStatus and pg_stat_ssl then? -- Michael signature.asc Description: PGP signature
Re: Disallow SSL compression?
On 03.03.21 11:31, Daniel Gustafsson wrote: On 26 Feb 2021, at 20:34, Daniel Gustafsson wrote: Attached is a v2 which retains the sslcompression parameter for backwards compatibility. And now a v3 which fixes an oversight in postgres_fdw as well as adds an SSL TAP test to cover deprecated parameters. Per your other thread, you should also remove the environment variable. In postgres_fdw, I think commenting it out is not the right change. The other commented out values are still valid settings but are omitted from the test for other reasons. It's not entirely all clear, but we don't have to keep obsolete stuff in there forever.
Re: Disallow SSL compression?
> On 26 Feb 2021, at 20:34, Daniel Gustafsson wrote: > Attached is a v2 which retains the sslcompression parameter for backwards > compatibility. And now a v3 which fixes an oversight in postgres_fdw as well as adds an SSL TAP test to cover deprecated parameters. -- Daniel Gustafsson https://vmware.com/ v3-0001-Disallow-SSL-compression.patch Description: Binary data
Re: Disallow SSL compression?
> On 26 Feb 2021, at 03:34, Michael Paquier wrote: > There is just pain waiting ahead when breaking connection strings > that used to work previously. A "while" could take a long time > though, see the case of "tty" that's still around (cb7fb3c). I see your tty removal from 2003, and raise you one "authtype" which was axed on January 26 1998 in commit d5bbe2aca55bc833e38c768d7f82, but which is still around. More on that in a separate thread though. -- Daniel Gustafsson https://vmware.com/
Re: Disallow SSL compression?
> On 26 Feb 2021, at 11:02, Magnus Hagander wrote: > On Mon, Feb 22, 2021 at 12:28 PM Daniel Gustafsson wrote: >> >>> On 22 Feb 2021, at 11:52, Magnus Hagander wrote: >>> Agreed. It will also help with not having to implement it in new SSL >>> implementations I'm sure :) >> >> Not really, no other TLS library I would consider using actually has >> compression (except maybe wolfSSL?). GnuTLS and NSS both removed it, and >> Secure Transport and Schannel never had it AFAIK. > > Ah, well, you'd still have to implement some empty placeholder :) Correct. The attached removes sslcompression to see what it would look like. The server actively disallows it and the parameter is removed, but the sslcompression column in the stat view is retained. An alternative could be to retain the parameter but not act on it in order to not break scripts etc, but that just postpones the pain until when we inevitably do remove it. Thoughts? Any reason to keep supporting SSL compression or is it time for v14 to remove it? Are there still users leveraging this for protocol compression without security making it worthwhile to keep? >>> >>> When the last round of discussion happened, I had multiple customers >>> who did exactly that. None of them do that anymore, due to the pain of >>> making it work... >> >> Unsurprisingly. >> >>> I think for libpq we want to keep the option for a while but making it >>> a no-op, to not unnecessarily break systems where people just upgrade >>> libpq, though. And document it as such having no effect and "will >>> eventually be removed". >> >> Agreed, that's better. > > In fact, pg_basebackup with -R will generate a connection string that > includes sslcompression=0 when used today (unless you jump through the > hoops to make it work), so not accepting that noe would definitely > break a lot of things needlessly. Yup, and as mentioned elsewhere in the thread the standard way of doing it is to leave the param behind and just document it as not in use. Attached is a v2 which retains the sslcompression parameter for backwards compatibility. -- Daniel Gustafsson https://vmware.com/ v2-0001-Disallow-SSL-compression.patch Description: Binary data
Re: Disallow SSL compression?
On Mon, Feb 22, 2021 at 12:28 PM Daniel Gustafsson wrote: > > > On 22 Feb 2021, at 11:52, Magnus Hagander wrote: > > > > On Thu, Feb 18, 2021 at 1:51 PM Daniel Gustafsson wrote: > >> > >> A few years ago we discussed whether to disable SSL compression [0] which > >> ended > >> up with it being off by default combined with a recommendation against it > >> in > >> the docs. > >> > >> OpenSSL themselves disabled SSL compression by default in 2016 in 1.1.0 > >> with > >> distros often having had it disabled for a long while before then. > >> Further, > >> TLSv1.3 removes compression entirely on the protocol level mandating that > >> only > >> NULL compression is allowed in the ClientHello. NSS, which is discussed in > >> another thread, removed SSL compression entirely in version 3.33 in 2017. > >> > >> It seems about time to revisit this since it's unlikely to work anywhere > >> but in > >> a very small subset of system setups (being disabled by default > >> everywhere) and > >> is thus likely to be very untested at best. There is also the security > >> aspect > >> which is less clear-cut for us compared to HTTP client/servers, but not > >> refuted > >> (the linked thread has a good discussion on this). > > > > Agreed. It will also help with not having to implement it in new SSL > > implementations I'm sure :) > > Not really, no other TLS library I would consider using actually has > compression (except maybe wolfSSL?). GnuTLS and NSS both removed it, and > Secure Transport and Schannel never had it AFAIK. Ah, well, you'd still have to implement some empty placeholder :) > >> The attached removes sslcompression to see what it would look like. The > >> server > >> actively disallows it and the parameter is removed, but the sslcompression > >> column in the stat view is retained. An alternative could be to retain the > >> parameter but not act on it in order to not break scripts etc, but that > >> just > >> postpones the pain until when we inevitably do remove it. > >> > >> Thoughts? Any reason to keep supporting SSL compression or is it time for > >> v14 > >> to remove it? Are there still users leveraging this for protocol > >> compression > >> without security making it worthwhile to keep? > > > > When the last round of discussion happened, I had multiple customers > > who did exactly that. None of them do that anymore, due to the pain of > > making it work... > > Unsurprisingly. > > > I think for libpq we want to keep the option for a while but making it > > a no-op, to not unnecessarily break systems where people just upgrade > > libpq, though. And document it as such having no effect and "will > > eventually be removed". > > Agreed, that's better. In fact, pg_basebackup with -R will generate a connection string that includes sslcompression=0 when used today (unless you jump through the hoops to make it work), so not accepting that noe would definitely break a lot of things needlessly. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: Disallow SSL compression?
On Mon, Feb 22, 2021 at 12:27:38PM +0100, Daniel Gustafsson wrote: > On 22 Feb 2021, at 11:52, Magnus Hagander wrote: >> I think for libpq we want to keep the option for a while but making it >> a no-op, to not unnecessarily break systems where people just upgrade >> libpq, though. And document it as such having no effect and "will >> eventually be removed". > > Agreed, that's better. +1. There is just pain waiting ahead when breaking connection strings that used to work previously. A "while" could take a long time though, see the case of "tty" that's still around (cb7fb3c). Could you update the patch to do that? This requires an update of fe-connect.c and libpq.sgml. -- Michael signature.asc Description: PGP signature
Re: Disallow SSL compression?
> On 22 Feb 2021, at 11:52, Magnus Hagander wrote: > > On Thu, Feb 18, 2021 at 1:51 PM Daniel Gustafsson wrote: >> >> A few years ago we discussed whether to disable SSL compression [0] which >> ended >> up with it being off by default combined with a recommendation against it in >> the docs. >> >> OpenSSL themselves disabled SSL compression by default in 2016 in 1.1.0 with >> distros often having had it disabled for a long while before then. Further, >> TLSv1.3 removes compression entirely on the protocol level mandating that >> only >> NULL compression is allowed in the ClientHello. NSS, which is discussed in >> another thread, removed SSL compression entirely in version 3.33 in 2017. >> >> It seems about time to revisit this since it's unlikely to work anywhere but >> in >> a very small subset of system setups (being disabled by default everywhere) >> and >> is thus likely to be very untested at best. There is also the security >> aspect >> which is less clear-cut for us compared to HTTP client/servers, but not >> refuted >> (the linked thread has a good discussion on this). > > Agreed. It will also help with not having to implement it in new SSL > implementations I'm sure :) Not really, no other TLS library I would consider using actually has compression (except maybe wolfSSL?). GnuTLS and NSS both removed it, and Secure Transport and Schannel never had it AFAIK. >> The attached removes sslcompression to see what it would look like. The >> server >> actively disallows it and the parameter is removed, but the sslcompression >> column in the stat view is retained. An alternative could be to retain the >> parameter but not act on it in order to not break scripts etc, but that just >> postpones the pain until when we inevitably do remove it. >> >> Thoughts? Any reason to keep supporting SSL compression or is it time for >> v14 >> to remove it? Are there still users leveraging this for protocol compression >> without security making it worthwhile to keep? > > When the last round of discussion happened, I had multiple customers > who did exactly that. None of them do that anymore, due to the pain of > making it work... Unsurprisingly. > I think for libpq we want to keep the option for a while but making it > a no-op, to not unnecessarily break systems where people just upgrade > libpq, though. And document it as such having no effect and "will > eventually be removed". Agreed, that's better. -- Daniel Gustafsson https://vmware.com/
Re: Disallow SSL compression?
On Thu, Feb 18, 2021 at 1:51 PM Daniel Gustafsson wrote: > > A few years ago we discussed whether to disable SSL compression [0] which > ended > up with it being off by default combined with a recommendation against it in > the docs. > > OpenSSL themselves disabled SSL compression by default in 2016 in 1.1.0 with > distros often having had it disabled for a long while before then. Further, > TLSv1.3 removes compression entirely on the protocol level mandating that only > NULL compression is allowed in the ClientHello. NSS, which is discussed in > another thread, removed SSL compression entirely in version 3.33 in 2017. > > It seems about time to revisit this since it's unlikely to work anywhere but > in > a very small subset of system setups (being disabled by default everywhere) > and > is thus likely to be very untested at best. There is also the security aspect > which is less clear-cut for us compared to HTTP client/servers, but not > refuted > (the linked thread has a good discussion on this). Agreed. It will also help with not having to implement it in new SSL implementations I'm sure :) > The attached removes sslcompression to see what it would look like. The > server > actively disallows it and the parameter is removed, but the sslcompression > column in the stat view is retained. An alternative could be to retain the > parameter but not act on it in order to not break scripts etc, but that just > postpones the pain until when we inevitably do remove it. > > Thoughts? Any reason to keep supporting SSL compression or is it time for v14 > to remove it? Are there still users leveraging this for protocol compression > without security making it worthwhile to keep? When the last round of discussion happened, I had multiple customers who did exactly that. None of them do that anymore, due to the pain of making it work... I think for libpq we want to keep the option for a while but making it a no-op, to not unnecessarily break systems where people just upgrade libpq, though. And document it as such having no effect and "will eventually be removed". -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Disallow SSL compression?
A few years ago we discussed whether to disable SSL compression [0] which ended up with it being off by default combined with a recommendation against it in the docs. OpenSSL themselves disabled SSL compression by default in 2016 in 1.1.0 with distros often having had it disabled for a long while before then. Further, TLSv1.3 removes compression entirely on the protocol level mandating that only NULL compression is allowed in the ClientHello. NSS, which is discussed in another thread, removed SSL compression entirely in version 3.33 in 2017. It seems about time to revisit this since it's unlikely to work anywhere but in a very small subset of system setups (being disabled by default everywhere) and is thus likely to be very untested at best. There is also the security aspect which is less clear-cut for us compared to HTTP client/servers, but not refuted (the linked thread has a good discussion on this). The attached removes sslcompression to see what it would look like. The server actively disallows it and the parameter is removed, but the sslcompression column in the stat view is retained. An alternative could be to retain the parameter but not act on it in order to not break scripts etc, but that just postpones the pain until when we inevitably do remove it. Thoughts? Any reason to keep supporting SSL compression or is it time for v14 to remove it? Are there still users leveraging this for protocol compression without security making it worthwhile to keep? -- Daniel Gustafsson https://vmware.com/ [0] https://www.postgresql.org/message-id/flat/595cf3b1-4ffe-7f05-6f72-f72b7afa7993%402ndquadrant.com openssl_disallow_compression.patch Description: Binary data
