From: Amos Jeffries Date: Mon, 29 Oct 2012 23:12:04 +0000 (+1300) Subject: Support OK/ERR/BH response codes from any helper X-Git-Tag: SQUID_3_4_0_1~539 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e166785ad679d33acc445d2d96facc065bc63a35;p=thirdparty%2Fsquid.git Support OK/ERR/BH response codes from any helper Updates the helper reponse callback API from using char* buffer to a HelperReply object storing teh response code, a blob buffer, and pointer to the responding helper 'server' (if stateful). * the helper I/O read handler is updated to parse the result code off the start of the helper response as is currently done for channel-ID. The callback handlers are altered to use the HelperReply::status instead of parsing it off themselves individually. * the remaining I/O read buffer is stored in a MemBuf and callbacks are updated to use it via the method other(). * the responding helper-server is stored into the HelperReply object and stateful helper callbacks are combined into the same API as stateless. The callback handlers are updated to use HelperReply::lastserver instead of function parameter. After this patch the helper response format is: [channel-ID] SP [result] [ [SP] blob] 'SP' being one octet \0x20 character. The behavour changes expected from this is that all helpers are now able to send OK/ERR/BH states. Although the handlers for some helpers will deal with the new states as unknown response. None of the bundled helpers have yet been altered to make use of this changed potential. TODO: * implement key=value parser for the blob area of the format, and update handlers to use the HelperReply API to retrieve them. * upgrade helpers to make use of new response format --- diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h index 3c7f817c7e..28108d9300 100644 --- a/src/ClientRequestContext.h +++ b/src/ClientRequestContext.h @@ -14,6 +14,8 @@ class ClientHttpRequest; class DnsLookupDetails; class ErrorState; +class HelperReply; + class ClientRequestContext : public RefCountable { @@ -32,7 +34,7 @@ public: void clientAccessCheck2(); void clientAccessCheckDone(const allow_t &answer); void clientRedirectStart(); - void clientRedirectDone(char *result); + void clientRedirectDone(const HelperReply &reply); void checkNoCache(); void checkNoCacheDone(const allow_t &answer); #if USE_ADAPTATION diff --git a/src/HelperReply.cc b/src/HelperReply.cc new file mode 100644 index 0000000000..983dabd232 --- /dev/null +++ b/src/HelperReply.cc @@ -0,0 +1,94 @@ +#include "squid.h" +#include "HelperReply.h" +#include "helper.h" + +HelperReply::HelperReply(const char *buf, size_t len) : + result(HelperReply::Unknown), + whichServer(NULL) +{ + // check we have something to parse + if (!buf || len < 1) { + // for now ensure that legacy handlers are not presented with NULL strings. + other_.init(1,1); + other_.terminate(); + return; + } + + const char *p = buf; + + // optimization: do not consider parsing result code if the response is short. + // URL-rewriter may return relative URLs or empty response for a large portion + // of its replies. + if (len >= 2) { + // some helper formats (digest auth, URL-rewriter) just send a data string + // we must also check for the ' ' character after the response token (if anything) + if (!strncmp(p,"OK",2) && (len == 2 || p[2] == ' ')) { + result = HelperReply::Okay; + p+=2; + } else if (!strncmp(p,"ERR",3) && (len == 3 || p[3] == ' ')) { + result = HelperReply::Error; + p+=3; + } else if (!strncmp(p,"BH",2) && (len == 2 || p[2] == ' ')) { + result = HelperReply::BrokenHelper; + p+=2; + } else if (!strncmp(p,"TT ",3)) { + // NTLM challenge token + result = HelperReply::TT; + p+=3; + } else if (!strncmp(p,"AF ",3)) { + // NTLM OK response + result = HelperReply::AF; + p+=3; + } else if (!strncmp(p,"NA ",3)) { + // NTLM fail-closed ERR response + result = HelperReply::NA; + p+=3; + } + + for(;xisspace(*p);++p); // skip whitespace + } + + const mb_size_t blobSize = (buf+len-p); + other_.init(blobSize+1, blobSize+1); + other_.append(p, blobSize); // remainders of the line. + + // NULL-terminate so the helper callback handlers do not buffer-overrun + other_.terminate(); +} + +std::ostream & +operator <<(std::ostream &os, const HelperReply &r) +{ + os << "{result="; + switch(r.result) + { + case HelperReply::Okay: + os << "OK"; + break; + case HelperReply::Error: + os << "ERR"; + break; + case HelperReply::BrokenHelper: + os << "BH"; + break; + case HelperReply::TT: + os << "TT"; + break; + case HelperReply::AF: + os << "AF"; + break; + case HelperReply::NA: + os << "NA"; + break; + case HelperReply::Unknown: + os << "Unknown"; + break; + } + + if (r.other().hasContent()) + os << ", other: \"" << r.other().content() << '\"'; + + os << '}'; + + return os; +} diff --git a/src/HelperReply.h b/src/HelperReply.h new file mode 100644 index 0000000000..ea1657d701 --- /dev/null +++ b/src/HelperReply.h @@ -0,0 +1,65 @@ +#ifndef _SQUID_SRC_HELPERREPLY_H +#define _SQUID_SRC_HELPERREPLY_H + +#include "base/CbcPointer.h" +#include "MemBuf.h" + +#if HAVE_OSTREAM +#include +#endif + +class helper_stateful_server; + +/** + * This object stores the reply message from a helper lookup + * It provides parser routing to accept a raw buffer and process the + * helper reply into fields for easy access by callers + */ +class HelperReply +{ +private: + // copy are prohibited for now + HelperReply(const HelperReply &r); + HelperReply &operator =(const HelperReply &r); + +public: + // create/parse details from the msg buffer provided + HelperReply(const char *buf, size_t len); + + const MemBuf &other() const { return other_; } + + /// backward compatibility: + /// access to modifiable blob, required by redirectHandleReply() + /// and by urlParse() in ClientRequestContext::clientRedirectDone() + /// and by token blob/arg parsing in Negotiate auth handler + MemBuf &modifiableOther() const { return *const_cast(&other_); } + +public: + /// The helper response 'result' field. + enum Result_ { + Unknown, // no result code received, or unknown result code + Okay, // "OK" indicating success/positive result + Error, // "ERR" indicating failure/negative result + BrokenHelper, // "BH" indicating failure due to helper internal problems. + + // some result codes for backward compatibility with NTLM/Negotiate + // TODO: migrate these into variants of the above results with key-pair parameters + TT, + AF, + NA + } result; + +// TODO other key=pair values. when the callbacks actually use this object. +// for now they retain their own parsing routines handling other() + + /// for stateful replies the responding helper 'server' needs to be preserved across callbacks + CbcPointer whichServer; + +private: + /// the remainder of the line + MemBuf other_; +}; + +std::ostream &operator <<(std::ostream &os, const HelperReply &r); + +#endif /* _SQUID_SRC_HELPERREPLY_H */ diff --git a/src/Makefile.am b/src/Makefile.am index f908ed2394..6da7b724e9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -364,6 +364,8 @@ squid_SOURCES = \ helper.h \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ HierarchyLogEntry.h \ $(HTCPSOURCE) \ @@ -1452,6 +1454,8 @@ tests_testCacheManager_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ $(HTCPSOURCE) \ HttpStateFlags.h \ http.cc \ @@ -1858,6 +1862,8 @@ tests_testEvent_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -2103,6 +2109,8 @@ tests_testEventLoop_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -2345,6 +2353,8 @@ tests_test_http_range_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -2637,6 +2647,8 @@ tests_testHttpRequest_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ @@ -3724,6 +3736,8 @@ tests_testURL_SOURCES = \ helper.cc \ HelperChildConfig.h \ HelperChildConfig.cc \ + HelperReply.cc \ + HelperReply.h \ hier_code.h \ $(HTCPSOURCE) \ http.cc \ diff --git a/src/SquidDns.h b/src/SquidDns.h index 83cc516cf6..ed44c0b425 100644 --- a/src/SquidDns.h +++ b/src/SquidDns.h @@ -1,6 +1,10 @@ #ifndef SQUID_DNS_H #define SQUID_DNS_H +#if USE_DNSHELPER +#include "helper.h" +#endif + namespace Ip { class Address; diff --git a/src/auth/State.h b/src/auth/State.h index 5722f2ad09..16970418e7 100644 --- a/src/auth/State.h +++ b/src/auth/State.h @@ -5,6 +5,7 @@ #include "auth/UserRequest.h" #include "cbdata.h" +#include "helper.h" namespace Auth { diff --git a/src/auth/UserRequest.h b/src/auth/UserRequest.h index 787bc29f82..f175573bbf 100644 --- a/src/auth/UserRequest.h +++ b/src/auth/UserRequest.h @@ -37,7 +37,7 @@ #include "auth/User.h" #include "dlink.h" #include "ip/Address.h" -#include "typedefs.h" +#include "helper.h" #include "HttpHeader.h" class ConnStateData; diff --git a/src/auth/basic/UserRequest.cc b/src/auth/basic/UserRequest.cc index a990818461..a3d9651579 100644 --- a/src/auth/basic/UserRequest.cc +++ b/src/auth/basic/UserRequest.cc @@ -5,6 +5,7 @@ #include "auth/State.h" #include "charset.h" #include "Debug.h" +#include "HelperReply.h" #include "rfc1738.h" #include "SquidTime.h" @@ -135,23 +136,12 @@ Auth::Basic::UserRequest::module_start(AUTHCB * handler, void *data) } void -Auth::Basic::UserRequest::HandleReply(void *data, char *reply) +Auth::Basic::UserRequest::HandleReply(void *data, const HelperReply &reply) { Auth::StateData *r = static_cast(data); BasicAuthQueueNode *tmpnode; - char *t = NULL; void *cbdata; - debugs(29, 5, HERE << "{" << (reply ? reply : "") << "}"); - - if (reply) { - if ((t = strchr(reply, ' '))) { - *t = '\0'; - ++t; - } - - if (*reply == '\0') - reply = NULL; - } + debugs(29, 5, HERE << "reply=" << reply); assert(r->auth_user_request != NULL); assert(r->auth_user_request->user()->auth_type == Auth::AUTH_BASIC); @@ -162,13 +152,13 @@ Auth::Basic::UserRequest::HandleReply(void *data, char *reply) assert(basic_auth != NULL); - if (reply && (strncasecmp(reply, "OK", 2) == 0)) + if (reply.result == HelperReply::Okay) basic_auth->credentials(Auth::Ok); else { basic_auth->credentials(Auth::Failed); - if (t && *t) - r->auth_user_request->setDenyMessage(t); + if (reply.other().hasContent()) + r->auth_user_request->setDenyMessage(reply.other().content()); } basic_auth->expiretime = squid_curtime; diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc index 11764a319e..2caea95f1b 100644 --- a/src/auth/digest/UserRequest.cc +++ b/src/auth/digest/UserRequest.cc @@ -272,27 +272,18 @@ Auth::Digest::UserRequest::module_start(AUTHCB * handler, void *data) } void -Auth::Digest::UserRequest::HandleReply(void *data, char *reply) +Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply) { Auth::StateData *replyData = static_cast(data); - char *t = NULL; - void *cbdata; - debugs(29, 9, HERE << "{" << (reply ? reply : "") << "}"); - - if (reply) { - if ((t = strchr(reply, ' '))) { - *t = '\0'; - ++t; - } - - if (*reply == '\0' || *reply == '\n') - reply = NULL; - } + debugs(29, 9, HERE << "reply=" << reply); assert(replyData->auth_user_request != NULL); Auth::UserRequest::Pointer auth_user_request = replyData->auth_user_request; - if (reply && (strncasecmp(reply, "ERR", 3) == 0)) { + switch(reply.result) + { + case HelperReply::Error: + { /* allow this because the digest_request pointer is purely local */ Auth::Digest::UserRequest *digest_request = dynamic_cast(auth_user_request.getRaw()); assert(digest_request); @@ -300,17 +291,28 @@ Auth::Digest::UserRequest::HandleReply(void *data, char *reply) digest_request->user()->credentials(Auth::Failed); digest_request->flags.invalid_password = 1; - if (t && *t) - digest_request->setDenyMessage(t); - } else if (reply) { + if (reply.other().hasContent()) + digest_request->setDenyMessage(reply.other().content()); + } + break; + + case HelperReply::Unknown: // Squid 3.2 and older the digest helper only returns a HA1 hash (no "OK") + case HelperReply::Okay: + { /* allow this because the digest_request pointer is purely local */ Auth::Digest::User *digest_user = dynamic_cast(auth_user_request->user().getRaw()); assert(digest_user != NULL); - CvtBin(reply, digest_user->HA1); + CvtBin(reply.other().content(), digest_user->HA1); digest_user->HA1created = 1; } + break; + + default: + ; // XXX: handle other states properly. + } + void *cbdata = NULL; if (cbdataReferenceValidDone(replyData->data, &cbdata)) replyData->handler(cbdata); diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc index 1b0aef6299..19e99fce78 100644 --- a/src/auth/negotiate/UserRequest.cc +++ b/src/auth/negotiate/UserRequest.cc @@ -232,25 +232,18 @@ Auth::Negotiate::UserRequest::authenticate(HttpRequest * aRequest, ConnStateData } void -Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *reply) +Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply) { Auth::StateData *r = static_cast(data); - char *blob, *arg = NULL; - - debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply ? reply : "") << "'"); + debugs(29, 8, HERE << "helper: '" << reply.whichServer << "' sent us reply=" << reply); if (!cbdataReferenceValid(r->data)) { - debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication invalid callback data. helper '" << lastserver << "'."); + debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication invalid callback data. helper '" << reply.whichServer << "'."); delete r; return; } - if (!reply) { - debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication Helper '" << lastserver << "' crashed!."); - reply = (char *)"BH Internal error"; - } - Auth::UserRequest::Pointer auth_user_request = r->auth_user_request; assert(auth_user_request != NULL); @@ -265,26 +258,26 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re assert(auth_user_request->user()->auth_type == Auth::AUTH_NEGOTIATE); if (lm_request->authserver == NULL) - lm_request->authserver = static_cast(lastserver); + lm_request->authserver = reply.whichServer.get(); // XXX: no locking? else - assert(lm_request->authserver == lastserver); + assert(reply.whichServer == lm_request->authserver); /* seperate out the useful data */ - blob = strchr(reply, ' '); - - if (blob) { - ++blob; - arg = strchr(blob + 1, ' '); - } else { - arg = NULL; - } - - if (strncasecmp(reply, "TT ", 3) == 0) { - /* we have been given a blob to send to the client */ + char *modifiableBlob = reply.modifiableOther().content(); + char *arg = NULL; + if (modifiableBlob && *modifiableBlob != '\0') { + arg = strchr(modifiableBlob + 1, ' '); if (arg) { *arg = '\0'; ++arg; } + } + const char *blob = modifiableBlob; + + switch(reply.result) + { + case HelperReply::TT: + /* we have been given a blob to send to the client */ safe_free(lm_request->server_blob); lm_request->request->flags.mustKeepalive = 1; if (lm_request->request->flags.proxyKeepalive) { @@ -296,14 +289,19 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re auth_user_request->user()->credentials(Auth::Failed); auth_user_request->denyMessage("NTLM authentication requires a persistent connection"); } - } else if (strncasecmp(reply, "AF ", 3) == 0 && arg != NULL) { - /* we're finished, release the helper */ + break; - if (arg) { - *arg = '\0'; - ++arg; + case HelperReply::AF: + case HelperReply::Okay: + { + if (arg == NULL) { + // XXX: handle a success with no username better + /* protocol error */ + fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply.other().content()); + break; } + /* we're finished, release the helper */ auth_user_request->user()->username(arg); auth_user_request->denyMessage("Login successful"); safe_free(lm_request->server_blob); @@ -336,23 +334,32 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re * existing user or a new user */ local_auth_user->expiretime = current_time.tv_sec; auth_user_request->user()->credentials(Auth::Ok); - debugs(29, 4, HERE << "Successfully validated user via Negotiate. Username '" << blob << "'"); - - } else if (strncasecmp(reply, "NA ", 3) == 0 && arg != NULL) { - /* authentication failure (wrong password, etc.) */ + debugs(29, 4, HERE << "Successfully validated user via Negotiate. Username '" << arg << "'"); + } + break; - if (arg) { - *arg = '\0'; - ++arg; + case HelperReply::NA: + case HelperReply::Error: + if (arg == NULL) { + /* protocol error */ + fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply.other().content()); + break; } - + /* authentication failure (wrong password, etc.) */ auth_user_request->denyMessage(arg); auth_user_request->user()->credentials(Auth::Failed); safe_free(lm_request->server_blob); lm_request->server_blob = xstrdup(blob); lm_request->releaseAuthServer(); - debugs(29, 4, HERE << "Failed validating user via Negotiate. Error returned '" << blob << "'"); - } else if (strncasecmp(reply, "BH ", 3) == 0) { + debugs(29, 4, HERE << "Failed validating user via Negotiate. Error returned '" << reply << "'"); + break; + + case HelperReply::Unknown: + debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication Helper '" << reply.whichServer << "' crashed!."); + blob = "Internal error"; + /* continue to the next case */ + + case HelperReply::BrokenHelper: /* 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. @@ -362,12 +369,10 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, void *lastserver, char *re auth_user_request->user()->credentials(Auth::Failed); safe_free(lm_request->server_blob); lm_request->releaseAuthServer(); - debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication validating user. Error returned '" << reply << "'"); - } else { - /* protocol error */ - fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply); + debugs(29, DBG_IMPORTANT, "ERROR: Negotiate Authentication validating user. Error returned " << reply); } + xfree(arg); if (lm_request->request) { HTTPMSGUNLOCK(lm_request->request); lm_request->request = NULL; diff --git a/src/auth/negotiate/UserRequest.h b/src/auth/negotiate/UserRequest.h index 478680cfcd..79c0e19f19 100644 --- a/src/auth/negotiate/UserRequest.h +++ b/src/auth/negotiate/UserRequest.h @@ -52,7 +52,7 @@ public: HttpRequest *request; private: - static HLPSCB HandleReply; + static HLPCB HandleReply; }; } // namespace Negotiate diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc index 2d31ff4f3d..80f33f2e09 100644 --- a/src/auth/ntlm/UserRequest.cc +++ b/src/auth/ntlm/UserRequest.cc @@ -225,24 +225,18 @@ Auth::Ntlm::UserRequest::authenticate(HttpRequest * aRequest, ConnStateData * co } void -Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply) +Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply) { Auth::StateData *r = static_cast(data); - char *blob; - debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply ? reply : "") << "'"); + debugs(29, 8, HERE << "helper: '" << reply.whichServer << "' sent us reply=" << reply); if (!cbdataReferenceValid(r->data)) { - debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication invalid callback data. helper '" << lastserver << "'."); + debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication invalid callback data. helper '" << reply.whichServer << "'."); delete r; return; } - if (!reply) { - debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication Helper '" << lastserver << "' crashed!."); - reply = (char *)"BH Internal error"; - } - Auth::UserRequest::Pointer auth_user_request = r->auth_user_request; assert(auth_user_request != NULL); @@ -257,16 +251,16 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply) assert(auth_user_request->user()->auth_type == Auth::AUTH_NTLM); if (lm_request->authserver == NULL) - lm_request->authserver = static_cast(lastserver); + lm_request->authserver = reply.whichServer.get(); // XXX: no locking? else - assert(lm_request->authserver == lastserver); + assert(reply.whichServer == lm_request->authserver); /* seperate out the useful data */ - blob = strchr(reply, ' '); - if (blob) - ++blob; + const char *blob = reply.other().content(); - if (strncasecmp(reply, "TT ", 3) == 0) { + switch(reply.result) + { + case HelperReply::TT: /* we have been given a blob to send to the client */ safe_free(lm_request->server_blob); lm_request->request->flags.mustKeepalive = 1; @@ -279,7 +273,11 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply) auth_user_request->user()->credentials(Auth::Failed); auth_user_request->denyMessage("NTLM authentication requires a persistent connection"); } - } else if (strncasecmp(reply, "AF ", 3) == 0) { + break; + + case HelperReply::AF: + case HelperReply::Okay: + { /* we're finished, release the helper */ auth_user_request->user()->username(blob); auth_user_request->denyMessage("Login successful"); @@ -314,15 +312,25 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply) local_auth_user->expiretime = current_time.tv_sec; auth_user_request->user()->credentials(Auth::Ok); debugs(29, 4, HERE << "Successfully validated user via NTLM. Username '" << blob << "'"); + } + break; - } else if (strncasecmp(reply, "NA ", 3) == 0) { + case HelperReply::NA: + case HelperReply::Error: /* authentication failure (wrong password, etc.) */ auth_user_request->denyMessage(blob); auth_user_request->user()->credentials(Auth::Failed); safe_free(lm_request->server_blob); lm_request->releaseAuthServer(); debugs(29, 4, HERE << "Failed validating user via NTLM. Error returned '" << blob << "'"); - } else if (strncasecmp(reply, "BH ", 3) == 0) { + break; + + case HelperReply::Unknown: + debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication Helper '" << reply.whichServer << "' crashed!."); + blob = "Internal error"; + /* continue to the next case */ + + case HelperReply::BrokenHelper: /* 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 NTLM start. @@ -333,9 +341,7 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, void *lastserver, char *reply) safe_free(lm_request->server_blob); lm_request->releaseAuthServer(); debugs(29, DBG_IMPORTANT, "ERROR: NTLM Authentication validating user. Error returned '" << reply << "'"); - } else { - /* protocol error */ - fatalf("authenticateNTLMHandleReply: *** Unsupported helper response ***, '%s'\n", reply); + break; } if (lm_request->request) { diff --git a/src/auth/ntlm/UserRequest.h b/src/auth/ntlm/UserRequest.h index 82e72b101d..406ae5e305 100644 --- a/src/auth/ntlm/UserRequest.h +++ b/src/auth/ntlm/UserRequest.h @@ -48,7 +48,7 @@ public: HttpRequest *request; private: - static HLPSCB HandleReply; + static HLPCB HandleReply; }; } // namespace Ntlm diff --git a/src/client_side.cc b/src/client_side.cc index d3d8d7ff44..0ec0ece14a 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3689,23 +3689,23 @@ httpsAccept(const CommAcceptCbParams ¶ms) } void -ConnStateData::sslCrtdHandleReplyWrapper(void *data, char *reply) +ConnStateData::sslCrtdHandleReplyWrapper(void *data, const HelperReply &reply) { ConnStateData * state_data = (ConnStateData *)(data); state_data->sslCrtdHandleReply(reply); } void -ConnStateData::sslCrtdHandleReply(const char * reply) +ConnStateData::sslCrtdHandleReply(const HelperReply &reply) { - if (!reply) { + if (!reply.other().hasContent()) { debugs(1, DBG_IMPORTANT, HERE << "\"ssl_crtd\" helper return reply"); } else { Ssl::CrtdMessage reply_message; - if (reply_message.parse(reply, strlen(reply)) != Ssl::CrtdMessage::OK) { + if (reply_message.parse(reply.other().content(), reply.other().contentSize()) != Ssl::CrtdMessage::OK) { debugs(33, 5, HERE << "Reply from ssl_crtd for " << sslConnectHostOrIp << " is incorrect"); } else { - if (reply_message.getCode() != "OK") { + if (reply.result != HelperReply::Okay) { debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " cannot be generated. ssl_crtd response: " << reply_message.getBody()); } else { debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " was successfully recieved from ssl_crtd"); diff --git a/src/client_side.h b/src/client_side.h index 2df5c9b19b..de9b94c4f9 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -53,6 +53,7 @@ class ConnStateData; class ClientHttpRequest; class clientStreamNode; class ChunkedCodingParser; +class HelperReply; namespace AnyP { class PortCfg; @@ -342,9 +343,9 @@ public: */ void getSslContextDone(SSL_CTX * sslContext, bool isNew = false); /// Callback function. It is called when squid receive message from ssl_crtd. - static void sslCrtdHandleReplyWrapper(void *data, char *reply); + static void sslCrtdHandleReplyWrapper(void *data, const HelperReply &reply); /// Proccess response from ssl_crtd. - void sslCrtdHandleReply(const char * reply); + void sslCrtdHandleReply(const HelperReply &reply); void switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode); bool switchedToHttps() const { return switchedToHttps_; } diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 4070550e8b..047b672ad7 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -57,6 +57,7 @@ #include "fde.h" #include "format/Token.h" #include "gopher.h" +#include "helper.h" #include "http.h" #include "HttpHdrCc.h" #include "HttpReply.h" @@ -67,6 +68,7 @@ #include "MemObject.h" #include "Parsing.h" #include "profiler/Profiler.h" +#include "redirect.h" #include "SquidConfig.h" #include "SquidTime.h" #include "Store.h" @@ -129,7 +131,7 @@ static void sslBumpAccessCheckDoneWrapper(allow_t, void *); #endif static int clientHierarchical(ClientHttpRequest * http); static void clientInterpretRequestHeaders(ClientHttpRequest * http); -static RH clientRedirectDoneWrapper; +static HLPCB clientRedirectDoneWrapper; static void checkNoCacheDoneWrapper(allow_t, void *); SQUIDCEXTERN CSR clientGetMoreData; SQUIDCEXTERN CSS clientReplyStatus; @@ -900,7 +902,7 @@ clientRedirectAccessCheckDone(allow_t answer, void *data) if (answer == ACCESS_ALLOWED) redirectStart(http, clientRedirectDoneWrapper, context); else - context->clientRedirectDone(NULL); + context->clientRedirectDone(HelperReply(NULL,0)); } void @@ -1181,7 +1183,7 @@ clientInterpretRequestHeaders(ClientHttpRequest * http) } void -clientRedirectDoneWrapper(void *data, char *result) +clientRedirectDoneWrapper(void *data, const HelperReply &result) { ClientRequestContext *calloutContext = (ClientRequestContext *)data; @@ -1192,14 +1194,19 @@ clientRedirectDoneWrapper(void *data, char *result) } void -ClientRequestContext::clientRedirectDone(char *result) +ClientRequestContext::clientRedirectDone(const HelperReply &reply) { HttpRequest *old_request = http->request; - debugs(85, 5, "clientRedirectDone: '" << http->uri << "' result=" << (result ? result : "NULL")); + debugs(85, 5, HERE << "'" << http->uri << "' result=" << reply); assert(redirect_state == REDIRECT_PENDING); redirect_state = REDIRECT_DONE; - if (result) { + if (reply.other().hasContent()) { + /* 2012-06-28: This cast is due to urlParse() truncating too-long URLs itself. + * At this point altering the helper buffer in that way is not harmful, but annoying. + * When Bug 1961 is resolved and urlParse has a const API, this needs to die. + */ + char * result = const_cast(reply.other().content()); http_status status = (http_status) atoi(result); if (status == HTTP_MOVED_PERMANENTLY @@ -1207,7 +1214,7 @@ ClientRequestContext::clientRedirectDone(char *result) || status == HTTP_SEE_OTHER || status == HTTP_PERMANENT_REDIRECT || status == HTTP_TEMPORARY_REDIRECT) { - char *t = result; + char *t = NULL; if ((t = strchr(result, ':')) != NULL) { http->redirect.status = status; diff --git a/src/client_side_request.h b/src/client_side_request.h index ca12ad247c..a05177f87e 100644 --- a/src/client_side_request.h +++ b/src/client_side_request.h @@ -206,7 +206,6 @@ int clientHttpRequestStatus(int fd, ClientHttpRequest const *http); void clientAccessCheck(ClientHttpRequest *); /* ones that should be elsewhere */ -void redirectStart(ClientHttpRequest *, RH *, void *); void tunnelStart(ClientHttpRequest *, int64_t *, int *); #if _USE_INLINE_ diff --git a/src/dns.cc b/src/dns.cc index 31b83d18a9..f119433945 100644 --- a/src/dns.cc +++ b/src/dns.cc @@ -33,6 +33,7 @@ #include "squid.h" #include "helper.h" +#include "HelperReply.h" #include "mgr/Registration.h" #include "SquidConfig.h" #include "SquidTime.h" @@ -129,8 +130,8 @@ dnsSubmit(const char *lookup, HLPCB * callback, void *data) debugs(34, DBG_IMPORTANT, "dnsSubmit: queue overload, rejecting " << lookup); - callback(data, (char *)"$fail Temporary network problem, please retry later"); - + const char *t = "$fail Temporary network problem, please retry later"; + callback(data, HelperReply(t, strlen(t))); return; } diff --git a/src/external_acl.cc b/src/external_acl.cc index b9cb7d19d8..4dcd91ae4a 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -1306,30 +1306,28 @@ free_externalAclState(void *data) * the whitespace escaped using \ (\ escaping obviously also applies to * any " characters) */ - static void -externalAclHandleReply(void *data, char *reply) +externalAclHandleReply(void *data, const HelperReply &reply) { externalAclState *state = static_cast(data); externalAclState *next; - char *status; - char *token; - char *value; char *t = NULL; ExternalACLEntryData entryData; entryData.result = ACCESS_DENIED; external_acl_entry *entry = NULL; - debugs(82, 2, "externalAclHandleReply: reply=\"" << reply << "\""); + debugs(82, 2, HERE << "reply=" << reply); - if (reply) { - status = strwordtok(reply, &t); + if (reply.result == HelperReply::Okay) + entryData.result = ACCESS_ALLOWED; + // XXX: handle other non-DENIED results better - if (status && strcmp(status, "OK") == 0) - entryData.result = ACCESS_ALLOWED; + if (reply.other().hasContent()) { + char *temp = reply.modifiableOther().content(); + char *token = strwordtok(temp, &t); while ((token = strwordtok(NULL, &t))) { - value = strchr(token, '='); + char *value = strchr(token, '='); if (value) { *value = '\0'; /* terminate the token, and move up to the value */ @@ -1363,7 +1361,8 @@ externalAclHandleReply(void *data, char *reply) dlinkDelete(&state->list, &state->def->queue); if (cbdataReferenceValid(state->def)) { - if (reply) + // only cache OK and ERR results. + if (reply.result == HelperReply::Okay || reply.result == HelperReply::Error) entry = external_acl_cache_add(state->def, state->key, entryData); else { external_acl_entry *oldentry = (external_acl_entry *)hash_lookup(state->def->cache, state->key); diff --git a/src/fqdncache.cc b/src/fqdncache.cc index 2530ac8552..f341bd329c 100644 --- a/src/fqdncache.cc +++ b/src/fqdncache.cc @@ -34,6 +34,7 @@ #include "cbdata.h" #include "DnsLookupDetails.h" #include "event.h" +#include "HelperReply.h" #include "Mem.h" #include "mgr/Registration.h" #include "SquidConfig.h" @@ -497,7 +498,7 @@ fqdncacheParse(fqdncache_entry *f, const rfc1035_rr * answers, int nr, const cha */ static void #if USE_DNSHELPER -fqdncacheHandleReply(void *data, char *reply) +fqdncacheHandleReply(void *data, const HelperReply &reply) #else fqdncacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char *error_message) #endif @@ -509,7 +510,7 @@ fqdncacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char statCounter.dns.svcTime.count(age); #if USE_DNSHELPER - fqdncacheParse(f, reply); + fqdncacheParse(f, reply.other().content()); #else fqdncacheParse(f, answers, na, error_message); diff --git a/src/helper.cc b/src/helper.cc index 85d0dc0392..4be084bdb1 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -398,7 +398,7 @@ helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data) { if (hlp == NULL) { debugs(84, 3, "helperSubmit: hlp == NULL"); - callback(data, NULL); + callback(data, HelperReply(NULL,0)); return; } @@ -419,11 +419,11 @@ helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data) /// lastserver = "server last used as part of a reserved request sequence" void -helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver) +helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver) { if (hlp == NULL) { debugs(84, 3, "helperStatefulSubmit: hlp == NULL"); - callback(data, 0, NULL); + callback(data, HelperReply(NULL,0)); return; } @@ -751,11 +751,12 @@ helperServerFree(helper_server *srv) } for (i = 0; i < concurrency; ++i) { + // XXX: re-schedule these on another helper? if ((r = srv->requests[i])) { void *cbdata; if (cbdataReferenceValidDone(r->data, &cbdata)) - r->callback(cbdata, NULL); + r->callback(cbdata, HelperReply(NULL,0)); helperRequestFree(r); @@ -818,8 +819,11 @@ helperStatefulServerFree(helper_stateful_server *srv) if ((r = srv->request)) { void *cbdata; - if (cbdataReferenceValidDone(r->data, &cbdata)) - r->callback(cbdata, srv, NULL); + if (cbdataReferenceValidDone(r->data, &cbdata)) { + HelperReply nilReply(NULL,0); + nilReply.whichServer = srv; + r->callback(cbdata, nilReply); + } helperStatefulRequestFree(r); @@ -835,10 +839,14 @@ helperStatefulServerFree(helper_stateful_server *srv) } /// Calls back with a pointer to the buffer with the helper output -static void helperReturnBuffer(int request_number, helper_server * srv, helper * hlp, char * msg, char * msg_end) +static void +helperReturnBuffer(int request_number, helper_server * srv, helper * hlp, char * msg, char * msg_end) { helper_request *r = srv->requests[request_number]; if (r) { +// TODO: parse the reply into new helper reply object +// pass that to the callback instead of msg + HLPCB *callback = r->callback; srv->requests[request_number] = NULL; @@ -847,7 +855,7 @@ static void helperReturnBuffer(int request_number, helper_server * srv, helper * void *cbdata = NULL; if (cbdataReferenceValidDone(r->data, &cbdata)) - callback(cbdata, msg); + callback(cbdata, HelperReply(msg, (msg_end-msg))); -- srv->stats.pending; ++ srv->stats.replies; @@ -1017,7 +1025,9 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *buf, size_t *t = '\0'; if (r && cbdataReferenceValid(r->data)) { - r->callback(r->data, srv, srv->rbuf); + HelperReply res(srv->rbuf, (t - srv->rbuf)); + res.whichServer = srv; + r->callback(r->data, res); } else { debugs(84, DBG_IMPORTANT, "StatefulHandleRead: no callback data registered"); called = 0; @@ -1350,11 +1360,13 @@ helperStatefulDispatch(helper_stateful_server * srv, helper_stateful_request * r /* a callback is needed before this request can _use_ a helper. */ /* we don't care about releasing this helper. The request NEVER * gets to the helper. So we throw away the return code */ - r->callback(r->data, srv, NULL); + HelperReply nilReply(NULL,0); + nilReply.whichServer = srv; + r->callback(r->data, nilReply); /* throw away the placeholder */ helperStatefulRequestFree(r); /* and push the queue. Note that the callback may have submitted a new - * request to the helper which is why we test for the request*/ + * request to the helper which is why we test for the request */ if (srv->request == NULL) helperStatefulServerDone(srv); diff --git a/src/helper.h b/src/helper.h index d929049c26..1ee9d5e904 100644 --- a/src/helper.h +++ b/src/helper.h @@ -39,10 +39,11 @@ #include "dlink.h" #include "ip/Address.h" #include "HelperChildConfig.h" +#include "HelperReply.h" class helper_request; -typedef void HLPSCB(void *, void *lastserver, char *buf); +typedef void HLPCB(void *, const HelperReply &reply); class helper { @@ -191,7 +192,7 @@ class helper_stateful_request public: MEMPROXY_CLASS(helper_stateful_request); char *buf; - HLPSCB *callback; + HLPCB *callback; int placeholder; /* if 1, this is a dummy request waiting for a stateful helper to become available */ void *data; }; @@ -202,7 +203,7 @@ MEMPROXY_CLASS_INLINE(helper_stateful_request); void helperOpenServers(helper * hlp); void helperStatefulOpenServers(statefulhelper * hlp); void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data); -void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver); +void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver); void helperStats(StoreEntry * sentry, helper * hlp, const char *label = NULL); void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label = NULL); void helperShutdown(helper * hlp); diff --git a/src/ipcache.cc b/src/ipcache.cc index dba1ce0734..2d1071da71 100644 --- a/src/ipcache.cc +++ b/src/ipcache.cc @@ -595,7 +595,7 @@ ipcacheParse(ipcache_entry *i, const rfc1035_rr * answers, int nr, const char *e /// \ingroup IPCacheInternal static void #if USE_DNSHELPER -ipcacheHandleReply(void *data, char *reply) +ipcacheHandleReply(void *data, const HelperReply &reply) #else ipcacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char *error_message) #endif @@ -607,7 +607,7 @@ ipcacheHandleReply(void *data, const rfc1035_rr * answers, int na, const char *e statCounter.dns.svcTime.count(age); #if USE_DNSHELPER - ipcacheParse(i, reply); + ipcacheParse(i, reply.other().content()); #else int done = ipcacheParse(i, answers, na, error_message); diff --git a/src/redirect.cc b/src/redirect.cc index bb021be12a..63aadb3b5b 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -40,7 +40,6 @@ #include "fqdncache.h" #include "globals.h" #include "HttpRequest.h" -#include "helper.h" #include "mgr/Registration.h" #include "redirect.h" #include "rfc1738.h" @@ -63,7 +62,7 @@ typedef struct { Ip::Address client_addr; const char *client_ident; const char *method_s; - RH *handler; + HLPCB *handler; } redirectStateData; static HLPCB redirectHandleReply; @@ -74,21 +73,36 @@ static int n_bypassed = 0; CBDATA_TYPE(redirectStateData); static void -redirectHandleReply(void *data, char *reply) +redirectHandleReply(void *data, const HelperReply &reply) { redirectStateData *r = static_cast(data); - char *t; - void *cbdata; - debugs(61, 5, "redirectHandleRead: {" << (reply && *reply != '\0' ? reply : "") << "}"); - - if (reply) { - if ((t = strchr(reply, ' '))) - *t = '\0'; - - if (*reply == '\0') - reply = NULL; + debugs(61, 5, HERE << "reply=" << reply); + + // XXX: This funtion is now kept only to check for and display this garbage use-case + // it can be removed when the helpers are all updated to the normalized "OK/ERR key-pairs" format + + if (reply.result == HelperReply::Unknown) { + // BACKWARD COMPATIBILITY 2012-06-15: + // Some nasty old helpers send back the entire input line including extra format keys. + // This is especially bad for simple perl search-replace filter scripts. + // + // * trim all but the first word off the response. + // * warn once every 50 responses that this will stop being fixed-up soon. + // + if (const char * res = reply.other().content()) { + if (const char *t = strchr(res, ' ')) { + static int warn = 0; + debugs(61, (!(warn++%50)? DBG_CRITICAL:2), "UPGRADE WARNING: URL rewriter reponded with garbage '" << t << + "'. Future Squid will treat this as part of the URL."); + const mb_size_t garbageLength = reply.other().contentSize() - (t-res); + reply.modifiableOther().truncate(garbageLength); + } + if (reply.other().hasContent() && *res == '\0') + reply.modifiableOther().clean(); // drop the whole buffer of garbage. + } } + void *cbdata; if (cbdataReferenceValidDone(r->data, &cbdata)) r->handler(cbdata, reply); @@ -120,7 +134,7 @@ redirectStats(StoreEntry * sentry) /**** PUBLIC FUNCTIONS ****/ void -redirectStart(ClientHttpRequest * http, RH * handler, void *data) +redirectStart(ClientHttpRequest * http, HLPCB * handler, void *data) { ConnStateData * conn = http->getConn(); redirectStateData *r = NULL; @@ -137,7 +151,7 @@ redirectStart(ClientHttpRequest * http, RH * handler, void *data) if (Config.onoff.redirector_bypass && redirectors->stats.queue_size) { /* Skip redirector if there is one request queued */ ++n_bypassed; - handler(data, NULL); + handler(data, HelperReply(NULL,0)); return; } diff --git a/src/redirect.h b/src/redirect.h index 6cbbdd3161..d7f73a1199 100644 --- a/src/redirect.h +++ b/src/redirect.h @@ -33,7 +33,12 @@ * */ +#include "helper.h" + +class ClientHttpRequest; + void redirectInit(void); void redirectShutdown(void); +void redirectStart(ClientHttpRequest *, HLPCB *, void *); #endif /* SQUID_REDIRECT_H_ */ diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index 6c8803b99f..d3792f819f 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -93,7 +93,8 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void if (squid_curtime - first_warn > 3 * 60) fatal("SSL servers not responding for 3 minutes"); debugs(34, DBG_IMPORTANT, HERE << "Queue overload, rejecting"); - callback(data, (char *)"error 45 Temporary network problem, please retry later"); + const char *errMsg = "BH error 45 Temporary network problem, please retry later"; // XXX: upgrade to message="" + callback(data, HelperReply(errMsg,strlen(errMsg))); return; } diff --git a/src/tests/stub_helper.cc b/src/tests/stub_helper.cc index d178a0396b..08da5eb577 100644 --- a/src/tests/stub_helper.cc +++ b/src/tests/stub_helper.cc @@ -5,7 +5,7 @@ #include "tests/STUB.h" void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data) STUB -void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver) STUB +void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver) STUB helper::~helper() STUB CBDATA_CLASS_INIT(helper); diff --git a/src/typedefs.h b/src/typedefs.h index 488671adbc..09b1f05fa3 100644 --- a/src/typedefs.h +++ b/src/typedefs.h @@ -75,7 +75,6 @@ typedef void IDCB(const char *ident, void *data); class CachePeer; typedef void IRCB(CachePeer *, peer_t, AnyP::ProtocolType, void *, void *data); -typedef void RH(void *data, char *); /* in wordlist.h */ class wordlist; @@ -90,7 +89,6 @@ class StoreEntry; typedef void OBJH(StoreEntry *); typedef void SIGHDLR(int sig); typedef void STVLDCB(void *, int, int); -typedef void HLPCB(void *, char *buf); typedef int HLPSAVAIL(void *); typedef void HLPSONEQ(void *); typedef void HLPCMDOPTS(int *argc, char **argv);