Re: [PATCH] Support bump-ssl-server-first and mimic SSL server certificates

On 18/07/2012 11:47 p.m., Tsantilas Christos wrote:
> I am working on merging bump-ssl-server-first to to trunk.
> Please my comments bellow to be sure that all are OK, before merging..
>
> First of all I had forgot to mention that in the latests patches I post
> here (t10 and t11 patches), a new log formating code has added the
> ssl::bump_mode which logs the SslBump decisions:it prints "none",
> "client-first", "server-first" or "-" for no ssl-bump enabled ports.
> It is a small feature implemented after the initial patch posted in
> squid-dev.

Ack. I saw that and have no issue.

> On 07/17/2012 04:57 AM, Amos Jeffries wrote:
>> On 17.07.2012 04:15, Tsantilas Christos wrote:
>>> This is one more patch for bump-ssl-server-first feature.
>>> This is handle most of Amos comments and allow use old ssl_bump syntax:
>>> ssl_bump allow/deny acl ...
>>>
>>> This patch try to implement the following rules:
>>> 1. Convert allow to client-first, with a deprecation warning. One
>>> such warning per config.
>>> 2. Convert deny to none, with a deprecation warning. One such warning
>>> per config.
> I must note here that currently the patch I post only one message
> printed on squid start-up or reconfigure, a "deny to none" warning or a
> "allow to client-first" warning message. I am thinking now that we may
> need a "deny to none" AND an "allow to client-first" .

Er, yes. Sorry I overlooked that.

We have warning on every line occurance for other directives. Long term
I think it does not matter, and personally think the extra annoyance
will help prompt admin to update the config all the faster.

>>> 3. If there was a conversion, make the implicit negation rule
>>> explicit by adding either "none all" or "client-first all" as
>>> appropriate. Emit a warning specifying which rule has been added. This
>>> will need to be done after the entire configuration has been parsed, of
>>> course. It uses the rrFinalizeConfig Runner.
>>> 4. Issue a fatal error if a mixture of old and new keywords is found.
>>>
>>>
>>> I am attaching two patches here. The first is the changes over the
>>> original bump-ssl-server-first patch, which requested by Amos. And the
>>> second is the final patch.
>>>
>>> Regards,
>>> Christos
>>
>> Thank you.
>>
>> Are the new WARNING really that important that they always be at
>> CRITICAL level?
>> I would think the impact was a bit less (no impact in the case of deny)
>> s/DBG_CRITICAL/DBG_PARSE_NOTE(2)/ for the deny conversion?
>> s/DBG_CRITICAL/DBG_PARSE_NOTE(DBG_IMPORTANT)/ for the allow conversion?
>>
>> ... using the DBG_PARSE_NOTE(x) macro which is now available in trunk,
>> it bumps the message up to CRITICAL when -k parse is used, but outputs
>> at the indicated value during normal startup/reconfigure operations.
> OK.
>
>>
>> Also, the "allow" message would seem to qualify for the "SECURITY
>> NOTICE:" prefix instead of just "WARNING:".
>> http://wiki.squid-cache.org/SquidFaq/SquidLogs#Squid_Error_Messages
> Do you mean the messages inside sslBumpCfgRr::run method or the "allow
> to client-first conversion" warning message?

Both. The risk is in client-first not being the optimal security choice,
so anything auto-converting to it is a serious event.

Amos
Received on Wed Jul 18 2012 - 12:48:54 MDT