Re: [PATCH] Do not send unretriable requests on reused pinned connections

On 12/06/2012 05:21 AM, Amos Jeffries wrote:
> On 5/12/2012 9:19 a.m., Alex Rousskov wrote:
>> On 12/01/2012 06:02 PM, Alex Rousskov wrote:
>>> To summarize, our choices for a pinned connection created for the
>>> current client are:
>>>
>>> Before sending the request:
>>>
>>> 1. Reopen and repin used pconns to send unretriable requests,
>>> just like non-pinned connection code does. This eliminates
>>> HTTP pconn race conditions for unretriable requests. This is
>>> what the proposed patch does for SslBump.
>>>
>>> 2. Send all requests without reopening the pinned connection.
>>> If we encounter an HTTP pconn race condition, see below.
>>>
>>>
>>> After the request, if an HTTP pconn race happens:
>>>
>>> 0. Send an error message to the client.
>>> This is what we do today and it breaks things.
>>>
>>> 1. Reset the client connection.
>>> Pretend to be a dumb tunnel.
>>>
>>> 2. Retry, just like the current non-pinned connection code does.
>>> This is what the proposed patch implements.
>>>
>>>
>>> Current code does 2+0. That does not work.
>>>
>>> The proposed patch does 1+2. That works in my limited tests.
>>>
>>> Henrik suggested 2+1. I agree that it is also an option worth
>>> considering.
>> I am leaning towards implementing 2+1 _and_ removing the current
>> connection repinning code as not needed.
>>
>> We could leave repinning code in case it is needed later to fix broken
>> clients, but no such clients are known at this time, while that code is
>> complex and requires fixes (the patch posted here earlier), so I am
>> leaning towards removing it completely for now. With some effort it can
>> be re-introduced at a later time, of course.
>>
>> If there are any better ideas, please let me know.
>
> Re-pinning is somewhat useful for the cases of:
> * Kerberos/Bearer auth where credentials on the request are sufficient
> to re-auth the new connection identically to the original one.
> * NAT (not TPROXY) intercepted connections to the same destination
> IP:port. Where verification can lock it down to that destination, but
> not completely.

Hi Amos,

    Since repinning has been declared a bug in general, let's leave
developing support for these special cases for those who need them.
Those folks would be in a better position to advocate that their cases
need repinning _and_ properly detect safe repinning conditions, validate
repinned connections, etc. They would be able to reuse most of our
deleted code, of course, but some new code will be needed to take care
of their cases anyway.

The same applies to any other special cases for repinning that we are
not aware of right now. Hopefully, each repinning case will be reviewed
more carefully in the future to avoid removal.

Thank you,

Alex.
Received on Thu Dec 06 2012 - 15:48:33 MST