Skip to content

Dump HTTP request headers#13992

Closed
promag wants to merge 4 commits intobitcoin:masterfrom
promag:2017-08-dump-request-headers
Closed

Dump HTTP request headers#13992
promag wants to merge 4 commits intobitcoin:masterfrom
promag:2017-08-dump-request-headers

Conversation

@promag
Copy link
Contributor

@promag promag commented Aug 17, 2018

No description provided.

promag
promag commented Aug 17, 2018
View reviewed changes
src/httpserver.cpp Outdated
Copy link
Contributor Author

@promag promag Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should redact authorization header?

@promag
Copy link
Contributor Author

promag commented Aug 17, 2018 •
edited
Loading

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
Copy link
Contributor

practicalswift commented Aug 17, 2018 •
edited
Loading

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 promag force-pushed the 2017-08-dump-request-headers branch from af855e2 to 8d416e4 Compare August 17, 2018 14:27
@promag
Copy link
Contributor Author

promag commented Aug 17, 2018

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

laanwj
laanwj reviewed Aug 17, 2018
View reviewed changes
src/httpserver.cpp Outdated
Copy link
Member

@laanwj laanwj Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@laanwj
Copy link
Member

laanwj commented Aug 17, 2018 •
edited
Loading

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 !

@promag promag force-pushed the 2017-08-dump-request-headers branch from f0e8c1d to 1a4af64 Compare August 17, 2018 14:36
@promag
Copy link
Contributor Author

promag commented Aug 17, 2018

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

@laanwj
Copy link
Member

laanwj commented Aug 17, 2018

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.

@promag promag force-pushed the 2017-08-dump-request-headers branch from 1a4af64 to 88f1c9d Compare August 17, 2018 15:22
@promag
Copy link
Contributor Author

promag commented Aug 17, 2018

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.

practicalswift
practicalswift reviewed Aug 17, 2018
View reviewed changes
src/httpserver.cpp

bool EqualHeaders(const std::string& h1, const std::string& h2)
{
return evutil_ascii_strcasecmp(h1.c_str(), h2.c_str()) == 0;
Copy link
Contributor

@practicalswift practicalswift Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

DrahtBot commented Aug 17, 2018

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

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 promag closed this Aug 19, 2018
@promag promag deleted the 2017-08-dump-request-headers branch August 19, 2018 09:35
@l2a5b1
Copy link
Contributor

l2a5b1 commented Aug 19, 2018

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
Copy link
Contributor Author

promag commented Aug 19, 2018

Yeah I was going to cherry pick that.

Thanks for reviewing.

@promag
Copy link
Contributor Author

promag commented Aug 20, 2018

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

@251labs see #14006.

@promag promag mentioned this pull request Nov 2, 2018
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@promag @practicalswift @laanwj @DrahtBot @l2a5b1