Re: Recent vendor SSL renegotiation patches break PostgreSQL

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.


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/