On 9/12/2011 12:44 p.m., Alex Rousskov wrote:
On 12/08/2011 02:34 PM, Amos Jeffries wrote:
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.
FWIW, we are fiddling with sslHostName-related code as a part of the
bump-server-first project. I even tried passing the request itself
(instead of request->GetHost) but that did not work well. Our current
code reads:
void
ConnStateData::switchToHttps(const char *host, const int port)
{
assert(!switchedToHttps_);
sslHostName = host;
so both host and port are now available where sslHostName is set (either
from CONNECT or from intercepted connections). We can change sslHostName
to something that holds both the host name and the port OR we can
convert available information to full URI before calling switchToHttps().
:) I was looking at it today and reached the same conclusion. I added a
sslBumpedPort field to ConnStateData to hold the port, but did not quite
get around to the bits you have done to get the port there where the
member needs to get set.
Essentially we keep the authority-URL (only) but as two pieces instead
of one. The URL refactoring/cleanup work I have ticking away in the
background can fix that up again later in a nice way.
PS. Keep a record of the problems about that request passing. The bug
about doing re-CONNECT will likely hit the same issues again, since it
ideally sends the original request headers out again. Can be avoided if
it was very problematic, but forwarding things like unknown headers and
U-A details would be good in that bugs fix.
Amos
