Re: [MERGE] Connection pinning version 2

Tsantilas Christos wrote:
> Hi all,
>
> Comments/problems which resolved/fixed:
>
> Alex Rousskov wrote:
> >
> > *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
>
> Done
>
> >
> > *B1* You already know about s/debug/debugs/ changes from Amos :-)
>
> done
>
>
> > *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?
> >
>
> done
>
> >
> > *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:
> > ..........
>
> done
>
> > *B1* Docs missing:
> >
> >> +++ src/HttpRequest.h 2008-06-23 18:02:37 +0000
> >> @@ -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?
>
> done
>
>
> >
> >
> > *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.
>
> done
>
> >
> >
> > *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?
> >
> >
>
> renamed to connection_auth_disabled
> Done
>
>
> > *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]
>
> My first thought was that this parameter should be connection-auth and
> both Amos and Alex suggesting connection-auth.
> I must note that the connection-auth for peer configuration takes tree
> different values(on|off|auto) and connection-auth for http_port
> configuration only two(on|off). For this reason maybe an on/off
> parameter (like the no-connection-auth) is better for http_port.
> But I made this change. The no-connection-auth will also work for
> squid2x compatibility.
> If we decide that the old parameter was better I can change it again.
>
>
> > *B2* no_connection_auth should have bool type, right?
>
> done for http_port_list::connection_auth_disabled
>
>
> > *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.
> >
>
> done
>
>
> >
> > *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.
>
> This method removed
>
> >
> >
> >> /**
> >> + * 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;
> >
>
> pinning peer now can retrieved using the ConnStateData::pinnedPeer() method
>
> >
> > *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?
> >
> >> + auth = pinning.auth;
> >> + if (peer != pinning.peer){
> >> + comm_close(pinning_fd);
> >> + return -1;
> >> + }
> >
> > Same here. I suggest removing pinning_fd local and updating
> pinning.fd if needed.
>
> The new methods which replaced the getPinnedInfo and getPinnedConnection
> does what you are suggesting here.
>
> >
> >
> > * Please check the above few comments for both
> > ConnStateData::getPinnedInfo and ConnStateData::getPinnedConnection as
> > they are very similar.
> >
> >
>
> The method getPinnedInfo and getPinnedConnection replaced with:
> - validatePinnedConnection which checks if the pinning info is valid and
> if not, remove any pinning info and closes the pinning peer
> - pinnedPeer which returns the pinning peer
> - unpinConnection which removes any pinning info
>
> I think the new code solves the all the above problems related to the
> getPinnedConnection and getPinnedInfo
>
>
> > *B3* Polish docs
> >
> >> + I was not able to find documentation
> about the use of the header
> >> + "Connection: Proxy-Support" and how other
> http proxies use this header.
> >> + The only I found is a Henrik's comment:
> >> +
> >> + "[...] However, this alone is not
> sufficient. You must also add a
> >> + "Connection: Proxy-support" header to
> mark the extension header as a
> >> + hop-by-hop header to protect from other
> proxies inbetween this proxy
> >> + and the client.
> >
> > I would replace the above with
> >
> > We send "[Proxy-]Connection: Proxy-Support" header to mark
> > Proxy-Support as a hop-by-hop header for intermediaries that do not
> > understand the semantics of this header. The RFC should have included
> > this recommendation.
> >
>
> Done
>
> >
> > *B3* Constant method missing const?
> >
> >> +/**
> >> + * returns true if the peer can support connection pinning
> >> +*/
> >> +bool HttpStateData::peerSupportsConnectionPinning()
> >
> done
>
> >
> > *B3* If you do not have a strong preference, please use
> > /// single line comments
> > for single line comments
> >
> >
> > *B3* Rename peerGetPinned to peerSelectPinned as it is not a getter
> > method. And yes, I know you were following the naming for other peer
> > methods. That is why this is a level-3 comment which you may ignore.
> >
> >> + * peerGetPinned
> >> + *
> >> + * Selects a pinned connection
> done
>
> >
> >
> > *B3* spellcheck:
> >
> > + unsigned int pinned:1; /* Request seont on a pinned
> connection */
>
> done
>
> I hope it is OK, and I am not adding more bugs/problems than those I am
> solving/removing.
>
> Regards,
> Christos
>

Went to build and run test this tonight.
   src/tests/stub_HttpRequest.cc appears to be missing the stub function:

bool HttpRequest::inheritProperties(const HttpMsg *aMsg)
{
     fatal("Not implemented");
     return false;
}

same for src/tests/stub_HttpReply.cc

Other than those it builds. I'll see how it goes under traffic now. I
certainly noticed the rise in clients complaining about MSN failures
when the last build went live without your v1 patch.

Amos

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