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

Reply via email to