Re: [MERGE] Connection pinning patch

[Tsantilas Christos]

Alex Rousskov wrote:

On Mon, 2008-09-22 at 00:07 +0800, Adrian Chadd wrote:

Its a 900-odd line patch; granted, a lot of it is boiler plate for
config parsing and management, but I recall the issues connection
pinning had when it was introduced and I'd hate to try and be the one
debugging whatever crazy stuff pops up in 3.1 combined with the
changes to the workflow connection pinning introduces.


It would help if there was a document describing what connection pinning
is and what are the known pitfalls. Do we have such a document? Is RFC
4559 enough?
I think RFC4559 is enough and it is small enough if someone want a quick 
view. The only does not document well in this document is what will 
happens if more than one proxy servers used (Connection: Proxy-Support 
header)




If not, Christos, can you write one and have Adrian and others
contribute pitfalls? It does not have to be long -- just a few
paragraphs describing the basics of the feature. We can add that
description to code documentation too.


The problem with negotiate authentication mechanism (NTLM and Kerberos) 
is that it does not authenticate the client request to the server but 
each client connection to the server separately. At the beggining of 
each connection (I mean low level connection, a socket connection) to 
the server the following conversation required:


1st Step
=
client to server: GET dir/index.html

Server to client response:
 HTTP/1.1 401 Unauthorized
 WWW-Authenticate: Negotiate

Here the server saids that authentication required.


2nd Step
=
client to server:
 GET dir/index.html
 Authorization: Negotiate a87421000492aa874209af8bc028

server to client:
  HTTP/1.1 401 Unauthorized
  WWW-Authenticate: Negotiate 749efa7b23409c20b92356

Client send required authentication data and server respond with its 
authentication data



3d Step
===
 Client to server:
  GET dir/index.html
  Authorization: Negotiate 89a8742aa8729a8b028

Server to Client response:
  HTTP/1.1 200 OK
  WWW-Authenticate: Negotiate ade0234568a4209af8bc0280289eca
  Normal response..

Client produces the final authentication data and send new request to 
the server which if the credentials are OK respond with 200 OK



Now when a proxy server exist between client and server the SAME 
connections pair, lets say the client-to-proxy and proxy-to server 
connections pair,  MUST used for any request tunneled through the 
client-to-proxy connection, else the authentication will fail.
Also the proxy must send the Proxy-support: 
Session-Based-Authentication header with its responses to the client so 
the client know that the proxy supports session based authentication 
(connection pinning).


So the connection pinning project try to correlate the client-to-proxy 
and proxy-to-server connections and use always the same pairs for client 
requests to the web server. (and it is difficult to debug it because 
after a successful test still you are not sure if the pinning code 
worked or you was jsut lucky and the same pairs used :-(  ...)


Some extra notes about Proxy-support header. This header is hop-by-hop 
header, so if there are more than one proxy servers in the chain must 
removed before the http response forwarded to the next hop (and created 
again if the  proxy server supports negotiate authentication). This is 
why the Connection: Proxy-support header required (if the 
Proxy-support string included in the list values of the Connection 
header will remove any header named Proxy-support from response 
headers). This is proposed by Henrik to the authors of the RFC when the 
rfc4559 was still draft but it is not included in the final RFC document.






I don't pretend to completely understand the implications for ICAP
either. Is there any documentation for how connection pinning should
behave with ICAP and friends?


I think there is not any need for special handling for ICAP unless we 
are planning to support Negotiate authentication on ICAP servers. In 
this case we maybe should extend connection pinning to the ICAP server 
connections too. But I am not seeing any requirement at this time.




ICAP and eCAP do not care about HTTP connections or custom headers. Is
connection pinning more than connection management via some custom
headers?
Yes it is something more but I think ICAP should not care about 
connection pinning...





Is there any particular rush to get this in for this release at such a
late point in the release cycle?


Sine NTLM authentication forwarding appears to be a required feature for
many and since connection pinning patch is not trivial (but is not huge
either), I would rather see it added now (after the proper review
process, of course). It could be the right icing on 3.1 cake for many
users. I do realize that, like any 900-line patch, it may cause problems
even if it is reviewed and tested.



I hope I am not confusing the squid developers and I explained a little 
the 

Re: [MERGE] Connection pinning patch

[Alex Rousskov]

On Sat, 2008-09-20 at 20:48 +0300, Tsantilas Christos wrote:

 This patch fixes the bug 1632 
 (http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
 It is based on the original squid2.5 connection pinning patch developed 
 by Henrik (http://devel.squid-cache.org/projects.html#pinning) and the 
 related squid 2.6 connection pinning code.

Hi Christos,

Thank you for attacking this problem. It looks like you got the
basics of the solution done, but we need to polish a few things.

*B1* Client side cleanup

 +if (pinning.fd = 0)
 +comm_close(pinning.fd);

Please move cleanup to swanSong. A job destructor is not a good place to
cleanup things because in the destructor 

- you should not use virtual functions (which may be needed during
cleanup and logging)
- you must not throw (which may happen when you do complicated cleanup)

More work is needed to move old code where it belongs, but the new code
should go into the right place.


*B1* You already know about s/debug/debugs/ changes from Amos :-)


*B2* Asymmetric unlock  in HttpRequest:

 + if(pinned_connection)
 + cbdataReferenceDone(pinned_connection);

HttpRequest never locks the pointer, yet it unlocks it during cleanup.
This worries me. Can you move the locking inside HttpRequest, perhaps by
adding a pinned_connection getter and setter methods?


*B3* Excessive cleanup:

 +if(pinned_connection)
 +   cbdataReferenceDone(pinned_connection);
 +pinned_connection = NULL;

The last line is extra because cbdataReferenceDone sets the pointer to
NULL. Same here:

 +if (conn-pinning.peer) {
 +   cbdataReferenceDone(conn-pinning.peer);
 +   conn-pinning.peer = NULL;
 +}

And elsewhere (please grep for more):

 +cbdataReferenceDone(pinning.peer);
 +pinning.peer = NULL;


*B1* Docs missing:

 +++ src/HttpRequest.h   2008-06-23 18:02:37 +
 @@ -148,6 +148,8 @@
  
  String extacl_log; /* String to be used for access.log purposes 
 */
  
 +ConnStateData *pinned_connection;

Please document the pinned_connection member. Requests are used in many
contexts, on many sides... Which connection do we point to?


*B3* inheritVirginReplyProperties is getting out of hand. There is
already a TODO item to move it to HttpRequest. Now, you have created
another one for HttpReply. Both should be moved, with a pure virtual
method added to HttpMsg. However, do not do this for this project if you
do not feel comfortable.


*B2* Please remove negation from the new member name:

 +int no_connection_auth; /* Don't support connection oriented auth */

Call it conn_auth_prohibited or _disabled if you want to keep the same
semantics. With a negative name, I find it difficult to quickly
interpret the code below, and I am sure I am not alone:

 +if (!request-flags.no_connection_auth) { // XXX: no no authentication?


*B2*  Inconsistent and negative configuration option:

 +   no-connection-auth
 +Prevent forwarding of Microsoft connection oriented
 +authentication (NTLM, Negotiate and Kerberos)

Can this be changed to connection-auth=[off|on|no|yes] or similar? We
can support the old style for Squid2 compatibility sake, but it is
better to avoid negative name as the only option. This is especially
annoying since we already have the right stuff for cache_peer:

 + connection-auth[=on|off|auto]


*B2* no_connection_auth should have bool type, right?


*B2* Please use methods as comm callbacks handlers:

 
 +/* This is a handler normally called by comm_close() */
 +static void
 +clientPinnedConnectionClosed(int fd, void *data)
...
 +comm_add_close_handler(pinning_fd, clientPinnedConnectionClosed, this);

This will improve debugging and overall code appearance.


*B1*  The ConnStateData::getPinnedInfo seems to have serious
side-effects like closing file descriptors and removing close handlers.
This is not a simple constant getter! Please rename and mention that the
call may close the descriptor in the method description.


 /**
 + * If the current ConnStateData has pinned connection returns the socket 
 descriptor of 
 + * pinned connection and the peer object if exists
 +*/
 +int ConnStateData::getPinnedInfo(const HttpRequest * request, struct peer * 
 peer){
...
 +if (request  pinning.port != request-port){
 +   comm_close(pinning.fd);
 +   return -1;
 +}

*B1* The peer output parameter should be set to NULL when
ConnStateData::getPinnedInfo fails. You can do it at the start of the
method.


*B1* Scary pointer copying. Can, for example, *peer go away during
reconfigure and such? Please ask Henrik if you are not sure whether this
is safe:

 +/*
 +  Maybe the peer should be a cbdataReference of pinning.peer (and the 
 caller 
 +  use cbdataReferenceDone).
 +  Now the peer is a single pointer...
 + */
 +peer = pinning.peer;


*B2* Something potentially wrong with descriptor management here:

 +  

Re: [MERGE] Connection pinning patch

[Tsantilas Christos]

Alex Rousskov wrote:

On Sat, 2008-09-20 at 20:48 +0300, Tsantilas Christos wrote:

This patch fixes the bug 1632 
(http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
It is based on the original squid2.5 connection pinning patch developed 
by Henrik (http://devel.squid-cache.org/projects.html#pinning) and the 
related squid 2.6 connection pinning code.


Hi Christos,

Thank you for attacking this problem. It looks like you got the
basics of the solution done, but we need to polish a few things.

*B1* Client side cleanup


+if (pinning.fd = 0)
+comm_close(pinning.fd);


Please move cleanup to swanSong. A job destructor is not a good place to
cleanup things because in the destructor 


OK.


- you should not use virtual functions (which may be needed during
cleanup and logging)
- you must not throw (which may happen when you do complicated cleanup)

More work is needed to move old code where it belongs, but the new code
should go into the right place.


*B1* You already know about s/debug/debugs/ changes from Amos :-)


OK



*B2* Asymmetric unlock  in HttpRequest:


+ if(pinned_connection)
+ cbdataReferenceDone(pinned_connection);


HttpRequest never locks the pointer, yet it unlocks it during cleanup.
This worries me. Can you move the locking inside HttpRequest, perhaps by
adding a pinned_connection getter and setter methods?


Ok I will add the setter and getter methods.




*B3* Excessive cleanup:


+if(pinned_connection)
+   cbdataReferenceDone(pinned_connection);
+pinned_connection = NULL;


The last line is extra because cbdataReferenceDone sets the pointer to
NULL. Same here:


+if (conn-pinning.peer) {
+   cbdataReferenceDone(conn-pinning.peer);
+   conn-pinning.peer = NULL;
+}


And elsewhere (please grep for more):


+cbdataReferenceDone(pinning.peer);
+pinning.peer = NULL;



OK



*B1* Docs missing:


+++ src/HttpRequest.h   2008-06-23 18:02:37 +
@@ -148,6 +148,8 @@
 
 String extacl_log; /* String to be used for access.log purposes */
 
+ConnStateData *pinned_connection;


Please document the pinned_connection member. Requests are used in many
contexts, on many sides... Which connection do we point to?


OK


*B3* inheritVirginReplyProperties is getting out of hand. There is
already a TODO item to move it to HttpRequest. Now, you have created
another one for HttpReply. Both should be moved, with a pure virtual
method added to HttpMsg. However, do not do this for this project if you
do not feel comfortable.


No problem, a virtual method inheritProperties can implemented


*B2* Please remove negation from the new member name:


+int no_connection_auth; /* Don't support connection oriented auth */


Call it conn_auth_prohibited or _disabled if you want to keep the same
semantics. With a negative name, I find it difficult to quickly
interpret the code below, and I am sure I am not alone:


+if (!request-flags.no_connection_auth) { // XXX: no no authentication?



I will keep the connection_auth_disabled if there is not any problem


*B2*  Inconsistent and negative configuration option:


+   no-connection-auth
+Prevent forwarding of Microsoft connection oriented
+authentication (NTLM, Negotiate and Kerberos)


Can this be changed to connection-auth=[off|on|no|yes] or similar? We
can support the old style for Squid2 compatibility sake, but it is
better to avoid negative name as the only option. This is especially
annoying since we already have the right stuff for cache_peer:


+ connection-auth[=on|off|auto]


No problem




*B2* no_connection_auth should have bool type, right?

Yes




*B2* Please use methods as comm callbacks handlers:


+/* This is a handler normally called by comm_close() */
+static void
+clientPinnedConnectionClosed(int fd, void *data)

...

+comm_add_close_handler(pinning_fd, clientPinnedConnectionClosed, this);


This will improve debugging and overall code appearance.


OK




*B1*  The ConnStateData::getPinnedInfo seems to have serious
side-effects like closing file descriptors and removing close handlers.
This is not a simple constant getter! Please rename and mention that the
call may close the descriptor in the method description.

Maybe it is better to split in two methods one method which check pinned 
info state and if it is required close the invalid pinning connection 
and an other one which simply returns the pinned info





/**
+ * If the current ConnStateData has pinned connection returns the socket descriptor of 
+ * pinned connection and the peer object if exists

+*/
+int ConnStateData::getPinnedInfo(const HttpRequest * request, struct peer * 
peer){

...

+if (request  pinning.port != request-port){
+   comm_close(pinning.fd);
+   return -1;
+}


*B1* The peer output parameter should be set to NULL when
ConnStateData::getPinnedInfo fails. You can do it at the start of 

Re: [MERGE] Connection pinning patch

[Amos Jeffries]

 On Sat, 2008-09-20 at 20:48 +0300, Tsantilas Christos wrote:

 This patch fixes the bug 1632
 (http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
 It is based on the original squid2.5 connection pinning patch developed
 by Henrik (http://devel.squid-cache.org/projects.html#pinning) and the
 related squid 2.6 connection pinning code.

 Hi Christos,

 Thank you for attacking this problem. It looks like you got the
 basics of the solution done, but we need to polish a few things.

snip

 *B2* Please remove negation from the new member name:

 +int no_connection_auth; /* Don't support connection oriented
 auth */

 Call it conn_auth_prohibited or _disabled if you want to keep the same
 semantics. With a negative name, I find it difficult to quickly
 interpret the code below, and I am sure I am not alone:

 +if (!request-flags.no_connection_auth) { // XXX: no no
 authentication?


 *B2*  Inconsistent and negative configuration option:

 +   no-connection-auth
 +Prevent forwarding of Microsoft connection
 oriented
 +authentication (NTLM, Negotiate and Kerberos)

 Can this be changed to connection-auth=[off|on|no|yes] or similar? We
 can support the old style for Squid2 compatibility sake, but it is
 better to avoid negative name as the only option. This is especially
 annoying since we already have the right stuff for cache_peer:

 + connection-auth[=on|off|auto]

 *B2* no_connection_auth should have bool type, right?


Christos and I have already covered this issue.
no-connection-auth is a bool http_port option for incomings.
connection-auth (with auto) is a tri-state cache_peer option for outgoings.


 *B2* Please use methods as comm callbacks handlers:


 +/* This is a handler normally called by comm_close() */
 +static void
 +clientPinnedConnectionClosed(int fd, void *data)
 ...
 +comm_add_close_handler(pinning_fd, clientPinnedConnectionClosed,
 this);

 This will improve debugging and overall code appearance.

For my education, could you specify the best way to do this?

Amos

Re: [MERGE] Connection pinning patch

[Amos Jeffries]

 On Sat, 2008-09-20 at 20:48 +0300, Tsantilas Christos wrote:

 This patch fixes the bug 1632
 (http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
 It is based on the original squid2.5 connection pinning patch developed
 by Henrik (http://devel.squid-cache.org/projects.html#pinning) and the
 related squid 2.6 connection pinning code.

 Hi Christos,

 Thank you for attacking this problem. It looks like you got the
 basics of the solution done, but we need to polish a few things.

snip

 *B2* Please remove negation from the new member name:

 +int no_connection_auth; /* Don't support connection oriented
 auth */

 Call it conn_auth_prohibited or _disabled if you want to keep the same
 semantics. With a negative name, I find it difficult to quickly
 interpret the code below, and I am sure I am not alone:

 +if (!request-flags.no_connection_auth) { // XXX: no no
 authentication?


 *B2*  Inconsistent and negative configuration option:

 +   no-connection-auth
 +Prevent forwarding of Microsoft connection
 oriented
 +authentication (NTLM, Negotiate and Kerberos)

 Can this be changed to connection-auth=[off|on|no|yes] or similar? We
 can support the old style for Squid2 compatibility sake, but it is
 better to avoid negative name as the only option. This is especially
 annoying since we already have the right stuff for cache_peer:

 + connection-auth[=on|off|auto]


 *B2* no_connection_auth should have bool type, right?


Christos and I already covered this point.
no-connection-auth is a bool squid http_port option.
connection-auth is a tri-state cache_peer option.

Amos

Re: [MERGE] Connection pinning patch

[Alex Rousskov]

On Tue, 2008-09-23 at 01:31 +0300, Tsantilas Christos wrote:
 
  *B1*  The ConnStateData::getPinnedInfo seems to have serious
  side-effects like closing file descriptors and removing close handlers.
  This is not a simple constant getter! Please rename and mention that the
  call may close the descriptor in the method description.
  
 Maybe it is better to split in two methods one method which check pinned 
 info state and if it is required close the invalid pinning connection 
 and an other one which simply returns the pinned info

Sounds good to me. This may also help to remove some code duplication
among the two new ::getPinned* methods.

  *B2* Something potentially wrong with descriptor management here:
  
  +int pinning_fd = pinning.fd;
  +if (pinning_fd  0)
  +   return -1;
  +
  +if (pinning.auth  request  strcasecmp(pinning.host, 
  request-GetHost()) != 0) {
  +   comm_close(pinning_fd);
  +   return -1;
  +}
  
  If the if-statement condition is true, the pinning.fd is pointing is a
  closing descriptor. Is that what you want?
 
 Or just the pinning.fd is not set. I think in both cases returns the 
 correct value

The returned value is correct.

 The pinning.fd will set in clientPinnedConnectionClosed comm_close 
 handler. I think it is better to not touch it in this function

My wording was clumsy, sorry. Let me rephrase. I am worried that
pinning.fd is positive but the corresponding descriptor is closing. I am
worried that some cleanup code will look at a positive pinning.fd and
try to use it. Can that happen before clientPinnedConnectionClosed is
dialed?

I think there are two different approaches of descriptor management:

1) Invalidate the descriptor (fd) when calling comm_close. This protects
the code from using a closing descriptor.

2) Invalidate the descriptor (fd) when comm_close calls us. This avoids
the illusion that we know when the descriptor starts closing. We do not
know that, in general, because some other code can close our descriptor.

I think your code above is using approach #2 and I was kind of rooting
for approach #1. I am not sure which approach is better, but it would be
nice to be consistent.

Does anybody have an opinion on which approach should be used, in
general (other than the opinion that the whole mess should be replaced
with a better API; which is correct, but not relevant for this
project)? 

If there is one approach that should be used in most cases, I will add
it to Comm API notes.

Thank you,

Alex.

Re: [MERGE] Connection pinning patch

[Alex Rousskov]

On Tue, 2008-09-23 at 10:37 +1200, Amos Jeffries wrote:

  *B2* Please remove negation from the new member name:
 
  +int no_connection_auth; /* Don't support connection oriented
  auth */
 
  Call it conn_auth_prohibited or _disabled if you want to keep the same
  semantics. With a negative name, I find it difficult to quickly
  interpret the code below, and I am sure I am not alone:
 
  +if (!request-flags.no_connection_auth) { // XXX: no no
  authentication?
 
 
  *B2*  Inconsistent and negative configuration option:
 
  +   no-connection-auth
  +Prevent forwarding of Microsoft connection
  oriented
  +authentication (NTLM, Negotiate and Kerberos)
 
  Can this be changed to connection-auth=[off|on|no|yes] or similar? We
  can support the old style for Squid2 compatibility sake, but it is
  better to avoid negative name as the only option. This is especially
  annoying since we already have the right stuff for cache_peer:
 
  + connection-auth[=on|off|auto]
 
  *B2* no_connection_auth should have bool type, right?
 
 
 Christos and I have already covered this issue.
 no-connection-auth is a bool http_port option for incomings.
 connection-auth (with auto) is a tri-state cache_peer option for outgoings.

Yes, I understand that. There are three other issues here (and I hope
Christos got it):

- It is not a good idea to name class members using no prefixes.
- Negative configuration option names are also a poor default.
- no_connection_auth data member should have bool type.


  *B2* Please use methods as comm callbacks handlers:
 
 
  +/* This is a handler normally called by comm_close() */
  +static void
  +clientPinnedConnectionClosed(int fd, void *data)
  ...
  +comm_add_close_handler(pinning_fd, clientPinnedConnectionClosed,
  this);
 
  This will improve debugging and overall code appearance.
 
 For my education, could you specify the best way to do this?

Sure, here is one example:

AsyncCall::Pointer call = asyncCall(33, 5, ConnStateData::connStateClosed,
Dialer(connState, ConnStateData::connStateClosed));
comm_add_close_handler(newfd, call);

The first statement creates an async call with a given section,
debugging level, name, and dialer. The second statement gives that call
to Comm as a close handler callback.

You can find many more (most written by Christos) if you grep for
comm_add_close_handler and look for those calls that have :: above
them.

The call can probably be declared as const unless you want to store it
to cancel later (which is guaranteed to work unlike canceling of
old-style callbacks).

HTH,

Alex.

Re: [MERGE] Connection pinning patch

[Amos Jeffries]

Tsantilas Christos wrote:

Hi all,

This patch fixes the bug 1632 
(http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
It is based on the original squid2.5 connection pinning patch developed 
by Henrik (http://devel.squid-cache.org/projects.html#pinning) and the 
related squid 2.6 connection pinning code.


Although I spend many hours looking on pined connections I am still not 
absolutely  sure that does not have  bugs. However the code is very 
similar with this in squid2.6 (where the pinning code runs for years) 
and I hope will be easy to fix problems and bugs.


Regards,
Christos


bb:approve

I think this should be on the fast track. I'll leave a few days for 
others to comment or veto, then commit early.

Theres only some debug(33, 2) (X) lines which need to change.

Amos
--
Please use Squid 2.7.STABLE4 or 3.0.STABLE9

Re: [MERGE] Connection pinning patch

[Amos Jeffries]

Amos Jeffries has voted approve.
Status is now: Semi-approved


For details, see: 
http://bundlebuggy.aaronbentley.com/project/squid/request/%3C48D53750.8020104%40users.sourceforge.net%3E

Project: Squid

Re: [MERGE] Connection pinning patch

[Adrian Chadd]

Its a 900-odd line patch; granted, a lot of it is boiler plate for
config parsing and management, but I recall the issues connection
pinning had when it was introduced and I'd hate to try and be the one
debugging whatever crazy stuff pops up in 3.1 combined with the
changes to the workflow connection pinning introduces.

I don't pretend to completely understand the implications for ICAP
either. Is there any documentation for how connection pinning should
behave with ICAP and friends?

Is there any particular rush to get this in for this release at such a
late point in the release cycle?

Could we hold off of it until the next release, and just focus on
getting whats currently in 3.HEAD released and stable?



Adrian


2008/9/21 Tsantilas Christos [EMAIL PROTECTED]:
 Hi all,

 This patch fixes the bug 1632
 (http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
 It is based on the original squid2.5 connection pinning patch developed by
 Henrik (http://devel.squid-cache.org/projects.html#pinning) and the related
 squid 2.6 connection pinning code.

 Although I spend many hours looking on pined connections I am still not
 absolutely  sure that does not have  bugs. However the code is very similar
 with this in squid2.6 (where the pinning code runs for years) and I hope
 will be easy to fix problems and bugs.

 Regards,
Christos

Re: [MERGE] Connection pinning patch

[Tsantilas Christos]

Adrian Chadd wrote:

Its a 900-odd line patch; granted, a lot of it is boiler plate for
config parsing and management, but I recall the issues connection
pinning had when it was introduced and I'd hate to try and be the one
debugging whatever crazy stuff pops up in 3.1 combined with the
changes to the workflow connection pinning introduces.


I am saying in my mail that I am not sure that there is not any bug. I 
did not test it using production servers, only I used test web servers, 
The ntlm authentication is not widely used in the internet, it is not 
easy to find a number of ntlm authenticated sites to do a better test.
However I believe that it is in a good state and we need feedback from 
production users in order to fix any remaining bug (IF there is any)




I don't pretend to completely understand the implications for ICAP
either. Is there any documentation for how connection pinning should
behave with ICAP and friends?


There is not any special issue when ICAP used...



Is there any particular rush to get this in for this release at such a
late point in the release cycle?

Could we hold off of it until the next release, and just focus on
getting whats currently in 3.HEAD released and stable?



I believe it is an important feature. The ntlm authentication maybe is 
not used for internet sites but it is widely used for corporation 
internal sites.
Talking with other system administrators, the connection pinning and 
chunked encoding are the most important reasons for a system admin to 
not choose squid3 as  proxy server for his networks. I am system admin 
too and for the same reasons my 2-3 production proxy servers are squid2. 
I am waiting 3.1 release to upgrade at least one of them :-)


In any case if the connection pinning will not included in squid 3.1 I 
can try to maintain a connection pinning patch for squid 3.1 as 
workaround/solution for the people need this feature.


Regards,
   Christos




Adrian


2008/9/21 Tsantilas Christos [EMAIL PROTECTED]:

Hi all,

This patch fixes the bug 1632
(http://www.squid-cache.org/bugs/show_bug.cgi?id=1632)
It is based on the original squid2.5 connection pinning patch developed by
Henrik (http://devel.squid-cache.org/projects.html#pinning) and the related
squid 2.6 connection pinning code.

Although I spend many hours looking on pined connections I am still not
absolutely  sure that does not have  bugs. However the code is very similar
with this in squid2.6 (where the pinning code runs for years) and I hope
will be easy to fix problems and bugs.

Regards,
   Christos

Re: [MERGE] Connection pinning patch

[Alex Rousskov]

On Mon, 2008-09-22 at 00:07 +0800, Adrian Chadd wrote:
 Its a 900-odd line patch; granted, a lot of it is boiler plate for
 config parsing and management, but I recall the issues connection
 pinning had when it was introduced and I'd hate to try and be the one
 debugging whatever crazy stuff pops up in 3.1 combined with the
 changes to the workflow connection pinning introduces.

It would help if there was a document describing what connection pinning
is and what are the known pitfalls. Do we have such a document? Is RFC
4559 enough?

If not, Christos, can you write one and have Adrian and others
contribute pitfalls? It does not have to be long -- just a few
paragraphs describing the basics of the feature. We can add that
description to code documentation too.

 I don't pretend to completely understand the implications for ICAP
 either. Is there any documentation for how connection pinning should
 behave with ICAP and friends?

ICAP and eCAP do not care about HTTP connections or custom headers. Is
connection pinning more than connection management via some custom
headers?

 Is there any particular rush to get this in for this release at such a
 late point in the release cycle?

Sine NTLM authentication forwarding appears to be a required feature for
many and since connection pinning patch is not trivial (but is not huge
either), I would rather see it added now (after the proper review
process, of course). It could be the right icing on 3.1 cake for many
users. I do realize that, like any 900-line patch, it may cause problems
even if it is reviewed and tested.

Thank you,

Alex.

Re: [MERGE] Connection pinning patch

[Adrian Chadd]

2008/9/22 Alex Rousskov [EMAIL PROTECTED]:


 It would help if there was a document describing what connection pinning
 is and what are the known pitfalls. Do we have such a document? Is RFC
 4559 enough?

I'll take another read. I think we should look at documenting these
sorts of features somewhere else though.

 If not, Christos, can you write one and have Adrian and others
 contribute pitfalls? It does not have to be long -- just a few
 paragraphs describing the basics of the feature. We can add that
 description to code documentation too.

I'd be happy to help troll over the 2.X code and see what its doing.
Henrik and Steven know the code better than I do; I've just spent some
time figuring out how it interplays with load balancing to peers and
such.

 ICAP and eCAP do not care about HTTP connections or custom headers. Is
 connection pinning more than connection management via some custom
 headers?

Nope; it just changes the semantics a little and some code may assume
things work a certain way.

 Sine NTLM authentication forwarding appears to be a required feature for
 many and since connection pinning patch is not trivial (but is not huge
 either), I would rather see it added now (after the proper review
 process, of course). It could be the right icing on 3.1 cake for many
 users. I do realize that, like any 900-line patch, it may cause problems
 even if it is reviewed and tested.

*nodnod* I'm just making sure the reasons for pushing it through are
recorded somewhere during the process.



Adrian

Re: [MERGE] Connection pinning patch

[Amos Jeffries]

 2008/9/22 Alex Rousskov [EMAIL PROTECTED]:


 It would help if there was a document describing what connection pinning
 is and what are the known pitfalls. Do we have such a document? Is RFC
 4559 enough?

 I'll take another read. I think we should look at documenting these
 sorts of features somewhere else though.

My reading of it, is that it works very similar to keep-alive. But limits
the alive connection to prevent collapsed forwarding over it or re-use by
other clients.
The impact on Squid-3 should be very minimal at this point, since we don't
yet have collapsed forwarding. This is though a pre-requisite of a good
collapsed design for Squid-3, which is high priority for 3.2.

As Christos said, its far more widely used for internal networks than
Internet. Going by the squid-users traffic it's starting to be the biggest
squid upgrade blocker at present.


 If not, Christos, can you write one and have Adrian and others
 contribute pitfalls? It does not have to be long -- just a few
 paragraphs describing the basics of the feature. We can add that
 description to code documentation too.

 I'd be happy to help troll over the 2.X code and see what its doing.
 Henrik and Steven know the code better than I do; I've just spent some
 time figuring out how it interplays with load balancing to peers and
 such.

 ICAP and eCAP do not care about HTTP connections or custom headers. Is
 connection pinning more than connection management via some custom
 headers?

 Nope; it just changes the semantics a little and some code may assume
 things work a certain way.

 Sine NTLM authentication forwarding appears to be a required feature for
 many and since connection pinning patch is not trivial (but is not huge
 either), I would rather see it added now (after the proper review
 process, of course). It could be the right icing on 3.1 cake for many
 users. I do realize that, like any 900-line patch, it may cause problems
 even if it is reviewed and tested.

 *nodnod* I'm just making sure the reasons for pushing it through are
 recorded somewhere during the process.



 Adrian