From 51a3dd58359bf26bb2bcfeb1f09a0acdf901dd41 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Wed, 20 Apr 2011 17:08:16 +1200 Subject: [PATCH] SourceLayout: Add enum Direction for AuthUserRequests state logics The state of credentials lookup and handling is recorded by authenticateDirection / AuthUserRequest::direction() and its per-scheme helper methods AuthUserRequest::module_direction(). This formalizes and coordinates the state being returned by using a shared enum. The states can generally be considerd as: - LOOKUP with a helper still needs to validate the credentials - CHALLENGE if the helepr needs more info from the client - VALID if everything is fine and the credentials are known Good/Bad - ERROR if there is any problem with the state or credentials TODO: This combination has highlighted a few strange things in the NTLM and Negotiate states. Where known but Failed credentials are marked as ERROR. This needs closer investigation why it is not a CHALLENGE in all auth schemes. Also there is a little obfuscation of the cases around the generalized fixHeader() calls. This will be handled in a followup patch. --- src/auth/UserRequest.cc | 60 +++++++++++++------------------ src/auth/UserRequest.h | 34 ++++++++++++------ src/auth/basic/UserRequest.cc | 16 ++++----- src/auth/basic/UserRequest.h | 2 +- src/auth/digest/UserRequest.cc | 12 +++---- src/auth/digest/UserRequest.h | 2 +- src/auth/negotiate/UserRequest.cc | 17 +++++---- src/auth/negotiate/UserRequest.h | 2 +- src/auth/ntlm/UserRequest.cc | 17 +++++---- src/auth/ntlm/UserRequest.h | 2 +- 10 files changed, 82 insertions(+), 82 deletions(-) diff --git a/src/auth/UserRequest.cc b/src/auth/UserRequest.cc index 94b2fcaea7..6e1cd1c8f3 100644 --- a/src/auth/UserRequest.cc +++ b/src/auth/UserRequest.cc @@ -208,11 +208,14 @@ authenticateUserAuthenticated(AuthUserRequest::Pointer auth_user_request) return auth_user_request->authenticated(); } -int +Auth::Direction AuthUserRequest::direction() { + if (user() == NULL) + return Auth::CRED_ERROR; // No credentials. Should this be a CHALLENGE instead? + if (authenticateUserAuthenticated(this)) - return 0; + return Auth::CRED_VALID; return module_direction(); } @@ -397,42 +400,44 @@ AuthUserRequest::authenticate(AuthUserRequest::Pointer * auth_user_request, http } if (!authenticateUserAuthenticated(*auth_user_request)) { - /* User not logged in. Log them in */ + /* User not logged in. Try to log them in */ authenticateAuthenticateUser(*auth_user_request, request, conn, headertype); - switch (authenticateDirection(*auth_user_request)) { + switch ((*auth_user_request)->direction()) { - case 1: + case Auth::CRED_CHALLENGE: if (request->auth_user_request == NULL) { request->auth_user_request = *auth_user_request; } - /* fallthrough to -2 */ + /* fallthrough to ERROR case and do the challenge */ - case -2: + case Auth::CRED_ERROR: /* this ACL check is finished. */ *auth_user_request = NULL; return AUTH_ACL_CHALLENGE; - case -1: + case Auth::CRED_LOOKUP: /* we are partway through authentication within squid, * the *auth_user_request variables stores the auth_user_request * for the callback to here - Do not Unlock */ return AUTH_ACL_HELPER; - } - /* on 0 the authentication is finished - fallthrough */ - /* See if user authentication failed for some reason */ - if (!authenticateUserAuthenticated(*auth_user_request)) { - if ((*auth_user_request)->username()) { - if (!request->auth_user_request) { - request->auth_user_request = *auth_user_request; + case Auth::CRED_VALID: + /* authentication is finished */ + /* See if user authentication failed for some reason */ + if (!authenticateUserAuthenticated(*auth_user_request)) { + if ((*auth_user_request)->username()) { + if (!request->auth_user_request) { + request->auth_user_request = *auth_user_request; + } } - } - *auth_user_request = NULL; - return AUTH_ACL_CHALLENGE; + *auth_user_request = NULL; + return AUTH_ACL_CHALLENGE; + } + // otherwise fallthrough to acceptance. } } @@ -473,21 +478,6 @@ AuthUserRequest::tryToAuthenticateAndSetAuthUser(AuthUserRequest::Pointer * auth return result; } -/* returns - * 0: no output needed - * 1: send to client - * -1: send to helper - * -2: authenticate broken in some fashion - */ -int -authenticateDirection(AuthUserRequest::Pointer auth_user_request) -{ - if (auth_user_request == NULL || auth_user_request->user() == NULL) - return -2; - - return auth_user_request->direction(); -} - void AuthUserRequest::addReplyAuthHeader(HttpReply * rep, AuthUserRequest::Pointer auth_user_request, HttpRequest * request, int accelerated, int internal) /* send the auth types we are configured to support (and have compiled in!) */ @@ -520,8 +510,8 @@ AuthUserRequest::addReplyAuthHeader(HttpReply * rep, AuthUserRequest::Pointer au /* this is a authenticate-needed response */ { - if ((auth_user_request != NULL) && authenticateDirection(auth_user_request) == 1) - /* scheme specific */ + if (auth_user_request != NULL && auth_user_request->direction() == Auth::CRED_CHALLENGE) + /* add the scheme specific challenge header to the response */ auth_user_request->user()->config->fixHeader(auth_user_request, rep, type, request); else { /* call each configured & running authscheme */ diff --git a/src/auth/UserRequest.h b/src/auth/UserRequest.h index c2db0b434b..b585217a12 100644 --- a/src/auth/UserRequest.h +++ b/src/auth/UserRequest.h @@ -60,6 +60,19 @@ public: time_t ip_expiretime; }; +namespace Auth +{ + +// NP: numeric values specified for old code backward compatibility. +// remove after transition is complete +enum Direction { + CRED_CHALLENGE = 1, ///< Client needs to be challenged. secure token. + CRED_VALID = 0, ///< Credentials are valid and a up to date. The OK/Failed state is accurate. + CRED_LOOKUP = -1, ///< Credentials need to be validated with the backend helper + CRED_ERROR = -2 ///< ERROR in the auth module. Cannot determine the state of this request. +}; +} // namespace Auth + /** \ingroup AuthAPI * This is a short lived structure is the visible aspect of the authentication framework. @@ -83,14 +96,16 @@ public: /** * Used by squid to determine what the next step in performing authentication for a given scheme is. * - \retval -2 ERROR in the auth module. Cannot determine request direction. - \retval -1 The auth module needs to send data to an external helper. - * Squid will prepare for a callback on the request and call the AUTHSSTART function. - \retval 0 The auth module has all the information it needs to perform the authentication and provide a succeed/fail result. - \retval 1 The auth module needs to send a new challenge to the request originator. - * Squid will return the appropriate status code (401 or 407) and call the registered FixError function to allow the auth module to insert it's challenge. + * \retval CRED_ERROR ERROR in the auth module. Cannot determine request direction. + * \retval CRED_LOOKUP The auth module needs to send data to an external helper. + * Squid will prepare for a callback on the request and call the AUTHSSTART function. + * \retval CRED_VALID The auth module has all the information it needs to perform the authentication + * and provide a succeed/fail result. + * \retval CRED_CHALLENGE The auth module needs to send a new challenge to the request originator. + * Squid will return the appropriate status code (401 or 407) and call the registered + * FixError function to allow the auth module to insert it's challenge. */ - int direction(); + Auth::Direction direction(); /** * Used by squid to determine whether the auth scheme has successfully authenticated the user request. @@ -114,7 +129,7 @@ public: virtual void authenticate(HttpRequest * request, ConnStateData * conn, http_hdr_type type) = 0; /* template method */ - virtual int module_direction() = 0; + virtual Auth::Direction module_direction() = 0; virtual void addHeader(HttpReply * rep, int accel); virtual void addTrailer(HttpReply * rep, int accel); virtual void onConnectionClose(ConnStateData *); @@ -194,9 +209,6 @@ extern void authenticateAuthUserRequestRemoveIp(AuthUserRequest::Pointer, Ip::Ad extern void authenticateAuthUserRequestClearIp(AuthUserRequest::Pointer); /// \ingroup AuthAPI extern int authenticateAuthUserRequestIPCount(AuthUserRequest::Pointer); -/// \ingroup AuthAPI -/// \deprecated Use AuthUserRequest::direction() instead. -extern int authenticateDirection(AuthUserRequest::Pointer); /// \ingroup AuthAPI /// See AuthUserRequest::authenticated() diff --git a/src/auth/basic/UserRequest.cc b/src/auth/basic/UserRequest.cc index dd784fe862..bb2d5bedf7 100644 --- a/src/auth/basic/UserRequest.cc +++ b/src/auth/basic/UserRequest.cc @@ -42,29 +42,29 @@ AuthBasicUserRequest::authenticate(HttpRequest * request, ConnStateData * conn, return; } -int +Auth::Direction AuthBasicUserRequest::module_direction() { - /* null auth_user is checked for by authenticateDirection */ + /* null auth_user is checked for by AuthUserRequest::direction() */ if (user()->auth_type != Auth::AUTH_BASIC) - return -2; + return Auth::CRED_ERROR; switch (user()->credentials()) { case Auth::Unchecked: case Auth::Pending: - return -1; + return Auth::CRED_LOOKUP; case Auth::Ok: if (user()->expiretime + static_cast(Auth::Config::Find("basic"))->credentialsTTL <= squid_curtime) - return -1; - return 0; + return Auth::CRED_LOOKUP; + return Auth::CRED_VALID; case Auth::Failed: - return 0; + return Auth::CRED_VALID; default: - return -2; + return Auth::CRED_ERROR; } } diff --git a/src/auth/basic/UserRequest.h b/src/auth/basic/UserRequest.h index fabd599247..ed9f797e0d 100644 --- a/src/auth/basic/UserRequest.h +++ b/src/auth/basic/UserRequest.h @@ -20,7 +20,7 @@ public: virtual int authenticated() const; virtual void authenticate(HttpRequest * request, ConnStateData *conn, http_hdr_type type); - virtual int module_direction(); + virtual Auth::Direction module_direction(); virtual void module_start(RH *, void *); }; diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc index efca45c567..245072b731 100644 --- a/src/auth/digest/UserRequest.cc +++ b/src/auth/digest/UserRequest.cc @@ -166,27 +166,27 @@ AuthDigestUserRequest::authenticate(HttpRequest * request, ConnStateData * conn, return; } -int +Auth::Direction AuthDigestUserRequest::module_direction() { if (user()->auth_type != Auth::AUTH_DIGEST) - return -2; + return Auth::CRED_ERROR; switch (user()->credentials()) { case Auth::Ok: - return 0; + return Auth::CRED_VALID; case Auth::Failed: /* send new challenge */ - return 1; + return Auth::CRED_CHALLENGE; case Auth::Unchecked: case Auth::Pending: - return -1; + return Auth::CRED_LOOKUP; default: - return -2; + return Auth::CRED_ERROR; } } diff --git a/src/auth/digest/UserRequest.h b/src/auth/digest/UserRequest.h index 6c706bf195..c2416728bd 100644 --- a/src/auth/digest/UserRequest.h +++ b/src/auth/digest/UserRequest.h @@ -23,7 +23,7 @@ public: virtual int authenticated() const; virtual void authenticate(HttpRequest * request, ConnStateData * conn, http_hdr_type type); - virtual int module_direction(); + virtual Auth::Direction module_direction(); virtual void addHeader(HttpReply * rep, int accel); #if WAITING_FOR_TE diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc index d08355d932..b50df9eaf9 100644 --- a/src/auth/negotiate/UserRequest.cc +++ b/src/auth/negotiate/UserRequest.cc @@ -58,33 +58,32 @@ AuthNegotiateUserRequest::authenticated() const return 0; } -/* See AuthUserRequest.cc::authenticateDirection for return values */ -int +Auth::Direction AuthNegotiateUserRequest::module_direction() { - /* null auth_user is checked for by authenticateDirection */ + /* null auth_user is checked for by AuthUserRequest::direction() */ if (waiting || client_blob) - return -1; /* need helper response to continue */ + return Auth::CRED_LOOKUP; /* need helper response to continue */ if (user()->auth_type != Auth::AUTH_NEGOTIATE) - return -2; + return Auth::CRED_ERROR; switch (user()->credentials()) { case Auth::Handshake: assert(server_blob); - return 1; /* send to client */ + return Auth::CRED_CHALLENGE; case Auth::Ok: - return 0; /* do nothing */ + return Auth::CRED_VALID; case Auth::Failed: - return -2; + return Auth::CRED_ERROR; // XXX: really? not VALID or CHALLENGE? default: debugs(29, DBG_IMPORTANT, "WARNING: Negotiate Authentication in unexpected state: " << user()->credentials()); - return -2; + return Auth::CRED_ERROR; } } diff --git a/src/auth/negotiate/UserRequest.h b/src/auth/negotiate/UserRequest.h index 657f4b5cd0..f2d372df2f 100644 --- a/src/auth/negotiate/UserRequest.h +++ b/src/auth/negotiate/UserRequest.h @@ -21,7 +21,7 @@ public: virtual ~AuthNegotiateUserRequest(); virtual int authenticated() const; virtual void authenticate(HttpRequest * request, ConnStateData * conn, http_hdr_type type); - virtual int module_direction(); + virtual Auth::Direction module_direction(); virtual void onConnectionClose(ConnStateData *); virtual void module_start(RH *, void *); diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc index 59655df62f..b811357c95 100644 --- a/src/auth/ntlm/UserRequest.cc +++ b/src/auth/ntlm/UserRequest.cc @@ -37,33 +37,32 @@ AuthNTLMUserRequest::connLastHeader() return NULL; } -/* See AuthUserRequest.cc::authenticateDirection for return values */ -int +Auth::Direction AuthNTLMUserRequest::module_direction() { - /* null auth_user is checked for by authenticateDirection */ + /* null auth_user is checked for by AuthUserRequest::direction() */ if (waiting || client_blob) - return -1; /* need helper response to continue */ + return Auth::CRED_LOOKUP; /* need helper response to continue */ if (user()->auth_type != Auth::AUTH_NTLM) - return -2; + return Auth::CRED_ERROR; switch (user()->credentials()) { case Auth::Handshake: assert(server_blob); - return 1; /* send to client */ + return Auth::CRED_CHALLENGE; /* send to client */ case Auth::Ok: - return 0; /* do nothing */ + return Auth::CRED_VALID; case Auth::Failed: - return -2; + return Auth::CRED_ERROR; // XXX really? not VALID or CHALLENGE? default: debugs(29, DBG_IMPORTANT, "WARNING: NTLM Authentication in unexpected state: " << user()->credentials()); - return -2; + return Auth::CRED_ERROR; } } diff --git a/src/auth/ntlm/UserRequest.h b/src/auth/ntlm/UserRequest.h index 189cdcce3b..21561e85d0 100644 --- a/src/auth/ntlm/UserRequest.h +++ b/src/auth/ntlm/UserRequest.h @@ -20,7 +20,7 @@ public: virtual ~AuthNTLMUserRequest(); virtual int authenticated() const; virtual void authenticate(HttpRequest * request, ConnStateData * conn, http_hdr_type type); - virtual int module_direction(); + virtual Auth::Direction module_direction(); virtual void onConnectionClose(ConnStateData *); virtual void module_start(RH *, void *); -- 2.47.3