Re: [PATCH] Bug 3643: Connection Auth redesign

On 27/03/2013 10:16 a.m., Alex Rousskov wrote:
> On 03/25/2013 09:59 PM, Amos Jeffries wrote:
>> "
>> Bug 3643: NTLM helpers stuck in reserved state by Safari
>>
>> NTLM failures are not always cleaning up connection-auth credentials
>> properly. In particular they are not releasing the NTLM helpers when
>> the connection is closed between challenge and handshake completion.
>> Resulting in permanently reserved helpers locking up all access
>> through the proxy.
>>
>> This change redesigns the connection authentication state management
>> to move the auth link/unlink operations into the connection state
>> manager objects instead of being managed by NTLM auth components.
>> As a result we are able to manage credentials from any auth scheme
>> consistently and terminate the connection properly on several
>> error conditions which the auth components are not easily aware of.
>>
>> Fix sponsored by Netbox Blue Pty (http://netboxblue.com/)
>> "
>>
>> The bulk of the patch is symbol changes since we are moving the
>> credentials from a public member variable to private one with accessors.
>>
>> The core of the patch logics is contained in
>> ConnStateData::setConnectionAuth which performs error checking and
>> triggers termination of the connection in various ways determined by the
>> type of error that was encountered.
>>
>> In summary:
>> - once credentials are set they are baked into the connection state.
>> - all following requests require a credentials token matching the state
>> one.
>> - any request lacking credentials (or setting NULL) will terminate the
>> connection gracefully. It should end with an auth re-challenge, but that
>> specific detail does depend on ACLs.
>> - any request with a new or altered auth token (injection attacks,
>> broken relay pinning) will terminate the connection immediately (expect
>> lost bytes).
>> - any pinned server connection, and any pinned stateful auth helper is
>> unpined/released on credential errors.
>>
>> Specific cases and handling are covered in greater detail in the patch
>> comments.
>>
>> It also adds a stub file for client_side.h functionality.
>>
>> Amos
>
>
>> - if (conn->auth_user_request != NULL) {
>> - *auth_user_request = conn->auth_user_request;
>> + if (conn->getConnectionAuth() != NULL) {
>> + *auth_user_request = conn->getConnectionAuth();
>> } else {
>> /* failed connection based authentication */
>> debugs(29, 4, HERE << "Auth user request " <<
>> *auth_user_request << " conn-auth user request " <<
>> - conn->auth_user_request << " conn type " <<
>> - conn->auth_user_request->user()->auth_type << " authentication failed.");
>> + conn->getConnectionAuth() << " conn type " <<
>> + conn->getConnectionAuth()->user()->auth_type << " authentication failed.");
>>
> This is an old bug not introduced by this patch, but please note that
> the last debugs() line above is always dereferencing a NULL pointer (and
> the next-to-last line is always logging a nil pointer).

Fixed.

>
>> + if (credentialsState == NULL) {
>> + debugs(33, 2, HERE << "Adding connection-auth to " << clientConnection << " from " << by);
>> + credentialsState = aur;
>> + return;
>> + }
> The if-statement after the above implies that aur can be NULL. If aur is
> NULL, the above code will not change anything. Should we check for that
> before we claim that we are adding something or at least debugs() aur?
>
> Here and elsewhere, please remove HERE from the new debugs() messages.

Done.

>
>> +private:
>> + /// state of some credentials that can be used to perform authentication on this connection
>> + Auth::UserRequest::Pointer credentialsState;
> The words "state", "data", and "info" usually do not add to the
> description or understanding of a member. How about renaming
> "credentialsState" to just "credentials"?

"UserRequest" is not just the credentials. It is the credentials along
with all the additional state information. ie how, when and if they
validated, any protocol options line realm which are relevant, which
helper was used if stateful, the HelperReply line details etc. whatever
the potocol scheme is written to preserve.
  I think this is possibly one case where saying state is actually
warranted.

Changed anyways with the method re-naming.

>
> Please move the private section down, after the public/protected sections.

Done.

> Also, can the description be simplified to just say "connection-based
> credentials for this connection" or something similar?
Done.

> Finally, why do getConnectionAuth() and setConnectionAuth() use the term
> "connectionAuth" while the class member storing the same information is
> called credentialsState? Can all three names be made consistent/similar?
> I would drop "connection" from the accessor names because the
> ConnStateData class itself determines their scope as "connection". For
> example:
>
> public:
> const Auth::UserRequest::Pointer &credentials() {...}
> void credentials(...);
>
> private:
> Auth::UserRequest::Pointer credentials_;

Used getAuth() / setAuth() / auth_ since its more than just credentials.

>
>
>
>> + Auth::UserRequest::Pointer getConnectionAuth() const { return credentialsState; };
> Extra semicolon at the end.

Sigh. Thought I'd broken that bad habit finally. Thanks

> The above method can return "const Auth::UserRequest::Pointer &" if you
> want to avoid refcounting overheads during simple checks.
>

Seems to work so trying that.

>> + * NP: once set the credentials are fixed until closure because for any change to be needed
>> + * since something invalid has happened:
> Please rephrase the "because ..." part. I am not sure what it means.

Done.

>
>> + if (aur == NULL) {
>> + debugs(33, 2, HERE << "WARNING: Closing " << clientConnection << " due to connection-auth erase from " << by);
>> + credentialsState->releaseAuthServer();
>> + credentialsState = NULL;
>> + unpinConnection();
>> + // XXX: need to test whether the connection re-auth challenge is sent. If not, how to trigger it from here.
>> + // NP: the current situation seems to fix challenge loops in Safari without visible issues in others.
>> + // we stop receiving more traffic but can leave teh Job running to terminate after the error or challenge is delivered.
>> + stopReceiving("connection-auth removed");
>> + return;
>> + }
> The code above does not close the connection as promised by the WARNING.
> It only stops receiving more data from the client. Either the debugging
> or the code that follows the debugging should be changed to be in sync.
>

Fixed.

>> + // clobbered with alternative credentials
>> + if (aur != credentialsState) {
>> + debugs(33, 2, HERE << "ERROR: Closing " << clientConnection << " due to change of connection-auth from " << by);
>> + credentialsState->releaseAuthServer();
>> + credentialsState = NULL;
>> + unpinConnection();
>> + // this is a fatal type of problem. Close the connection immediately
>> + clientConnection->close(); // XXX: need to send TCP RST packet if we can.
>> + stopReceiving("connection-auth change attempted");
>> + return;
>> + }
> IIRC, we should either stopReceiving() or close the connection. Doing
> both is conceptually wrong because stopReceiving() implies that we
> should keep sending if needed (which connection closure prohibits) and
> because connection closure already implies that we will not receive
> anything. Doing both may also lead to confusing double-closures.

For this case we need to close the connection ASAP and data loss of
everything buffered for this connection is the desired outcome.
The stopReceiving() is to both speed up the signalling that the client
connection has closed, and produce a nice error message about why.
  Are you aware of any better way to exit ConnStateData with an error?

I'm trying both stopReceiving(); stopSending(); in this case, but
neither results in TCP RST either. So we still have to call
comm_reset_close() first.

NP: Comm::Connection::close() can be called any number of times and
protects against repeated comm_close() cycles if that was what you mean?

> Use comm_reset_close() to resolve the XXX?
>
>
> Disclaimer: I do not know enough about authentication code to say
> whether the primary changes in the patch are correct. The above can be
> fixed without another round of reviews if nobody needs them, I guess.

It has now had ~6 months in use in a major high performance network with
only one issue which was resolved by using stopReceiving() at the
connection abort case discussed above.

I am also having second thoughts about whether the unpin and
stopReceiving side effects matter when called from swanSong(). That
comment and code was written for the mk1 patch which used deleteThis in
the abort case (and crashed sometimes as a result). What do you think
about just calling setAuth(NULL) there?

New patch attached for review with all the above changes.

Amos