2010/2/24 Tom Lane <tgl@sss.pgh.pa.us>:
Magnus Hagander <magnus@hagander.net> writes:
2010/2/22 Tom Lane <tgl@sss.pgh.pa.us>:
If you want to do it, I'd be fine with it.
Seems easy enough, see attached. Comments?
Looks mostly okay to me, a few notes:

+               if (ssl_renegotiation_limit && port->count > ssl_renegotiation_limit * 1024)
You need "1024L" there to avoid risk of integer overflow when long is Check.
Also, the comment attached to ssl_renegotiation_limit needs to be fixed
to mention that zero disables renegotiation.
Check.

Also, the coding seems a bit confused about whether the
ssl_renegotiation_limit GUC exists when USE_SSL isn't set.  I think we
have a project policy about whether GUCs should still exist when the
underlying support isn't compiled, but I forget what it is :-(.
Anyway you need to test that the patch compiles with USE_SSL off.
Yeah, that's clearly wrong. The extern is there, but not the
definition. Fixed, and will test.

I personally find it highly annoying when a GUC goes away, so I'm all
for always having them there. And I thought that was our policy for
new ones, but I can't find a reference to it...

Also the xreflabel for the variable in the docs isn't consistent,
and you failed to mention the default value.
You mean add _limit to it, right?

I looked at the parameter next to it, and none of them include the
default value in the description. But I see now that's the exception,
rather than the rule. Fixed.
This version is set to superuser only. It's a security related
feature, so just letting a random user turn it off may be seen as
wrong. On the other hand, this is just about the connection security,
and if we have a malicious user on the other end, he can do a lot
worse things than disable renegotiation (such as resending the
plaintext after it's been decrypted).
I'd therefore suggest we make it USERSET. Anything wrong in that discussion?
SUSET seems less surprising to me.  I agree that it's hard to make
a concrete case for a user doing anything terribly bad with it,
but on the other hand is there much value in letting it be USERSET?
Anyway it's not very important to me either way.
The use case would be for example npgsql (or npgsql clients) being
able to disable it from the client side, because they know they can't
deal with it. Even in the case that the server doesn't know that.

Also, do we want to add a specific <note> to the documentation saying
this is the way around broken SSL libraries? Or leave that to release
notes? Or just leave it to the mailinglist archives?
I think a short note in the description of the variable wouldn't be out
of place.
Ok, will add.

Search Discussions

Discussion Posts

Previous

Follow ups

Related Discussions

People

Translate

site design / logo © 2022 Grokbase