Updated patch attached for audit with changes as discussed below.
On 27/10/2012 5:33 a.m., Alex Rousskov wrote:
On 10/26/2012 04:23 AM, Amos Jeffries wrote:
Updated patch attached for review.
This seems to add a typo bug:
+ } else if (!strncmp(p,"ERR",3) && (p[3] == ' ' || p[3] == '\n' || p[2]
== '\0')) {
s/p[2]/p[3]/?
Oops. Removed as below...
"buf" is guaranteed to be 0-terminated, right?
In a way yes. The EOL delimiter (whatever it is) gets replaced with \0
in helperHandleRead() before being passed to the HelperReply parser. But
there are helpers like ssl_crtd or pinger which may send back tokens
containing \0 so it is not dependable null-termination so much as a
safety backup for str*() functions being used. The "len" parameter tells
us far better where the end of line actually is...
Which brings to mind a far better solution. Thank you for making me
re-think this so much.
Since the tokens we are looking for are followed by SP or the only thing
on the line we can do this without worrying what EOL might be:
"
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] == ' ')) {
"
Should we be worried about \r before \n? Perhaps on Windows?
The whole \r\n is treated as \n and replaced by helperHandleRead()
before getting here. In all other cases \r might be part of some blob we
should not be matching.
+ // check we have something to parse
+ if (!buf || len < 1)
+ return;
...
+ if (len >= 2) {
Would not all the code work if len is zero or one? If yes, remove the
len checks, especially if len is going to be more than 1 in 99.9999% of
cases.
URL-rewriter which are unfortunatley common still return empty response
as their most common case.
The cases where >= 2 will be closer to 50% of overall helper responses
IMO. Unti that is gone this is an optimization to prevent multiple
strncmp() being run on 0-1 length strings.
Understood. Please add a comment explaining this. For example,
// optimization: large portion of [URL-rewriter] responses are a single
// character (that we will stuff into other_).
if (len >= 2) ...
However, it feels wrong that we do not check what that character is (or
does the URL rewriting code do that?).
It does no specific checks AFAICT. But the case needs to be written
through anyway to retain the old behaviour on those responses - even if
it was breaking.
Amos
=== modified file 'src/ClientRequestContext.h'
--- src/ClientRequestContext.h 2012-08-14 11:53:07 +0000
+++ src/ClientRequestContext.h 2012-10-26 00:07:58 +0000
@@ -14,6 +14,8 @@
class DnsLookupDetails;
class ErrorState;
+class HelperReply;
+
class ClientRequestContext : public RefCountable
{
@@ -32,7 +34,7 @@
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
=== added file 'src/HelperReply.cc'
--- src/HelperReply.cc 1970-01-01 00:00:00 +0000
+++ src/HelperReply.cc 2012-10-27 11:24:59 +0000
@@ -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;
+}
=== added file 'src/HelperReply.h'
--- src/HelperReply.h 1970-01-01 00:00:00 +0000
+++ src/HelperReply.h 2012-10-26 06:59:06 +0000
@@ -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 <ostream>
+#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<MemBuf*>(&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<helper_stateful_server> whichServer;
+
+private:
+ /// the remainder of the line
+ MemBuf other_;
+};
+
+std::ostream &operator <<(std::ostream &os, const HelperReply &r);
+
+#endif /* _SQUID_SRC_HELPERREPLY_H */
=== modified file 'src/Makefile.am'
--- src/Makefile.am 2012-10-26 19:42:31 +0000
+++ src/Makefile.am 2012-10-27 06:13:39 +0000
@@ -364,6 +364,8 @@
helper.h \
HelperChildConfig.h \
HelperChildConfig.cc \
+ HelperReply.cc \
+ HelperReply.h \
hier_code.h \
HierarchyLogEntry.h \
$(HTCPSOURCE) \
@@ -1452,6 +1454,8 @@
helper.cc \
HelperChildConfig.h \
HelperChildConfig.cc \
+ HelperReply.cc \
+ HelperReply.h \
$(HTCPSOURCE) \
HttpStateFlags.h \
http.cc \
@@ -1858,6 +1862,8 @@
helper.cc \
HelperChildConfig.h \
HelperChildConfig.cc \
+ HelperReply.cc \
+ HelperReply.h \
hier_code.h \
$(HTCPSOURCE) \
http.cc \
@@ -2103,6 +2109,8 @@
helper.cc \
HelperChildConfig.h \
HelperChildConfig.cc \
+ HelperReply.cc \
+ HelperReply.h \
hier_code.h \
$(HTCPSOURCE) \
http.cc \
@@ -2345,6 +2353,8 @@
helper.cc \
HelperChildConfig.h \
HelperChildConfig.cc \
+ HelperReply.cc \
+ HelperReply.h \
hier_code.h \
$(HTCPSOURCE) \
http.cc \
@@ -2637,6 +2647,8 @@
helper.cc \
HelperChildConfig.h \
HelperChildConfig.cc \
+ HelperReply.cc \
+ HelperReply.h \
hier_code.h \
$(HTCPSOURCE) \
http.cc \
@@ -3724,6 +3736,8 @@
helper.cc \
HelperChildConfig.h \
HelperChildConfig.cc \
+ HelperReply.cc \
+ HelperReply.h \
hier_code.h \
$(HTCPSOURCE) \
http.cc \
=== modified file 'src/SquidDns.h'
--- src/SquidDns.h 2012-09-21 14:57:30 +0000
+++ src/SquidDns.h 2012-10-26 00:07:58 +0000
@@ -1,6 +1,10 @@
#ifndef SQUID_DNS_H
#define SQUID_DNS_H
+#if USE_DNSHELPER
+#include "helper.h"
+#endif
+
namespace Ip
{
class Address;
=== modified file 'src/auth/State.h'
--- src/auth/State.h 2012-06-19 23:16:13 +0000
+++ src/auth/State.h 2012-10-26 00:07:58 +0000
@@ -5,6 +5,7 @@
#include "auth/UserRequest.h"
#include "cbdata.h"
+#include "helper.h"
namespace Auth
{
=== modified file 'src/auth/UserRequest.h'
--- src/auth/UserRequest.h 2012-09-21 14:57:30 +0000
+++ src/auth/UserRequest.h 2012-10-26 00:07:58 +0000
@@ -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;
=== modified file 'src/auth/basic/UserRequest.cc'
--- src/auth/basic/UserRequest.cc 2012-08-14 11:53:07 +0000
+++ src/auth/basic/UserRequest.cc 2012-10-26 00:07:58 +0000
@@ -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 @@
}
void
-Auth::Basic::UserRequest::HandleReply(void *data, char *reply)
+Auth::Basic::UserRequest::HandleReply(void *data, const HelperReply &reply)
{
Auth::StateData *r = static_cast<Auth::StateData *>(data);
BasicAuthQueueNode *tmpnode;
- char *t = NULL;
void *cbdata;
- debugs(29, 5, HERE << "{" << (reply ? reply : "<NULL>") << "}");
-
- 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 @@
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;
=== modified file 'src/auth/digest/UserRequest.cc'
--- src/auth/digest/UserRequest.cc 2012-10-26 11:36:45 +0000
+++ src/auth/digest/UserRequest.cc 2012-10-27 06:13:39 +0000
@@ -272,27 +272,18 @@
}
void
-Auth::Digest::UserRequest::HandleReply(void *data, char *reply)
+Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply)
{
Auth::StateData *replyData = static_cast<Auth::StateData *>(data);
- char *t = NULL;
- void *cbdata;
- debugs(29, 9, HERE << "{" << (reply ? reply : "<NULL>") << "}");
-
- 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::Digest::UserRequest *>(auth_user_request.getRaw());
assert(digest_request);
@@ -300,17 +291,28 @@
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::Digest::User
*>(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);
=== modified file 'src/auth/negotiate/UserRequest.cc'
--- src/auth/negotiate/UserRequest.cc 2012-09-19 17:16:56 +0000
+++ src/auth/negotiate/UserRequest.cc 2012-10-26 10:18:53 +0000
@@ -232,25 +232,18 @@
}
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<Auth::StateData *>(data);
- char *blob, *arg = NULL;
-
- debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply
? reply : "<NULL>") << "'");
+ 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 @@
assert(auth_user_request->user()->auth_type == Auth::AUTH_NEGOTIATE);
if (lm_request->authserver == NULL)
- lm_request->authserver =
static_cast<helper_stateful_server*>(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;
+ 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;
- if (strncasecmp(reply, "TT ", 3) == 0) {
+ switch(reply.result)
+ {
+ case HelperReply::TT:
/* we have been given a blob to send to the client */
- if (arg) {
- *arg = '\0';
- ++arg;
- }
safe_free(lm_request->server_blob);
lm_request->request->flags.mustKeepalive = 1;
if (lm_request->request->flags.proxyKeepalive) {
@@ -296,14 +289,19 @@
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) {
+ break;
+
+ 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 */
-
- if (arg) {
- *arg = '\0';
- ++arg;
- }
-
auth_user_request->user()->username(arg);
auth_user_request->denyMessage("Login successful");
safe_free(lm_request->server_blob);
@@ -336,23 +334,32 @@
* 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 << "'");
+ debugs(29, 4, HERE << "Successfully validated user via Negotiate.
Username '" << arg << "'");
+ }
+ break;
- } else if (strncasecmp(reply, "NA ", 3) == 0 && arg != NULL) {
+ 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.) */
-
- if (arg) {
- *arg = '\0';
- ++arg;
- }
-
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_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;
=== modified file 'src/auth/negotiate/UserRequest.h'
--- src/auth/negotiate/UserRequest.h 2012-06-19 23:16:13 +0000
+++ src/auth/negotiate/UserRequest.h 2012-10-26 00:07:58 +0000
@@ -52,7 +52,7 @@
HttpRequest *request;
private:
- static HLPSCB HandleReply;
+ static HLPCB HandleReply;
};
} // namespace Negotiate
=== modified file 'src/auth/ntlm/UserRequest.cc'
--- src/auth/ntlm/UserRequest.cc 2012-09-19 17:16:56 +0000
+++ src/auth/ntlm/UserRequest.cc 2012-10-26 10:20:45 +0000
@@ -225,24 +225,18 @@
}
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<Auth::StateData *>(data);
- char *blob;
- debugs(29, 8, HERE << "helper: '" << lastserver << "' sent us '" << (reply
? reply : "<NULL>") << "'");
+ 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 @@
assert(auth_user_request->user()->auth_type == Auth::AUTH_NTLM);
if (lm_request->authserver == NULL)
- lm_request->authserver =
static_cast<helper_stateful_server*>(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_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 @@
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 @@
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) {
=== modified file 'src/auth/ntlm/UserRequest.h'
--- src/auth/ntlm/UserRequest.h 2012-06-19 23:16:13 +0000
+++ src/auth/ntlm/UserRequest.h 2012-10-26 00:07:58 +0000
@@ -48,7 +48,7 @@
HttpRequest *request;
private:
- static HLPSCB HandleReply;
+ static HLPCB HandleReply;
};
} // namespace Ntlm
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2012-10-26 19:42:31 +0000
+++ src/client_side.cc 2012-10-27 06:13:39 +0000
@@ -3689,23 +3689,23 @@
}
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 <NULL>
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");
=== modified file 'src/client_side.h'
--- src/client_side.h 2012-10-04 00:23:44 +0000
+++ src/client_side.h 2012-10-26 00:07:58 +0000
@@ -53,6 +53,7 @@
class ClientHttpRequest;
class clientStreamNode;
class ChunkedCodingParser;
+class HelperReply;
namespace AnyP
{
class PortCfg;
@@ -342,9 +343,9 @@
*/
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_; }
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2012-10-26 11:36:45 +0000
+++ src/client_side_request.cc 2012-10-27 06:13:39 +0000
@@ -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 @@
#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 @@
if (answer == ACCESS_ALLOWED)
redirectStart(http, clientRedirectDoneWrapper, context);
else
- context->clientRedirectDone(NULL);
+ context->clientRedirectDone(HelperReply(NULL,0));
}
void
@@ -1181,7 +1183,7 @@
}
void
-clientRedirectDoneWrapper(void *data, char *result)
+clientRedirectDoneWrapper(void *data, const HelperReply &result)
{
ClientRequestContext *calloutContext = (ClientRequestContext *)data;
@@ -1192,14 +1194,19 @@
}
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<char*>(reply.other().content());
http_status status = (http_status) atoi(result);
if (status == HTTP_MOVED_PERMANENTLY
@@ -1207,7 +1214,7 @@
|| 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;
=== modified file 'src/client_side_request.h'
--- src/client_side_request.h 2012-09-25 16:38:36 +0000
+++ src/client_side_request.h 2012-10-26 00:07:58 +0000
@@ -206,7 +206,6 @@
void clientAccessCheck(ClientHttpRequest *);
/* ones that should be elsewhere */
-void redirectStart(ClientHttpRequest *, RH *, void *);
void tunnelStart(ClientHttpRequest *, int64_t *, int *);
#if _USE_INLINE_
=== modified file 'src/dns.cc'
--- src/dns.cc 2012-09-05 09:41:45 +0000
+++ src/dns.cc 2012-10-26 00:07:58 +0000
@@ -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 @@
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;
}
=== modified file 'src/external_acl.cc'
--- src/external_acl.cc 2012-09-04 09:10:20 +0000
+++ src/external_acl.cc 2012-10-26 00:07:58 +0000
@@ -1306,30 +1306,28 @@
* 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<externalAclState *>(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 << "\"");
-
- if (reply) {
- status = strwordtok(reply, &t);
-
- if (status && strcmp(status, "OK") == 0)
- entryData.result = ACCESS_ALLOWED;
+ debugs(82, 2, HERE << "reply=" << reply);
+
+ if (reply.result == HelperReply::Okay)
+ entryData.result = ACCESS_ALLOWED;
+ // XXX: handle other non-DENIED results better
+
+ 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 @@
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);
=== modified file 'src/fqdncache.cc'
--- src/fqdncache.cc 2012-09-04 09:10:20 +0000
+++ src/fqdncache.cc 2012-10-26 00:07:58 +0000
@@ -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 @@
*/
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 @@
statCounter.dns.svcTime.count(age);
#if USE_DNSHELPER
- fqdncacheParse(f, reply);
+ fqdncacheParse(f, reply.other().content());
#else
fqdncacheParse(f, answers, na, error_message);
=== modified file 'src/helper.cc'
--- src/helper.cc 2012-10-08 05:21:11 +0000
+++ src/helper.cc 2012-10-26 00:07:58 +0000
@@ -398,7 +398,7 @@
{
if (hlp == NULL) {
debugs(84, 3, "helperSubmit: hlp == NULL");
- callback(data, NULL);
+ callback(data, HelperReply(NULL,0));
return;
}
@@ -419,11 +419,11 @@
/// 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 @@
}
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 @@
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 @@
}
/// 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 @@
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 @@
*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 @@
/* 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);
=== modified file 'src/helper.h'
--- src/helper.h 2012-10-03 17:32:57 +0000
+++ src/helper.h 2012-10-26 00:07:58 +0000
@@ -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 @@
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 @@
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);
=== modified file 'src/ipcache.cc'
--- src/ipcache.cc 2012-10-10 00:57:33 +0000
+++ src/ipcache.cc 2012-10-26 00:07:58 +0000
@@ -595,7 +595,7 @@
/// \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 @@
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);
=== modified file 'src/redirect.cc'
--- src/redirect.cc 2012-09-04 09:10:20 +0000
+++ src/redirect.cc 2012-10-26 00:07:58 +0000
@@ -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 @@
Ip::Address client_addr;
const char *client_ident;
const char *method_s;
- RH *handler;
+ HLPCB *handler;
} redirectStateData;
static HLPCB redirectHandleReply;
@@ -74,21 +73,36 @@
CBDATA_TYPE(redirectStateData);
static void
-redirectHandleReply(void *data, char *reply)
+redirectHandleReply(void *data, const HelperReply &reply)
{
redirectStateData *r = static_cast<redirectStateData *>(data);
- char *t;
+ 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;
- debugs(61, 5, "redirectHandleRead: {" << (reply && *reply != '\0' ? reply
: "<NULL>") << "}");
-
- if (reply) {
- if ((t = strchr(reply, ' ')))
- *t = '\0';
-
- if (*reply == '\0')
- reply = NULL;
- }
-
if (cbdataReferenceValidDone(r->data, &cbdata))
r->handler(cbdata, reply);
@@ -120,7 +134,7 @@
/**** 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 @@
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;
}
=== modified file 'src/redirect.h'
--- src/redirect.h 2012-09-21 14:57:30 +0000
+++ src/redirect.h 2012-10-26 00:07:58 +0000
@@ -33,7 +33,12 @@
*
*/
+#include "helper.h"
+
+class ClientHttpRequest;
+
void redirectInit(void);
void redirectShutdown(void);
+void redirectStart(ClientHttpRequest *, HLPCB *, void *);
#endif /* SQUID_REDIRECT_H_ */
=== modified file 'src/ssl/helper.cc'
--- src/ssl/helper.cc 2012-09-04 09:10:20 +0000
+++ src/ssl/helper.cc 2012-10-26 00:07:58 +0000
@@ -93,7 +93,8 @@
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;
}
=== modified file 'src/tests/stub_helper.cc'
--- src/tests/stub_helper.cc 2012-01-20 18:55:04 +0000
+++ src/tests/stub_helper.cc 2012-10-26 00:07:58 +0000
@@ -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);
=== modified file 'src/typedefs.h'
--- src/typedefs.h 2012-09-23 10:30:46 +0000
+++ src/typedefs.h 2012-10-26 00:07:58 +0000
@@ -75,7 +75,6 @@
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 @@
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);
