Re: [PATCH] Bug 3643: Connection Auth redesign

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

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

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

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

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

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_;

> + Auth::UserRequest::Pointer getConnectionAuth() const { return credentialsState; };

Extra semicolon at the end.

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

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

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

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

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.

Thank you,

Alex.
Received on Tue Mar 26 2013 - 21:16:51 MDT