Re: [MERGE] Connection pinning patch

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 +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?

*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:

> + 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.

* Please check the above few comments for both
ConnStateData::getPinnedInfo and ConnStateData::getPinnedConnection as
they are very similar.

*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.

*B3* Constant method missing const?

> +/**
> + * returns true if the peer can support connection pinning
> +*/
> +bool HttpStateData::peerSupportsConnectionPinning()

*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

*B3* spellcheck:

+ unsigned int pinned:1; /* Request seont on a pinned connection */

Thank you,

Alex.
bb:reject
Received on Mon Sep 22 2008 - 20:38:56 MDT