Dump HTTP request headers #13992

promag commented at 1:59 PM on August 17, 2018: member

No description provided.

in src/httpserver.cpp:231 in dec52a3463 outdated

 223 | @@ -224,9 +224,13 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
 224 |      }
 225 |      std::unique_ptr<HTTPRequest> hreq(new HTTPRequest(req));
 226 |  
 227 | -    LogPrint(BCLog::HTTP, "Received a %s request for %s from %s\n",
 228 | +    LogPrint(BCLog::HTTP, "Received a %s request for %s from %s with headers:\n",
 229 |               RequestMethodString(hreq->GetRequestMethod()), hreq->GetURI(), hreq->GetPeer().ToString());
 230 |  
 231 | +    for (auto i : hreq->GetHeaders()) {
 232 | +        LogPrint(BCLog::HTTP, "  %s: %s\n", i.first, i.second);

promag commented at 1:59 PM on August 17, 2018:

Should redact authorization header?

promag commented at 2:01 PM on August 17, 2018: member

Output example when using bitcoin-cli:

2018-08-17T14:00:38Z Received a POST request for / from 127.0.0.1:61509
2018-08-17T14:00:38Z Request headers:
2018-08-17T14:00:38Z   Authorization: Basic X19jb29raWVfXzo3OGYxMzkzMzVmNmZkNjBhZjA5ZjY3MDcyNmM0ZDE0MDRiOGI2MTlmYjYwNDc3NjExYzZmYTQ1Y2E2ZjAxODZh
2018-08-17T14:00:38Z   Connection: close
2018-08-17T14:00:38Z   Content-Length: 37
2018-08-17T14:00:38Z   Host: 127.0.0.1

Output example when running the test framework:

2018-08-17T14:08:37.622063Z Received a POST request for / from 127.0.0.1:61759
2018-08-17T14:08:37.622063Z Request headers:
2018-08-17T14:08:37.623648Z   Accept-Encoding: identity
2018-08-17T14:08:37.623701Z   Authorization: Basic X19jb29raWVfXzoxMmQyMjMxOTY4YzM5NzRkN2ViMDI0ZjA5YzM1M2Y5ZjBkNDUxNjA1MjQ4NDY4NzY5YmJlOTlmMmYxNjY0NWY3
2018-08-17T14:08:37.623795Z   Content-Length: 68
2018-08-17T14:08:37.623844Z   Content-type: application/json
2018-08-17T14:08:37.623875Z   Host: 127.0.0.1
2018-08-17T14:08:37.623895Z   User-Agent: AuthServiceProxy/0.1

practicalswift commented at 2:04 PM on August 17, 2018: contributor

Concept ACK

Perhaps some sanitization or escaping is needed on the adversary controlled input before printing/logging to make sure we’re not opening up to log injection, etc?

promag force-pushed on Aug 17, 2018

promag commented at 2:29 PM on August 17, 2018: member

@practicalswift Added a commit that only dumps headers if the request is valid (authorized and method is known).

in src/httpserver.cpp:243 in 8d416e43fb outdated

 238 | @@ -239,6 +239,12 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
 239 |          return;
 240 |      }
 241 |  
 242 | +    // Dump headers if request is valid
 243 | +    LogPrint(BCLog::HTTP, "Request headers:\n",

laanwj commented at 2:32 PM on August 17, 2018:

it should avoid doing this if !LogAcceptCategory(BCLog::HTTP), no need for the overhead to copy the headers when the result is ignored

laanwj commented at 2:33 PM on August 17, 2018: member

Concept ACK, this could be useful (though is also very verbose)

agree that string sanitizing is very important to prevent manipulation with escape codes etc.

EDIT dumping the Authorization header as-is is also not a good idea, it leaks the cleartext username/password !

laanwj added the label RPC/REST/ZMQ on Aug 17, 2018

http: Add const modifier to HTTPRequest methods 51f1960531

promag force-pushed on Aug 17, 2018

promag commented at 2:44 PM on August 17, 2018: member

@laanwj right, I asked it above #13992 (review).

laanwj commented at 3:04 PM on August 17, 2018: member

Thinking of it, I'm not sure this is worth the added code and complexity. It's so easy to make tcpdump or wireshark dump this information if you really need it, say for debugging your HTTP implementation.

http: Add utility function EqualHeaders ec39a57a47

http: Add HTTPRequest::GetHeaders 1c130b35f1

http: Dump HTTP request headers 88f1c9d1af

promag force-pushed on Aug 17, 2018

promag commented at 3:28 PM on August 17, 2018: member

Updated to redact authorization header (added EqualHeaders).

Agree with @laanwj and I'm going to close this unless there is a strong reason to keep it.

in src/httpserver.cpp:675 in 88f1c9d1af

 669 | @@ -646,6 +670,11 @@ HTTPRequest::RequestMethod HTTPRequest::GetRequestMethod()
 670 |      }
 671 |  }
 672 |  
 673 | +bool EqualHeaders(const std::string& h1, const std::string& h2)
 674 | +{
 675 | +    return evutil_ascii_strcasecmp(h1.c_str(), h2.c_str()) == 0;

practicalswift commented at 3:36 PM on August 17, 2018:

Is there a reason for relying on libevent in this function? If not I suggest doing this without relying on third-party code by introducing the required locale independent string helper functions in-tree. That would perhaps also allow us to reduce the number of know locale dependence problems listed here: https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-locale-dependence.sh#L4

DrahtBot commented at 8:43 PM on August 17, 2018: member

Note to reviewers: This pull request conflicts with the following ones:

#12274 (http: avoid fd exhaustion by theuni)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag closed this on Aug 19, 2018

promag deleted the branch on Aug 19, 2018

l2a5b1 commented at 10:12 AM on August 19, 2018: contributor

Hey @promag I was looking at this PR when you closed it.

I agree with @laanwj and you that a web debugging proxy is the right tool for inspecting HTTP requests.

If the PR was still open and you where leaning towards implementing it, I would have suggested to extract the dump headers implementation and make it a freestanding utility function.

But more importantly, IMO the changes where non-const member functions have been changed to const member function are useful.

promag commented at 10:45 AM on August 19, 2018: member

Yeah I was going to cherry pick that.

Thanks for reviewing.

promag commented at 12:39 AM on August 20, 2018: member

But more importantly, IMO the changes where non-const member functions have been changed to const member function are useful. @251Labs see #14006.

DrahtBot locked this on Sep 8, 2021