Audit Review updates

* guarantee that note values output by the HelperReply parser are ""
  nil-terminated string and not undefined String.
* rename HelperReply::responseKeys to 'notes'
* rename Notes::findByName() to find()
* add Note::firstValue() to locate first value provided for a given key
  and present it as a char* terminated string
* various documentation updates

diff --git a/src/HelperReply.cc b/src/HelperReply.cc
index d12499a2e0a879060e6b220e5b66c4c3c5864cab..972d8c8456d8e3507c75208676acf42cd217dc87 100644 (file)
--- a/src/HelperReply.cc
+++ b/src/HelperReply.cc
@@ -54,11 +54,11 @@ HelperReply::parse(char *buf, size_t len)
                 MemBuf authToken;
                 authToken.init();
                 authToken.append(w1, strlen(w1));
-                responseKeys.add("token",authToken.content());
+                notes.add("token",authToken.content());
             } else {
                 // token field is mandatory on this response code
                 result = HelperReply::BrokenHelper;
-                responseKeys.add("message","Missing 'token' data");
+                notes.add("message","Missing 'token' data");
             }
 
         } else if (!strncmp(p,"AF ",3)) {
@@ -75,19 +75,19 @@ HelperReply::parse(char *buf, size_t len)
                 MemBuf authToken;
                 authToken.init();
                 authToken.append(w1, strlen(w1));
-                responseKeys.add("token",authToken.content());
+                notes.add("token",authToken.content());
 
                 MemBuf user;
                 user.init();
                 user.append(w2,strlen(w2));
-                responseKeys.add("user",user.content());
+                notes.add("user",user.content());
 
             } else if (w1 != NULL) {
                 // NTLM "user"
                 MemBuf user;
                 user.init();
                 user.append(w1,strlen(w1));
-                responseKeys.add("user",user.content());
+                notes.add("user",user.content());
             }
         } else if (!strncmp(p,"NA ",3)) {
             // NTLM fail-closed ERR response
@@ -109,7 +109,7 @@ HelperReply::parse(char *buf, size_t len)
 
     // Hack for backward-compatibility: BH used to be a text message...
     if (other().hasContent() && result == HelperReply::BrokenHelper) {
-        responseKeys.add("message",other().content());
+        notes.add("message",other().content());
         modifiableOther().clean();
     }
 }
@@ -139,9 +139,9 @@ HelperReply::parseResponseKeys()
         char *v = strwordtok(NULL, &p);
         if (v != NULL && urlDecode && (p-v) > 2) // 1-octet %-escaped requires 3 bytes
             rfc1738_unescape(v);
-        String value = v;
+        const String value(v?v:""); // value can be empty, but must not be NULL
 
-        responseKeys.add(key, value);
+        notes.add(key, value);
 
         modifiableOther().consume(p - other().content());
         modifiableOther().consumeWhitespacePrefix();
@@ -171,9 +171,9 @@ operator <<(std::ostream &os, const HelperReply &r)
     }
 
     // dump the helper key=pair "notes" list
-    if (r.responseKeys.notes.size() > 0) {
+    if (r.notes.notes.size() > 0) {
         os << ", notes={";
-        for (Notes::NotesList::const_iterator m = r.responseKeys.notes.begin(); m != r.responseKeys.notes.end(); ++m) {
+        for (Notes::NotesList::const_iterator m = r.notes.notes.begin(); m != r.notes.notes.end(); ++m) {
             for (Note::Values::iterator v = (*m)->values.begin(); v != (*m)->values.end(); ++v) {
                 os << ',' << (*m)->key << '=' << ConfigParser::QuoteString((*v)->value);
             }

diff --git a/src/HelperReply.h b/src/HelperReply.h
index 867cff833c3964d5e5a79d47c50b798135b2da11..c67c080e2ab376ec58caf0b04c77885432def52f 100644 (file)
--- a/src/HelperReply.h
+++ b/src/HelperReply.h
@@ -24,7 +24,7 @@ private:
     HelperReply &operator =(const HelperReply &r);
 
 public:
-    HelperReply() : result(HelperReply::Unknown), responseKeys(), whichServer(NULL) {
+    HelperReply() : result(HelperReply::Unknown), notes(), whichServer(NULL) {
         other_.init(1,1);
         other_.terminate();
     }
@@ -42,8 +42,8 @@ public:
     MemBuf &modifiableOther() const { return *const_cast<MemBuf*>(&other_); }
 
     /** parse a helper response line format:
-     *   line     := [ result ] *#( key-pair )
-     *   key-pair := OWS token '=' ( quoted-string | token )
+     *   line     := [ result ] *#( kv-pair )
+     *   kv-pair := OWS token '=' ( quoted-string | token )
      *
      * token are URL-decoded.
      * quoted-string are \-escape decoded and the quotes are stripped.
@@ -60,12 +60,12 @@ public:
         BrokenHelper, // "BH" indicating failure due to helper internal problems.
 
         // result codes for backward compatibility with NTLM/Negotiate
-        // TODO: migrate to a variant of the above results with key-pair parameters
+        // TODO: migrate to a variant of the above results with kv-pair parameters
         TT
     } result;
 
     // list of key=value pairs the helper produced
-    Notes responseKeys;
+    Notes notes;
 
     /// for stateful replies the responding helper 'server' needs to be preserved across callbacks
     CbcPointer<helper_stateful_server> whichServer;

diff --git a/src/Notes.cc b/src/Notes.cc
index 783f42fbf1357bf73593722b47e53458234f3f1b..2060528c4824b8dd956c255f8128579389858166 100644 (file)
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -73,7 +73,7 @@ Note::match(HttpRequest *request, HttpReply *reply)
 }
 
 Note::Pointer
-Notes::findByName(const String &noteKey) const
+Notes::find(const String &noteKey) const
 {
     typedef Notes::NotesList::const_iterator AMLI;
     for (AMLI i = notes.begin(); i != notes.end(); ++i) {
@@ -94,7 +94,7 @@ Notes::add(const String &noteKey, const String &noteValue)
 Note::Pointer
 Notes::add(const String &noteKey)
 {
-    Note::Pointer note = findByName(noteKey);
+    Note::Pointer note = find(noteKey);
     if (note == NULL) {
         note = new Note(noteKey);
         notes.push_back(note);

diff --git a/src/Notes.h b/src/Notes.h
index c93ce4ac94d2b963a57643cc5cb065047de8b851..abf105532c85ef85538ecfcf9d496903e41816a7 100644 (file)
--- a/src/Notes.h
+++ b/src/Notes.h
@@ -48,6 +48,12 @@ public:
      * or NULL if none matches.
      */
     const char *match(HttpRequest *request, HttpReply *reply);
+
+    /**
+     * Returns the first value for this key or an empty string.
+     */
+    const char *firstValue() const { return (values.size()>0&&values[0]->value.defined()?values[0]->value.termedBuf():""); }
+
     String key; ///< The note key
     Values values; ///< The possible values list for the note
 };
@@ -84,9 +90,21 @@ public:
     /// return true if the notes list is empty
     bool empty() { return notes.empty(); }
 
+    /**
+     * Adds a note key and value to the notes list.
+     * If the key name already exists in list, add the given value to its set of values.
+     */
+    void add(const String &noteKey, const String &noteValue);
+
+    /**
+     * Returns a pointer to an existing Note with given key name or nil.
+     */
+    Note::Pointer find(const String &noteKey) const;
+
     NotesList notes; ///< The Note::Pointer objects array list
     const char *descr; ///< A short description for notes list
     const char **blacklisted; ///< Null terminated list of blacklisted note keys
+
 private:
     /**
      * Adds a note to the notes list and returns a pointer to the
@@ -95,17 +113,6 @@ private:
      */
     Note::Pointer add(const String &noteKey);
 
-public:
-    /**
-     * Adds a note key and value to the notes list.
-     * If the key name already exists in list, add the given value to its set of values.
-     */
-    void add(const String &noteKey, const String &noteValue);
-
-    /**
-     * Looks up a note by key name and returns a Note::Pointer to it
-     */
-    Note::Pointer findByName(const String &noteKey) const;
 };
 
 class NotePairs : public HttpHeader

diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc
index ea4c3e2df3c8f67d4556ce6d2f3b6140e39acbb3..355cc47f9ca6d6d06bf704765d888bd8e539e828 100644 (file)
--- a/src/auth/digest/UserRequest.cc
+++ b/src/auth/digest/UserRequest.cc
@@ -304,9 +304,9 @@ Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply)
         Auth::Digest::User *digest_user = dynamic_cast<Auth::Digest::User *>(auth_user_request->user().getRaw());
         assert(digest_user != NULL);
 
-        Note::Pointer ha1Note = reply.responseKeys.findByName("ha1");
+        Note::Pointer ha1Note = reply.notes.find("ha1");
         if (ha1Note != NULL) {
-            CvtBin(ha1Note->values[0]->value.termedBuf(), digest_user->HA1);
+            CvtBin(ha1Note->firstValue(), digest_user->HA1);
             digest_user->HA1created = 1;
         } else {
             debugs(29, DBG_IMPORTANT, "ERROR: Digest auth helper did not produce a HA1. Using the wrong helper program? received: " << reply);
@@ -330,9 +330,9 @@ Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply)
         digest_request->user()->credentials(Auth::Failed);
         digest_request->flags.invalid_password = 1;
 
-        Note::Pointer msgNote = reply.responseKeys.findByName("message");
+        Note::Pointer msgNote = reply.notes.find("message");
         if (msgNote != NULL) {
-            digest_request->setDenyMessage(msgNote->values[0]->value.termedBuf());
+            digest_request->setDenyMessage(msgNote->firstValue());
         } else if (reply.other().hasContent()) {
             // old helpers did send ERR result but a bare message string instead of message= key name.
             digest_request->setDenyMessage(reply.other().content());

diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc
index bf03ce21c19622e6c1ef68d9b3c1c5a948089f08..0f4a9566382e39549fcb19503a532f20c0c40b74 100644 (file)
--- a/src/auth/negotiate/UserRequest.cc
+++ b/src/auth/negotiate/UserRequest.cc
@@ -268,11 +268,11 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
         safe_free(lm_request->server_blob);
         lm_request->request->flags.mustKeepalive = 1;
         if (lm_request->request->flags.proxyKeepalive) {
-            Note::Pointer tokenNote = reply.responseKeys.findByName("token");
-            lm_request->server_blob = xstrdup(tokenNote->values[0]->value.termedBuf());
+            Note::Pointer tokenNote = reply.notes.find("token");
+            lm_request->server_blob = xstrdup(tokenNote->firstValue());
             auth_user_request->user()->credentials(Auth::Handshake);
             auth_user_request->denyMessage("Authentication in progress");
-            debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << tokenNote->values[0]->value << "'");
+            debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << tokenNote->firstValue() << "'");
         } else {
             auth_user_request->user()->credentials(Auth::Failed);
             auth_user_request->denyMessage("Negotiate authentication requires a persistent connection");
@@ -280,8 +280,8 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
         break;
 
     case HelperReply::Okay: {
-        Note::Pointer userNote = reply.responseKeys.findByName("user");
-        Note::Pointer tokenNote = reply.responseKeys.findByName("token");
+        Note::Pointer userNote = reply.notes.find("user");
+        Note::Pointer tokenNote = reply.notes.find("token");
         if (userNote == NULL || tokenNote == NULL) {
             // XXX: handle a success with no username better
             /* protocol error */
@@ -290,10 +290,10 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
         }
 
         /* we're finished, release the helper */
-        auth_user_request->user()->username(userNote->values[0]->value.termedBuf());
+        auth_user_request->user()->username(userNote->firstValue());
         auth_user_request->denyMessage("Login successful");
         safe_free(lm_request->server_blob);
-        lm_request->server_blob = xstrdup(tokenNote->values[0]->value.termedBuf());
+        lm_request->server_blob = xstrdup(tokenNote->firstValue());
         lm_request->releaseAuthServer();
 
         /* connection is authenticated */
@@ -327,8 +327,8 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
     break;
 
     case HelperReply::Error: {
-        Note::Pointer messageNote = reply.responseKeys.findByName("message");
-        Note::Pointer tokenNote = reply.responseKeys.findByName("token");
+        Note::Pointer messageNote = reply.notes.find("message");
+        Note::Pointer tokenNote = reply.notes.find("token");
         if (tokenNote == NULL) {
             /* protocol error */
             fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply.other().content());
@@ -336,10 +336,10 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
         }
 
         /* authentication failure (wrong password, etc.) */
-        auth_user_request->denyMessage(messageNote->values[0]->value.termedBuf());
+        auth_user_request->denyMessage(messageNote->firstValue());
         auth_user_request->user()->credentials(Auth::Failed);
         safe_free(lm_request->server_blob);
-        lm_request->server_blob = xstrdup(tokenNote->values[0]->value.termedBuf());
+        lm_request->server_blob = xstrdup(tokenNote->firstValue());
         lm_request->releaseAuthServer();
         debugs(29, 4, HERE << "Failed validating user via Negotiate. Error returned '" << reply << "'");
     }
@@ -355,11 +355,11 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply)
          * Authenticate Negotiate start.
          * If after a KK deny the user's request w/ 407 and mark the helper as
          * Needing YR. */
-        Note::Pointer errNote = reply.responseKeys.findByName("message");
+        Note::Pointer errNote = reply.notes.find("message");
         if (reply.result == HelperReply::Unknown)
             auth_user_request->denyMessage("Internal Error");
         else if (errNote != NULL)
-            auth_user_request->denyMessage(errNote->values[0]->value.termedBuf());
+            auth_user_request->denyMessage(errNote->firstValue());
         else
             auth_user_request->denyMessage("Negotiate Authentication failed with no reason given");
         auth_user_request->user()->credentials(Auth::Failed);

diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc
index 0c9cd97695741446028390881eb607f69fb62efe..a11b9abd8ce1742d2cda56c4e0afbd0ce728a4ce 100644 (file)
--- a/src/auth/ntlm/UserRequest.cc
+++ b/src/auth/ntlm/UserRequest.cc
@@ -261,11 +261,11 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply)
         safe_free(lm_request->server_blob);
         lm_request->request->flags.mustKeepalive = 1;
         if (lm_request->request->flags.proxyKeepalive) {
-            Note::Pointer serverBlob = reply.responseKeys.findByName("token");
-            lm_request->server_blob = xstrdup(serverBlob->values[0]->value.termedBuf());
+            Note::Pointer serverBlob = reply.notes.find("token");
+            lm_request->server_blob = xstrdup(serverBlob->firstValue());
             auth_user_request->user()->credentials(Auth::Handshake);
             auth_user_request->denyMessage("Authentication in progress");
-            debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << serverBlob->values[0]->value << "'");
+            debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << serverBlob->firstValue() << "'");
         } else {
             auth_user_request->user()->credentials(Auth::Failed);
             auth_user_request->denyMessage("NTLM authentication requires a persistent connection");
@@ -274,13 +274,13 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply)
 
     case HelperReply::Okay: {
         /* we're finished, release the helper */
-        Note::Pointer userLabel = reply.responseKeys.findByName("user");
-        auth_user_request->user()->username(userLabel->values[0]->value.termedBuf());
+        Note::Pointer userLabel = reply.notes.find("user");
+        auth_user_request->user()->username(userLabel->firstValue());
         auth_user_request->denyMessage("Login successful");
         safe_free(lm_request->server_blob);
         lm_request->releaseAuthServer();
 
-        debugs(29, 4, HERE << "Successfully validated user via NTLM. Username '" << userLabel->values[0]->value << "'");
+        debugs(29, 4, HERE << "Successfully validated user via NTLM. Username '" << userLabel->firstValue() << "'");
         /* connection is authenticated */
         debugs(29, 4, HERE << "authenticated user " << auth_user_request->user()->username());
         /* see if this is an existing user with a different proxy_auth
@@ -313,15 +313,15 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply)
 
     case HelperReply::Error: {
         /* authentication failure (wrong password, etc.) */
-        Note::Pointer errNote = reply.responseKeys.findByName("message");
+        Note::Pointer errNote = reply.notes.find("message");
         if (errNote != NULL)
-            auth_user_request->denyMessage(errNote->values[0]->value.termedBuf());
+            auth_user_request->denyMessage(errNote->firstValue());
         else
             auth_user_request->denyMessage("NTLM Authentication denied with no reason given");
         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 '" << errNote->values[0]->value << "'");
+        debugs(29, 4, HERE << "Failed validating user via NTLM. Error returned '" << errNote->firstValue() << "'");
     }
     break;
 
@@ -335,11 +335,11 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply)
          * Authenticate NTLM start.
          * If after a KK deny the user's request w/ 407 and mark the helper as
          * Needing YR. */
-        Note::Pointer errNote = reply.responseKeys.findByName("message");
+        Note::Pointer errNote = reply.notes.find("message");
         if (reply.result == HelperReply::Unknown)
             auth_user_request->denyMessage("Internal Error");
         else if (errNote != NULL)
-            auth_user_request->denyMessage(errNote->values[0]->value.termedBuf());
+            auth_user_request->denyMessage(errNote->firstValue());
         else
             auth_user_request->denyMessage("NTLM Authentication failed with no reason given");
         auth_user_request->user()->credentials(Auth::Failed);

diff --git a/src/dns.cc b/src/dns.cc
index 5c51a5343f8c9e0ba48832a9e4a046e0c2102d8d..c0f9f159274ad38f4297ccd5d032bc2257b8bf68 100644 (file)
--- a/src/dns.cc
+++ b/src/dns.cc
@@ -134,8 +134,8 @@ dnsSubmit(const char *lookup, HLPCB * callback, void *data)
         HelperReply failReply;
         /* XXX: upgrade the ipcache and fqdn cache handlers to new syntax
         failReply.result= HelperReply::BrokenHelper;
-        failReply.responseKeys.add("message","Temporary network problem, please retry later");
-        failReply.responseKeys.add("message","DNS lookup queue overloaded");
+        failReply.notes.add("message","Temporary network problem, please retry later");
+        failReply.notes.add("message","DNS lookup queue overloaded");
         */
         failReply.modifiableOther().append(t, strlen(t));
         callback(data, failReply);

diff --git a/src/external_acl.cc b/src/external_acl.cc
index a7d5640c3159b4374f7d196bb496eb8dd82f0452..c0c46bb973180951d4d419e6bdea61ecf8d90375 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -1326,26 +1326,26 @@ externalAclHandleReply(void *data, const HelperReply &reply)
         entryData.result = ACCESS_ALLOWED;
     // XXX: handle other non-DENIED results better
 
-    // XXX: make entryData store a proper helperReply object.
+    // XXX: make entryData store a proper HelperReply object instead of copying.
 
-    Note::Pointer label = reply.responseKeys.findByName("tag");
+    Note::Pointer label = reply.notes.find("tag");
     if (label != NULL && label->values[0]->value.size() > 0)
         entryData.tag = label->values[0]->value;
 
-    label = reply.responseKeys.findByName("message");
+    label = reply.notes.find("message");
     if (label != NULL && label->values[0]->value.size() > 0)
         entryData.message = label->values[0]->value;
 
-    label = reply.responseKeys.findByName("log");
+    label = reply.notes.find("log");
     if (label != NULL && label->values[0]->value.size() > 0)
         entryData.log = label->values[0]->value;
 
 #if USE_AUTH
-    label = reply.responseKeys.findByName("user");
+    label = reply.notes.find("user");
     if (label != NULL && label->values[0]->value.size() > 0)
         entryData.user = label->values[0]->value;
 
-    label = reply.responseKeys.findByName("password");
+    label = reply.notes.find("password");
     if (label != NULL && label->values[0]->value.size() > 0)
         entryData.password = label->values[0]->value;
 #endif

diff --git a/src/redirect.cc b/src/redirect.cc
index a0eab3a05748733d8c76d0979c64086b142493ca..3d176852e2bc5771d7e1fc9109be398ac67a8e4d 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -79,7 +79,7 @@ redirectHandleReply(void *data, const HelperReply &reply)
     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
+    // it can be removed when the helpers are all updated to the normalized "OK/ERR kv-pairs" format
 
     if (reply.result == HelperReply::Unknown) {
         // BACKWARD COMPATIBILITY 2012-06-15:
@@ -153,7 +153,7 @@ redirectStart(ClientHttpRequest * http, HLPCB * handler, void *data)
         ++n_bypassed;
         HelperReply bypassReply;
         bypassReply.result = HelperReply::Okay;
-        bypassReply.responseKeys.add("message","URL rewrite/redirect queue too long. Bypassed.");
+        bypassReply.notes.add("message","URL rewrite/redirect queue too long. Bypassed.");
         handler(data, bypassReply);
         return;
     }

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index bf1153e0930794a403f9965c8d7148f8a8ee5265..068ddae560f1a50ad82a31b5a433de4ab55286b1 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -95,7 +95,7 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void
         debugs(34, DBG_IMPORTANT, HERE << "Queue overload, rejecting");
         HelperReply failReply;
         failReply.result = HelperReply::BrokenHelper;
-        failReply.responseKeys.add("message", "error 45 Temporary network problem, please retry later");
+        failReply.notes.add("message", "error 45 Temporary network problem, please retry later");
         callback(data, failReply);
         return;
     }