[MERGE] Connection pinning version 2

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