On 9/12/2011 3:59 a.m., Alex Rousskov wrote:
On 12/08/2011 06:34 AM, Amos Jeffries wrote:
On 8/12/2011 11:58 a.m., Amos Jeffries wrote:
On Wed, 07 Dec 2011 18:59:15 +0200, Tsantilas Christos wrote:
On 12/07/2011 01:19 PM, Amos Jeffries wrote:
So, in theory ssl-bump is a form of intercept not a form of
accelerator. Its use of prepareAcceleratorURL() to convert the partial
to absolute URL unconditionally sets the accel flag with a mix of side
effects. Some bad ones have been identified already.
This patch changes the flag setting, to allow ssl-bump to use the URL
preparation function without the side effects. I'm in half a mind to
make a ssl-bump specific URL preparation function, but only after this
is proven workable.
Christos: as the person who appears to have the best testing ability
for
ssl-bump can you run your tests over the resulting Squid and check that
the expected behaviours have not changed for the worse? I am fully
expecting there to be several as yet unknown places needing to add a
test of the sslBumped flag alongside testing accel flag.
Looks OK.
Also because accel and related flags are used in http authentication and
esi, I think too it is better to detach it from sslbumped requests,
where we should handle authentication with a different way...
The only objection is for the Config.onoff.ie_refresh parameter.
Look in the client_side_request.cc line 1027 inside block:
if (Config.onoff.ie_refresh) {
}
But does not look important....
Hmm, maybe. The docs around that code are incorrect "or transparent
proxy" is not one of the cases it kicks in. Only (accel&& ims).
We can either drop it or need to add all of sslbump, intercept,
tproxy, and accel flags.
Since it is so screwed I think we can leave it for now and fix
separately.
Thank you for the check. I have moved on to splitting the prepare*()
function now.
On the SSL intercept case we don't have any backup host:port that I
can see easily. If it is saved somewhere please let me know.
For now the order of hot:port locating is:
- use bumped requests Host: header, else
- use CONNECT authority-URL, (http->sslHostName contains the host:port
from the CONNECT right?)
- reject with "400 Invalid Request" (RFC 2616 mandated response on
missing Host:)
I have also added a test case to the transparent intercept case to
default port-443 received traffic through this new https:// URL
emitting function. port-80 and unknown ports through the old function
emitting http:// URLs.
Here is the mk2 patch. With better re-assembly of the absolute URL for
ssl-bump and SSL interception case as well.
The only thing I'm worried about now is that strange ports on CONNECT
request being ssl-bumped might be lost with sslHostName. The same is
assumed for intercepted SSL, so its not a huge blocker, but could be a
problem for some. Ideas?
If the changes result in Squid losing port information, it is a blocker.
I think it is OK to use 80/443 in comments (HTTP and HTTPS OR http_port
and https_port may be better though), but the actual port should be
passed to wherever it is needed. Can you clarify why passing the actual
port is problematic/difficult?
Unless I've missed something the only remnant we have of the original
CONNECT request-URL is stored in sslHostName. It is set from
request->GetHost() which is the bare hostname with port elided.
Relatively easy to send both, I just have not had time to investigate
sslHostName usage properly.
Amos
