Thread: Allow ssl_renegotiation_limit in PG 9.5
Hi hackers.
Thanks,
I noticed ssl_renegotiation_limit has been removed in PostgreSQL 9.5, good riddance...
However, the new situation where some versions of PG allow this parameter while others bomb when seeing it. Specifically, Npgsql sends ssl_renegotiation_limit=0 in the startup packet to completely disable renegotiation. At this early stage it doesn't know yet whether the database it's connecting to is PG 9.5 or earlier.
Is there any chance you'd consider allowing ssl_renegotiation_limit in PG 9.5, without it having any effect (I think that's the current behavior for recent 9.4, 9.3, right)? It may be a good idea to only allow this parameter to be set to zero, raising an error otherwise.
The alternative would be to force users to specify in advance whether the database they're connecting to supports this parameter, or to send it after the startup packet which complicates things etc.
Shay
On 2015-10-14 18:53:14 +0300, Shay Rojansky wrote: > However, the new situation where some versions of PG allow this parameter > while others bomb when seeing it. Specifically, Npgsql sends > ssl_renegotiation_limit=0 in the startup packet to completely disable > renegotiation. At this early stage it doesn't know yet whether the database > it's connecting to is PG 9.5 or earlier. I find it a rather debatable practice to send such a parameter unconditionally. Why are you sending it before the connection has even been established? > Is there any chance you'd consider allowing ssl_renegotiation_limit in PG > 9.5, without it having any effect (I think that's the current behavior for > recent 9.4, 9.3, right)? No, you can actually enable renegotiation in those versions, it's just a changed default value. > It may be a good idea to only allow this parameter to be set to zero, > raising an error otherwise. -0.1 from me. Andres
Andres Freund <andres@anarazel.de> writes:
> On 2015-10-14 18:53:14 +0300, Shay Rojansky wrote:
>> However, the new situation where some versions of PG allow this parameter
>> while others bomb when seeing it. Specifically, Npgsql sends
>> ssl_renegotiation_limit=0 in the startup packet to completely disable
>> renegotiation. At this early stage it doesn't know yet whether the database
>> it's connecting to is PG 9.5 or earlier.
> I find it a rather debatable practice to send such a parameter
> unconditionally. Why are you sending it before the connection has even
> been established?
It doesn't seem to me that a connector such as npgsql has any business
whatsoever fooling with such a parameter, unconditionally or otherwise.
regards, tom lane
On 2015-10-14 13:04:30 -0400, Tom Lane wrote: > It doesn't seem to me that a connector such as npgsql has any business > whatsoever fooling with such a parameter, unconditionally or otherwise. I think in npgsql simply doesn't support renegotiation (IIRC because it'd have been hard to implement in .net). Which makes it somewhat reasonable to set it to 0.
Andres Freund wrote: > On 2015-10-14 13:04:30 -0400, Tom Lane wrote: > > It doesn't seem to me that a connector such as npgsql has any business > > whatsoever fooling with such a parameter, unconditionally or otherwise. > > I think in npgsql simply doesn't support renegotiation (IIRC because > it'd have been hard to implement in .net). Which makes it somewhat > reasonable to set it to 0. I think we could continue to have the parameter except that it throws an error if you try to set it to something other than 0. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2015-10-14 14:19:40 -0300, Alvaro Herrera wrote: > I think we could continue to have the parameter except that it throws an > error if you try to set it to something other than 0. That'll make it hard to ever remove it tho. Not sure if it's worth doing so for a dubious use of the variable. Andres
Andres Freund wrote: > On 2015-10-14 14:19:40 -0300, Alvaro Herrera wrote: > > I think we could continue to have the parameter except that it throws an > > error if you try to set it to something other than 0. > > That'll make it hard to ever remove it tho. Well, we just have to wait until 9.4 is out of support (so by the time we're releasing 9.9, or 9.8 if we don't get release cycles under control). As far as I recall we still have the alias entry for sort_mem in map_old_guc_names, and what grief does it cause? > Not sure if it's worth doing so for a dubious use of the variable. What would you recommend then? Forcing the user to specify the version before the connection is established is not nice. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Andres Freund wrote:
>> On 2015-10-14 14:19:40 -0300, Alvaro Herrera wrote:
>>> I think we could continue to have the parameter except that it throws an
>>> error if you try to set it to something other than 0.
>> That'll make it hard to ever remove it tho.
> What would you recommend then? Forcing the user to specify the version
> before the connection is established is not nice.
Yeah. I thought about telling Shay to set the variable after establishing
the connection, but there's a problem with that: if the user issues RESET
ALL then his setting would go away. (IIRC, settings established in the
connection packet are considered to be what to reset to; but a SET sent
just after connection would not be.)
The only other alternative is to make a second connection attempt if the
first fails, which is pretty messy.
If we think it's legit for npgsql to try to force renegotiation off,
then we have to give pretty serious consideration to putting the variable
back as Alvaro suggests.
regards, tom lane
Just to give some added reasoning...
As Andres suggested, Npgsql sends ssl_renegotiation_limit=0 because we've seen renegotiation bugs with the standard .NET SSL implementation (which Npgsql uses). Seems like everyone has a difficult time with renegotiation.
As Tom suggested, it gets sent in the startup packet so that DISCARD/RESET ALL resets back to 0 and not to the default 512MB (in older versions). Npgsql itself issues DISCARD ALL in its connection pool implementation to reset the connection to its original opened state, and of course users may want to do it themselves. Having SSL renegotiation accidentally turned on because a user sent RESET ALL, when the SSL implementation is known to have issues, is something to be avoided...
Shay
Shay Rojansky wrote: > Just to give some added reasoning... > > As Andres suggested, Npgsql sends ssl_renegotiation_limit=0 because we've seen renegotiation bugs with > the standard .NET SSL implementation (which Npgsql uses). Seems like everyone has a difficult time > with renegotiation. As far as I remember, that was introduced because of renegotiation bugs with Mono: http://lists.pgfoundry.org/pipermail/npgsql-devel/2010-February/001074.html http://fxjr.blogspot.co.at/2010/03/ssl-renegotiation-patch.html Of course, with renegotiation disabled, nobody knows whether there would also have been renegotiation problems since Npgsql switched from Mono SSL to .NET SSL ... > As Tom suggested, it gets sent in the startup packet so that DISCARD/RESET ALL resets back to 0 and > not to the default 512MB (in older versions). Npgsql itself issues DISCARD ALL in its connection pool > implementation to reset the connection to its original opened state, and of course users may want to > do it themselves. Having SSL renegotiation accidentally turned on because a user sent RESET ALL, when > the SSL implementation is known to have issues, is something to be avoided... Maybe Npgsql could switch to sending the statement after the connection has been established and resending it after each RESET ALL? You could add documentation that people should not use RESET ALL with Npgsql unless they are ready to disable renegotiation afterwards. If not, the only solution I can see is for PostgreSQL to not protest if it sees the parameter in the startup packet. Yours, Laurenz Albe
As far as I remember, that was introduced because of renegotiation bugs with Mono:
http://lists.pgfoundry.org/pipermail/npgsql-devel/2010-February/001074.html
http://fxjr.blogspot.co.at/2010/03/ssl-renegotiation-patch.html
Of course, with renegotiation disabled, nobody knows whether there would also have been renegotiation
problems since Npgsql switched from Mono SSL to .NET SSL ...
You may be right (this was before my time on Npgsql). But since PostgreSQL is dropping negotiation on its side it doesn't really make sense to dive into this and find out if it's still buggy or not...
Maybe Npgsql could switch to sending the statement after the connection has been
established and resending it after each RESET ALL?
You could add documentation that people should not use RESET ALL with Npgsql unless they
are ready to disable renegotiation afterwards.
That's certainly possible, although it seems problematic to prohibit RESET ALL because of an issue like this. It's a useful feature that users may want to use as they manipulate parameters, that's why PostgreSQL has it in the first place...
I also prefer not to rely on documentation warnings which people may miss, especially in this case where the consequences of accidentally turning on renegotiation with a buggy stack would be extremely difficult to track and debug...
If not, the only solution I can see is for PostgreSQL to not protest if it sees the
parameter in the startup packet.
Yeah, that's the ideal solution here as far as I'm concerned.
On 2015-10-16 16:41:16 +0300, Shay Rojansky wrote: > > If not, the only solution I can see is for PostgreSQL to not protest if it > > sees the > > parameter in the startup packet. > > > > Yeah, that's the ideal solution here as far as I'm concerned. Well, it seems that's where we're ending up then. Could you prepare a patch? Regards, Andres
> > If not, the only solution I can see is for PostgreSQL to not protest if it
> > sees the
> > parameter in the startup packet.
> >
>
> Yeah, that's the ideal solution here as far as I'm concerned.
Well, it seems that's where we're ending up then. Could you prepare a
patch?
Yes, will do so in the coming days, thanks!
On 16 October 2015 at 14:41, Shay Rojansky <roji@roji.org> wrote:
It seems reasonable for us to have behaviour that varies according to the driver software that is trying to connect.
If not, the only solution I can see is for PostgreSQL to not protest if it sees the
parameter in the startup packet.Yeah, that's the ideal solution here as far as I'm concerned.
Agreed, but I don't like the idea of hardcoding something so horribly specific into the server.
I'd rather the driver added "driver=npgsql" as an additional parameter in the StartupMessage. We can then make the server run some driver specific logic, rather than hardcoding something that could cause breakage elsewhere. This mechanism would then be extensible to all drivers.
The StartupMessage already allows a variable number of parameters, so we don't need to change the protocol. We can backpatch a simple fix to all supported versions.
--
The StartupMessage already allows a variable number of parameters, so we don't need to change the protocol. We can backpatch a simple fix to all supported versions.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2015-10-17 12:49:17 +0100, Simon Riggs wrote: > Agreed, but I don't like the idea of hardcoding something so horribly > specific into the server. What's that specific about accepting the value for 'disabled' for a feature that's not supported anymore? > I'd rather the driver added "driver=npgsql" as an additional parameter in > the StartupMessage. We can then make the server run some driver specific > logic, rather than hardcoding something that could cause breakage > elsewhere. This mechanism would then be extensible to all drivers. How could this cause breakage alsewhere? Having to backpatch a new parameter to all supported versions seems far more invasive than adding a guc that can only be set to one value. Andres
On 17 October 2015 at 13:27, Andres Freund <andres@anarazel.de> wrote:
--
On 2015-10-17 12:49:17 +0100, Simon Riggs wrote:
> Agreed, but I don't like the idea of hardcoding something so horribly
> specific into the server.
What's that specific about accepting the value for 'disabled' for a
feature that's not supported anymore?
Because we don't do that in any other non-supported feature.
Do I really need to explain why a specific, hardcoded kluge is a bad idea?
> I'd rather the driver added "driver=npgsql" as an additional parameter in
> the StartupMessage. We can then make the server run some driver specific
> logic, rather than hardcoding something that could cause breakage
> elsewhere. This mechanism would then be extensible to all drivers.
How could this cause breakage alsewhere?
Because we are adding code for use by one specific driver, but doing nothing to ensure it runs only for that driver. We'll forget why we did this and it could cause breakage elsewhere.
Having to backpatch a new parameter to all supported versions seems far
more invasive than adding a guc that can only be set to one value.
I doubt it, since as I pointed out the protocol already supports it. The suggested method is principled and extensible.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andres Freund <andres@anarazel.de> writes:
> Having to backpatch a new parameter to all supported versions seems far
> more invasive than adding a guc that can only be set to one value.
Indeed. It is completely stupid to do this in any other way except
by reinstating ssl_renegotiation_limit as an ordinary GUC variable
whose min and max are both zero.
Quite aside from the implementation effort of inventing some
single-purpose kluge to do it another way, that solution would also
cover the complaints we're doubtless gonna get that "SET
ssl_renegotiation_limit = 0" doesn't work anymore.
regards, tom lane
On 17 October 2015 at 14:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
--
Andres Freund <andres@anarazel.de> writes:
> Having to backpatch a new parameter to all supported versions seems far
> more invasive than adding a guc that can only be set to one value.
Indeed. It is completely stupid to do this in any other way except
by reinstating ssl_renegotiation_limit as an ordinary GUC variable
whose min and max are both zero.
Agreed, my suggestion requires we can set that GUC, but we can set not-in-file also.
Quite aside from the implementation effort of inventing some
single-purpose kluge to do it another way, that solution would also
cover the complaints we're doubtless gonna get that "SET
ssl_renegotiation_limit = 0" doesn't work anymore.
Agreed, single purpose kluge is a bad thing.
Rough patch for the extensible, backpatchable, non-invasive proposal attached.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On October 17, 2015 4:18:50 PM GMT+02:00, Simon Riggs <simon@2ndQuadrant.com> wrote: >On 17 October 2015 at 14:39, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Andres Freund <andres@anarazel.de> writes: >> > Having to backpatch a new parameter to all supported versions seems >far >> > more invasive than adding a guc that can only be set to one value. >> >> Indeed. It is completely stupid to do this in any other way except >> by reinstating ssl_renegotiation_limit as an ordinary GUC variable >> whose min and max are both zero. >> > >Agreed, my suggestion requires we can set that GUC, but we can set >not-in-file also. > > >> Quite aside from the implementation effort of inventing some >> single-purpose kluge to do it another way, that solution would also >> cover the complaints we're doubtless gonna get that "SET >> ssl_renegotiation_limit = 0" doesn't work anymore. >> > >Agreed, single purpose kluge is a bad thing. > >Rough patch for the extensible, backpatchable, non-invasive proposal >attached. This just doesn't make any sense. This way npgsql setting that flag can't be released before a new set of backbranch releasesare in widespread use. Otherwise it'll just error out in all those, not just in 9.5 as it's now the case. It breakscompatibility with all unsupported versions of postgres because those will never learn to ignore this driver argument.Without any need. Ffs all were talking about is continuing to accept a guc in 9.5+, which had been accepted for 10+years. --- Please excuse brevity and formatting - I am writing this on my mobile phone.
On Oct 17, 2015, at 10:57 AM, Andres Freund <andres@anarazel.de> wrote: >> Rough patch for the extensible, backpatchable, non-invasive proposal >> attached. > > This just doesn't make any sense. This way npgsql setting that flag can't be released before a new set of backbranch releasesare in widespread use. Otherwise it'll just error out in all those, not just in 9.5 as it's now the case. It breakscompatibility with all unsupported versions of postgres because those will never learn to ignore this driver argument.Without any need. Quite so. Simon's proposal would leave a swath of devastation of exactly the type we're trying to fix, but on an epic scale. ...Robert
Here's a patch that adds back the GUC, with default/min/max 0 and GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE.
This is my first pg patch, please be gentle with any screwups :)
Attachment
On Sun, Oct 18, 2015 at 4:56 PM, Shay Rojansky <roji@roji.org> wrote: > Here's a patch that adds back the GUC, with default/min/max 0 and > GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE. > > This is my first pg patch, please be gentle with any screwups :) Why, you dummy. No, actually, this looks fine. I've committed it and back-patched it to 9.5. I took the liberty of making some cosmetic changes, however. Thanks for preparing this patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Why, you dummy.> Here's a patch that adds back the GUC, with default/min/max 0 and
> GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE.
>
> This is my first pg patch, please be gentle with any screwups :)
No, actually, this looks fine. I've committed it and back-patched it
to 9.5. I took the liberty of making some cosmetic changes, however.
Thanks for preparing this patch.
Thanks for accepting it! It will definitely save some trouble.