Re: Question ICAP-client

Hi Alex,

Alex Rousskov wrote:
> On Wed, 2007-03-07 at 23:57 +0200, Tsantilas Christos wrote:
>
>> When an http request adapted using ICAP then the client and server
>> addresses and the authentication information does not copied to adapted
>> request.
>> This is will cause problems in any following access control lists
>> proccessing.
>
> Should this copying be configurable? In some environments, an adapted
> request is not a request "authorized" by the user. Can we always trust
> the ICAP server to essentially impersonate the user?

I think that client address/port and squid address/port must copied.
They can not (and must not) changed by an ICAP server.
The same about authentication information because referred to users
authenticated on squid and this info must not changed by an ICAP server.
Not coping addresses and auth info, we are breaking the ACL checking in
later steps (e.g in FwdState::fwdStart).

Also I think, there is not any problem with HttpRequest.flags. A small
research about the flags show me that only the flags.redirected,
flags.accelerated, flags.transparent, flags.internal,
flags.internalclient are computed before the ICAP reqmod called. I do
not think that any of this flags can modified by an ICAP
server. The other flags set after the ICAP reqmod. (I kept a list with
flags and section code they set....)

>
> I have no problems with committing a fix that always copies this
> information as a temporary measure, but I think we need to at least put
> some comments in the code that we are copying user-specific information
> to a request that did not originate from the user.
>
I think it not so problem, the new request can appeared as originated by
the same user and the same IP as the original request...

> In the ICAP 204 case, we should probably always copy (and eventually we
> will be able simply reuse the same request object). For ICAP 200, we
> should be more careful and at least put a TODO comment. If we want to
> distinguish these two cases, then ICAPModXact should do the copying and
> not client_side.
>
> If you want to move the copying code to ICAPModXact, you can call it
> from handle204NoContent() unconditionally and perhaps from
> parseHttpHead() with a "TODO: make this copying configurable" comment.
>
Yep, the copying code must placed in ICAPModXact.....
I am including a new patch. If you think it can be applied please add
any comment or do any change you think needed....

Index: ICAPModXact.cc
===================================================================
RCS file: /cvsroot/squid/squid3/src/ICAP/ICAPModXact.cc,v
retrieving revision 1.1.2.22
diff -u -r1.1.2.22 ICAPModXact.cc
--- ICAPModXact.cc 20 Feb 2007 16:22:30 -0000 1.1.2.22
+++ ICAPModXact.cc 10 Mar 2007 11:52:08 -0000
@@ -551,7 +551,6 @@
 {
     if (adapted.header) // already allocated
         return;
-
     if (gotEncapsulated("res-hdr")) {
         adapted.setHeader(new HttpReply);
     } else if (gotEncapsulated("req-hdr")) {
@@ -701,6 +700,14 @@
     if (const HttpRequest *oldR = dynamic_cast<const
HttpRequest*>(oldHead)) {
         HttpRequest *newR = new HttpRequest;
         newR->client_addr = oldR->client_addr;
+ newR->client_port = oldR->client_port;
+ newR->my_addr = oldR->my_addr;
+ newR->my_port = oldR->my_port;
+ newR->flags = oldR->flags;
+ if (oldR->auth_user_request) {
+ newR->auth_user_request = oldR->auth_user_request;
+ newR->auth_user_request->lock();
+ }
         newHead = newR;
     } else
     if (dynamic_cast<const HttpReply*>(oldHead))
@@ -758,6 +765,21 @@

         if (!parseHead(adapted.header))
             return; // need more header data
+
+ if (HttpRequest *newHead =
dynamic_cast<HttpRequest*>(adapted.header)) {
+ const HttpRequest *oldR = dynamic_cast<const
HttpRequest*>(virgin.header);
+ Must(oldR);
+ newHead->client_addr = oldR->client_addr;
+ newHead->client_port = oldR->client_port;
+ newHead->my_addr = oldR->my_addr;
+ newHead->my_port = oldR->my_port;
+ newHead->flags = oldR->flags;
+ if (oldR->auth_user_request) {
+ newHead->auth_user_request = oldR->auth_user_request;
+ newHead->auth_user_request->lock();
+ }
+ }
+
     }

     decideOnParsingBody();
Received on Sat Mar 10 2007 - 06:59:35 MST