Bug #2009: Negotiate fails with assertion failure

crashed with assertion failure on first request.

Updated the Negotiate implementation to match current NTLM implementation
which gets rid of this. Looks like there has been some patches to NTLM
not applied on Negotiate. The two is pretty much the same code, and any
bugs hitting NTLM will hit Negotiate as well..

diff --git a/src/auth/negotiate/auth_negotiate.cc b/src/auth/negotiate/auth_negotiate.cc
index ffd7ffdc78cc82470e40f3c348562193327cce58..08623a826481e75507eeec56c605c0ddf7b5ccd6 100644 (file)
--- a/src/auth/negotiate/auth_negotiate.cc
+++ b/src/auth/negotiate/auth_negotiate.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: auth_negotiate.cc,v 1.20 2007/05/29 13:31:46 amosjeffries Exp $
+ * $Id: auth_negotiate.cc,v 1.21 2007/07/04 00:59:13 hno Exp $
  *
  * DEBUG: section 29    Negotiate Authenticator
  * AUTHOR: Robert Collins, Henrik Nordstrom, Francesco Chemolli
@@ -174,6 +174,7 @@ void
 AuthNegotiateConfig::init(AuthConfig * scheme)
 {
     if (authenticate) {
+
         authnegotiate_initialised = 1;
 
         if (negotiateauthenticators == NULL)
@@ -286,16 +287,17 @@ AuthNegotiateConfig::fixHeader(AuthUserRequest *auth_user_request, HttpReply *re
 {
     AuthNegotiateUserRequest *negotiate_request;
 
-    if (!request->flags.proxy_keepalive)
-        return;
-
     if (!authenticate)
         return;
 
+    /* Need keep-alive */
+    if (!request->flags.proxy_keepalive && request->flags.must_keepalive)
+       return;
+
     /* New request, no user details */
     if (auth_user_request == NULL) {
-        debugs(29, 9, "AuthNegotiateConfig::fixHeader: Sending type:" << type << " header: 'NEGOTIATE'");
-        httpHeaderPutStrf(&rep->header, type, "NEGOTIATE");
+        debugs(29, 9, "AuthNegotiateConfig::fixHeader: Sending type:" << type << " header: 'Negotiate'");
+        httpHeaderPutStrf(&rep->header, type, "Negotiate");
 
         if (!keep_alive) {
             /* drop the connection */
@@ -333,7 +335,7 @@ AuthNegotiateConfig::fixHeader(AuthUserRequest *auth_user_request, HttpReply *re
         case AUTHENTICATE_STATE_NONE:
             /* semantic change: do not drop the connection.
              * 2.5 implementation used to keep it open - Kinkie */
-            debugs(29, 9, "AuthNegotiateConfig::fixHeader: Sending type:" << type << " header: 'NEGOTIATE'");
+            debugs(29, 9, "AuthNegotiateConfig::fixHeader: Sending type:" << type << " header: 'Negotiate'");
             httpHeaderPutStrf(&rep->header, type, "Negotiate");
             break;
 
@@ -355,7 +357,7 @@ AuthNegotiateConfig::fixHeader(AuthUserRequest *auth_user_request, HttpReply *re
 
 NegotiateUser::~NegotiateUser()
 {
-    debugs(29, 5, "NegotiateUser::~NegotiateUser: doing nothing to clearNEGOTIATE scheme data for '" << this << "'");
+    debugs(29, 5, "NegotiateUser::~NegotiateUser: doing nothing to clearNegotiate scheme data for '" << this << "'");
 }
 
 static stateful_helper_callback_t
@@ -453,13 +455,13 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
 
         result = S_HELPER_RELEASE;
 
-        debugs(29, 4, "authenticateNegotiateHandleReply: Successfully validated user via NEGOTIATE. Username '" << blob << "'");
+        debugs(29, 4, "authenticateNegotiateHandleReply: Successfully validated user via Negotiate. Username '" << blob << "'");
 
         /* connection is authenticated */
         debugs(29, 4, "AuthNegotiateUserRequest::authenticate: authenticated user " << negotiate_user->username());
         /* see if this is an existing user with a different proxy_auth
          * string */
-       auth_user_hash_pointer *usernamehash = static_cast<AuthUserHashPointer *>(hash_lookup(proxy_auth_username_cache, negotiate_user->username()));
+        auth_user_hash_pointer *usernamehash = static_cast<AuthUserHashPointer *>(hash_lookup(proxy_auth_username_cache, negotiate_user->username()));
        auth_user_t *local_auth_user = negotiate_request->user();
         while (usernamehash && (usernamehash->user()->auth_type != AUTH_NEGOTIATE || strcmp(usernamehash->user()->username(), negotiate_user->username()) != 0))
             usernamehash = static_cast<AuthUserHashPointer *>(usernamehash->next);
@@ -480,7 +482,7 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
          * existing user or a new user */
         local_auth_user->expiretime = current_time.tv_sec;
         authenticateNegotiateReleaseServer(negotiate_request);
-        negotiate_request->auth_state = AUTHENTICATE_STATE_DONE;
+       negotiate_request->auth_state = AUTHENTICATE_STATE_DONE;
 
     } else if (strncasecmp(reply, "NA ", 3) == 0 && arg != NULL) {
         /* authentication failure (wrong password, etc.) */
@@ -500,11 +502,11 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
 
         result = S_HELPER_RELEASE;
 
-        debugs(29, 4, "authenticateNegotiateHandleReply: Failed validating user via NEGOTIATE. Error returned '" << blob << "'");
+        debugs(29, 4, "authenticateNegotiateHandleReply: Failed validating user via Negotiate. Error returned '" << blob << "'");
     } else if (strncasecmp(reply, "BH ", 3) == 0) {
         /* TODO kick off a refresh process. This can occur after a YR or after
          * a KK. If after a YR release the helper and resubmit the request via
-         * Authenticate NEGOTIATE start.
+         * Authenticate Negotiate start.
          * If after a KK deny the user's request w/ 407 and mark the helper as
          * Needing YR. */
         auth_user_request->denyMessage(blob);
@@ -512,7 +514,7 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
         safe_free(negotiate_request->server_blob);
         authenticateNegotiateReleaseServer(negotiate_request);
         result = S_HELPER_RELEASE;
-        debugs(29, 1, "authenticateNegotiateHandleReply: Error validating user via NEGOTIATE. Error returned '" << reply << "'");
+        debugs(29, 1, "authenticateNegotiateHandleReply: Error validating user via Negotiate. Error returned '" << reply << "'");
     } else {
         /* protocol error */
         fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply);
@@ -528,7 +530,7 @@ authenticateNegotiateHandleReply(void *data, void *lastserver, char *reply)
 static void
 authenticateNegotiateStats(StoreEntry * sentry)
 {
-    helperStatefulStats(sentry, negotiateauthenticators, "NEGOTIATE Authenticator Statistics");
+    helperStatefulStats(sentry, negotiateauthenticators, "Negotiate Authenticator Statistics");
 }
 
 
@@ -551,7 +553,7 @@ AuthNegotiateUserRequest::module_start(RH * handler, void *data)
     debugs(29, 8, "AuthNegotiateUserRequest::module_start: auth state is '" << auth_state << "'");
 
     if (negotiateConfig.authenticate == NULL) {
-        debugs(29, 0, "AuthNegotiateUserRequest::module_start: no NEGOTIATE program specified.");
+        debugs(29, 0, "AuthNegotiateUserRequest::module_start: no Negotiate program specified.");
         handler(data, NULL);
         return;
     }
@@ -559,7 +561,9 @@ AuthNegotiateUserRequest::module_start(RH * handler, void *data)
     r = cbdataAlloc(authenticateStateData);
     r->handler = handler;
     r->data = cbdataReference(data);
+    r->auth_user_request = this;
     AUTHUSERREQUESTLOCK(r->auth_user_request, "r");
+
     if (auth_state == AUTHENTICATE_STATE_INITIAL) {
         snprintf(buf, 8192, "YR %s\n", client_blob); //CHECKME: can ever client_blob be 0 here?
     } else {
@@ -572,7 +576,7 @@ AuthNegotiateUserRequest::module_start(RH * handler, void *data)
     helperStatefulSubmit(negotiateauthenticators, buf, authenticateNegotiateHandleReply, r, authserver);
 }
 
-/* clear the NEGOTIATE helper of being reserved for future requests */
+/* clear the Negotiate helper of being reserved for future requests */
 static void
 authenticateNegotiateReleaseServer(AuthUserRequest * auth_user_request)
 {
@@ -584,8 +588,12 @@ authenticateNegotiateReleaseServer(AuthUserRequest * auth_user_request)
      * Let's see what happens, might segfault in helperStatefulReleaseServer
      * if it does. I leave it like this not to cover possibly problematic
      * code-paths. Kinkie */
-    helperStatefulReleaseServer(negotiate_request->authserver);
-    negotiate_request->authserver = NULL;
+    /* DPW 2007-05-07
+     * yes, it is possible */
+    if (negotiate_request->authserver) {
+       helperStatefulReleaseServer(negotiate_request->authserver);
+       negotiate_request->authserver = NULL;
+    }
 }
 
 /* clear any connection related authentication details */
@@ -606,11 +614,12 @@ AuthNegotiateUserRequest::onConnectionClose(ConnStateData *conn)
 
     /* unlock the connection based lock */
     debugs(29, 9, "AuthNegotiateUserRequest::onConnectionClose: Unlocking auth_user from the connection '" << conn << "'.");
+
     AUTHUSERREQUESTUNLOCK(conn->auth_user_request, "conn");
 }
 
 /*
- * Decode a NEGOTIATE [Proxy-]Auth string, placing the results in the passed
+ * Decode a Negotiate [Proxy-]Auth string, placing the results in the passed
  * Auth_user structure.
  */
 AuthUserRequest *
@@ -623,8 +632,8 @@ AuthNegotiateConfig::decode(char const *proxy_auth)
     auth_user_request->user()->auth_type = AUTH_NEGOTIATE;
     auth_user_request->user()->addRequest(auth_user_request);
 
-    /* all we have to do is identify that it's NEGOTIATE - the helper does the rest */
-    debugs(29, 9, "AuthNegotiateConfig::decode: NEGOTIATE authentication");
+    /* all we have to do is identify that it's Negotiate - the helper does the rest */
+    debugs(29, 9, "AuthNegotiateConfig::decode: Negotiate authentication");
     return auth_user_request;
 }
 
@@ -665,6 +674,13 @@ AuthNegotiateUserRequest::authenticate(HttpRequest * request, ConnStateData::Poi
         return;
     }
 
+    if (!request->flags.proxy_keepalive) {
+        debugs(29, 2, "AuthNegotiateUserRequest::authenticate: attempt to perform authentication without a persistent connection!");
+        auth_state = AUTHENTICATE_STATE_FAILED;
+       request->flags.must_keepalive = 1;
+        return;
+    }
+
     if (waiting) {
         debugs(29, 1, "AuthNegotiateUserRequest::authenticate: waiting for helper reply!");
         return;
@@ -699,9 +715,9 @@ AuthNegotiateUserRequest::authenticate(HttpRequest * request, ConnStateData::Poi
         safe_free(client_blob);
         client_blob=xstrdup(blob);
         conn->auth_type = AUTH_NEGOTIATE;
-       assert(conn->auth_user_request == NULL);
+        assert(conn->auth_user_request == NULL);
         conn->auth_user_request = this;
-        AUTHUSERREQUESTLOCK(conn->auth_user_request, "conn");
+       AUTHUSERREQUESTLOCK(conn->auth_user_request, "conn");
         return;
 
         break;
@@ -725,6 +741,11 @@ AuthNegotiateUserRequest::authenticate(HttpRequest * request, ConnStateData::Poi
 
         break;
 
+    case AUTHENTICATE_STATE_DONE:
+       fatal("AuthNegotiateUserRequest::authenticate: unexpect auth state DONE! Report a bug to the squid developers.\n");
+
+       break;
+
     case AUTHENTICATE_STATE_FAILED:
         /* we've failed somewhere in authentication */
         debugs(29, 9, "AuthNegotiateUserRequest::authenticate: auth state negotiate failed. " << proxy_auth);
@@ -732,12 +753,6 @@ AuthNegotiateUserRequest::authenticate(HttpRequest * request, ConnStateData::Poi
         return;
 
         break;
-
-    case AUTHENTICATE_STATE_DONE:
-       fatal("AuthNegotiateUserRequest::authenticate: unexpect auth state DONE! Report a bug to the squid developers.\n");
-
-       break;
-
     }
 
     return;