HTTP Proxy : see #507

[guzzijones]

By submitting this pull-request, I confirm the following:

• I have read and understood the Contributing Guide.
• I have checked that another pull-request for this purpose does not exist.
• I have considered and confirmed that this submission will be valuable to others.
• I accept that this submission may not be used and that this pull-request may be closed by the will of the maintainer.
• I give this submission freely under the BSD 3-clause license.

Place an X inside the bracket (or click the box in preview) to confirm

• [x ] I confirm.

---

Enter your pull-request summary here

[guzzijones]

This must have been an artifact of git rebase -i

[anonimal]

Fix it, rebase, force push.

[anonimal]

There are too many issues to post in one review, so we'll have to go step-by-step. Fortunately, the key problems are fixed: user-agent and POST, so a huge thank you @guzzijones.

Note, you didn't have to close the previous PR: you can just force push the branch with --force (see man git-push). Please do that for the remainder of the work for this PR.

Now that you've also solved the learning curve of rebasing, we'll start with more of the basics:

1. run clang-format on all effected files as described in the style guide. If clang-format does something completely stupid, fix it
2. run cpplint (you can ignore warnings about any const references for now)
3. style refactor local variables (we don't use camelCase or HeaderBody for local variables)
4. Name variables as appropriate / use their intended purpose. This is specifically noted in our (and google's style guide). Variables like tbuffer tmpBuf bufString are, in this context, poorly named not because of the choice of words but because they are not tying into their intention (also, no camelCase). buf is fine to use if it's localized and unmistakable the only buffer we are operating on but the others need clarification (example, bufString implies there's another type of buffer).

These are the absolute easiest and most trivial part of development. If you can learn to organize your thoughts more visually, with an emphasis on clear composition, I believe that many other aspects of development will fall into place - and you will catch all the other non-style issues (and some dangerous style issues) that I will be pointing out after you've fixed these first 4 points.

For anyone else reading this: guzzi and I do most of our dialogue on IRC so if this response sounds harsh or pedantic, it's because @guzzijones has asked me to do so [respond in such a way] 😄

Referencing #527

[guzzijones]

I made changes to variables. I had ran cpplint on files again. I did a force push. Let me know. I am not sure why it is showing merge conflicts now as i don't believe it was before..

[anonimal]

You'll need to rebase against current master.

[guzzijones]

I will adjust the logging to the new system. That seems to be the reason for the merge conflicts.

[anonimal]

Yes, like I said, just rebase against current master.

[guzzijones]

I finished up the rebase against the current master.

[anonimal]

Now that you've finished rebasing, I must point out again that you didn't run clang-format on either the source or header file. That's fine, there are much bigger issues to deal with.

I don't want to spend an hour commenting on every issue, nor do I think I should patch this, so let's take the remaining issues step-by-step. First: notice the bottom of the build log https://build.getmonero.org/builders/kovri-all-ubuntu-amd64/builds/242/steps/compile/logs/stdio. You'll see the new warnings that this PR introduces. They point out problems with your async_read_until, async_read, and handler implementations. What you've written is not how function binding works nor is it how to use boost error codes. I suggest to either read the boost docs more or look at other areas of the code that implement what you're trying to do or simply look at what was previously there.

Once you resolve this issue, the remaining issues that prevent us from merging should should be a breeze to fix.

[guzzijones]

switch back to using async_receive is what i see as a possibility.

[guzzijones]

i removed the offending header and rebased again.

[anonimal]

You asked for harsh, so you shall receive! 😃

Too many comments on my end, I've lost my train of thought for this second pass. I can elaborate more on IRC for the remainder.

[anonimal]

All these header changes and line removals: please don't move them around unless needed; they can stay within their own groups.

[anonimal]

I see no reason why you wanted to change this name. Please don't change class/function names unless absolutely needed.

[anonimal]

As I'll note below, I2PService handler is not confined to the HTTP Proxy. Virtual or not, I have no idea why you wanted to change all of this.

[guzzijones]

This was not changed. Both of these functions still exist.

[anonimal]

I had no where else to comment, this comment was for your handling implementation as it relates to I2PService (and your handling implementation).

[anonimal]

If you're going to quote me, then quote me 😄

[anonimal]

AsyncHandleRead

What is this referencing? If you want to output the function name, use __func__. If not, please keep in line with the class name HTTPProxyHandler. Ideally, our logger will take care of this in the future.

[anonimal]

set_result

?

[anonimal]

I use my garlicsite as a reference-point to show that I've been in a certain area. Certainly you don't want me to take credit for your work? Why not guzzi.i2p?

Also, doesn't the RFC specify uppercase HTTP?

[anonimal]

You could stand to make these tests more robust. I can provide details on IRC.

[anonimal]

See above about logging class name/function name.

[anonimal]

This isn't needed. Please try to be more concise with your commits.

[guzzijones]

Thanks for the feedback. All good stuff. I will get on it this weekend. Enviado con AquaMail para Android http://www.aqua-mail.com En 27 de enero de 2017 10:29:00 PM 0x914409F1 <notifications@github.com> escribió:

…

anonimal requested changes on this pull request. You asked for harsh, so you shall receive! 😃 Too many comments on my end, I've lost my train of thought for this second pass. I can elaborate more on IRC for the remainder. > #include "core/router/identity.h" #include "core/util/i2p_endian.h" - All these header changes and line removals: please don't move them around unless needed; they can stay within their own groups. > namespace kovri { namespace client { -// -// Server -// - -HTTPProxyServer::HTTPProxyServer( +HTTPProxyServerService::HTTPProxyServerService( I see no reason why you wanted to change this name. Please don't change class/function names unless absolutely needed. > std::shared_ptr<boost::asio::ip::tcp::socket> socket) { return std::make_shared<HTTPProxyHandler>(this, socket); } -// -// Handler -// - -void HTTPProxyHandler::AsyncSockRead() { +void HTTPProxyHandler::Handle() { As I'll note below, I2PService handler is not confined to the HTTP Proxy. Virtual or not, I have no idea why you wanted to change all of this. > - LOG(warning) << "HTTPProxyHandler: sock recv got error: " << ecode; +void HTTPProxyHandler::AsyncSockRead( + std::shared_ptr<boost::asio::ip::tcp::socket> socket) { + // TODO(guzzi) but there's also use cases where you are providing an inproxy + // service for + // others + // 00:27 < zzz2> for a full threat model including "slowloris" attacks, + // you need to + // enforce max header lines, max header line length + // and a total header timeout + // 00:27 < zzz2> (in addition to the typical read timeout) + // read in header until \r\n\r\n, then read in small portions of body + // and forward along + // Read the request headers, which are terminated by a blank line. + // TODO(guzzi) + // unit test: One example is the addressbook unit-test I merged earlier. If you're going to quote me, then quote me 😄 > + + boost::asio::async_read_until( + *socket, + m_Protocol.m_Buffer, + "\r\n\r\n", + boost::bind( + &HTTPProxyHandler::AsyncHandleReadHeaders, + shared_from_this(), + boost::asio::placeholders::error, + boost::asio::placeholders::bytes_transferred)); +} +void HTTPProxyHandler::AsyncHandleReadHeaders( + const boost::system::error_code& error, + size_t bytes_transfered) { + if (error) { + LOG(debug) << "AsyncHandleRead: error sock read: " << bytes_transfered; >AsyncHandleRead What is this referencing? If you want to output the function name, use `__func__`. If not, please keep in line with the class name `HTTPProxyHandler`. Ideally, our logger will take care of this in the future. > Terminate(); return; } - if (HandleData(m_Buffer.data(), len)) { - if (m_State == static_cast<std::size_t>(State::done)) { - LOG(info) << "HTTPProxyHandler: proxy requested: " << m_URL; - GetOwner()->CreateStream( - std::bind( - &HTTPProxyHandler::HandleStreamRequestComplete, + boost::asio::streambuf::const_buffers_type bufs = m_Protocol.m_Buffer.data(); + std::string tbuffer( + boost::asio::buffers_begin(bufs), + boost::asio::buffers_begin(bufs) + m_Protocol.m_Buffer.size()); + + if (!m_Protocol.HandleData(tbuffer)) { + LOG(debug) << "AsyncHandleRead: error HandleData() " << "check http proxy"; See above about logging class name/function name. > - if (m_State == static_cast<std::size_t>(State::done)) { - LOG(info) << "HTTPProxyHandler: proxy requested: " << m_URL; - GetOwner()->CreateStream( - std::bind( - &HTTPProxyHandler::HandleStreamRequestComplete, + boost::asio::streambuf::const_buffers_type bufs = m_Protocol.m_Buffer.data(); + std::string tbuffer( + boost::asio::buffers_begin(bufs), + boost::asio::buffers_begin(bufs) + m_Protocol.m_Buffer.size()); + + if (!m_Protocol.HandleData(tbuffer)) { + LOG(debug) << "AsyncHandleRead: error HandleData() " << "check http proxy"; + HTTPRequestFailed(); // calls Terminate + return; + } + size_t num_additional_bytes = m_Protocol.m_Buffer.size() - bytes_transfered; Please use `std::size_t` and the standard library for all types when possible. Calling `size_t` directly is deprecated and the only reason this compiles now is because of deprecations elsewhere that have included `stddef.h`. > - LOG(info) << "HTTPProxyHandler: proxy requested: " << m_URL; - GetOwner()->CreateStream( - std::bind( - &HTTPProxyHandler::HandleStreamRequestComplete, + boost::asio::streambuf::const_buffers_type bufs = m_Protocol.m_Buffer.data(); + std::string tbuffer( + boost::asio::buffers_begin(bufs), + boost::asio::buffers_begin(bufs) + m_Protocol.m_Buffer.size()); + + if (!m_Protocol.HandleData(tbuffer)) { + LOG(debug) << "AsyncHandleRead: error HandleData() " << "check http proxy"; + HTTPRequestFailed(); // calls Terminate + return; + } + size_t num_additional_bytes = m_Protocol.m_Buffer.size() - bytes_transfered; + if (num_additional_bytes > 0) { if (num_additional_bytes) > - if (m_State == static_cast<std::size_t>(State::done)) { - LOG(info) << "HTTPProxyHandler: proxy requested: " << m_URL; - GetOwner()->CreateStream( - std::bind( - &HTTPProxyHandler::HandleStreamRequestComplete, + boost::asio::streambuf::const_buffers_type bufs = m_Protocol.m_Buffer.data(); + std::string tbuffer( + boost::asio::buffers_begin(bufs), + boost::asio::buffers_begin(bufs) + m_Protocol.m_Buffer.size()); + + if (!m_Protocol.HandleData(tbuffer)) { + LOG(debug) << "AsyncHandleRead: error HandleData() " << "check http proxy"; + HTTPRequestFailed(); // calls Terminate + return; + } + size_t num_additional_bytes = m_Protocol.m_Buffer.size() - bytes_transfered; Also, be aware of what your subtracting. This could go very badly. > + // add the additional bytes the m_body + m_Protocol.m_Body = str; + } + auto it = std::find_if( + m_Protocol.m_HeaderMap.begin(), + m_Protocol.m_HeaderMap.end(), + [](std::pair<std::string, std::string> arg) { + boost::trim_left(arg.first); + boost::trim_right(arg.first); + return arg.first == "Content-Length"; + }); + if (it != m_Protocol.m_HeaderMap.end()) { + std::istringstream iss(boost::trim_left_copy(it->second)); + size_t clen; + iss >> clen; + clen = clen - num_additional_bytes; Also, be aware of what your subtracting. This could go very badly. > + boost::asio::buffers_end(buf) - num_additional_bytes, + boost::asio::buffers_end(buf)); + // add the additional bytes the m_body + m_Protocol.m_Body = str; + } + auto it = std::find_if( + m_Protocol.m_HeaderMap.begin(), + m_Protocol.m_HeaderMap.end(), + [](std::pair<std::string, std::string> arg) { + boost::trim_left(arg.first); + boost::trim_right(arg.first); + return arg.first == "Content-Length"; + }); + if (it != m_Protocol.m_HeaderMap.end()) { + std::istringstream iss(boost::trim_left_copy(it->second)); + size_t clen; Please use `std::size_t` and the standard library for all types when possible. Calling `size_t` directly is deprecated and the only reason this compiles now is because of deprecations elsewhere that have included `stddef.h`. > + } + auto it = std::find_if( + m_Protocol.m_HeaderMap.begin(), + m_Protocol.m_HeaderMap.end(), + [](std::pair<std::string, std::string> arg) { + boost::trim_left(arg.first); + boost::trim_right(arg.first); + return arg.first == "Content-Length"; + }); + if (it != m_Protocol.m_HeaderMap.end()) { + std::istringstream iss(boost::trim_left_copy(it->second)); + size_t clen; + iss >> clen; + clen = clen - num_additional_bytes; + // need to call one more function to fill body after this read. + if (clen > 0) { if (clen) > } + + } else { + // call with success error code and zero bytes transfered additional + HandleSockRecv(error, 0); } You don't want to call handlers like this nor do you want to handle error codes like this. I'll explain more on IRC. > - if (m_State == static_cast<std::size_t>(State::done)) - return CreateHTTPRequest(buf, len); +void HTTPProxyHandler::HandleSockRecv( + const boost::system::error_code& error, + std::size_t bytes_transferred) { + if (error) { + LOG(debug) << "HTTPSockRecv: error sock read: " << bytes_transferred; + Terminate(); + return; + } + // TODO(guzzi) should not read entire body into memory + // instead read a buffer full ie 512 bytes and I2pconnect and send. + // if we read some buffer into the body buffer variable then save it to m_body + // will need another function handler call that loops itself until all is read + // and then finishes up below. if 0 is read it instead closes the connection + if (bytes_transferred != 0) { if (bytes_transferred) > + shared_from_this(), + std::placeholders::_1), + m_Protocol.m_Address, + m_Protocol.m_Port); + } +} + +bool HTTPProtocol::HandleData(const std::string& protocol_string) { + std::vector<std::string> header_body; + std::vector<std::string> tokens_header_body; + // get header info + // initially set error response to bad_request + m_ErrorResponse = HTTPResponse(HTTPResponseCodes::status_t::bad_request); + if (boost::algorithm::split_regex( + header_body, protocol_string, boost::regex("\r\n\r\n")) + .size() This member belongs to split_regex (e.g., simply attach it to previous line). > + } +} + +bool HTTPProtocol::HandleData(const std::string& protocol_string) { + std::vector<std::string> header_body; + std::vector<std::string> tokens_header_body; + // get header info + // initially set error response to bad_request + m_ErrorResponse = HTTPResponse(HTTPResponseCodes::status_t::bad_request); + if (boost::algorithm::split_regex( + header_body, protocol_string, boost::regex("\r\n\r\n")) + .size() + != HEADERBODY_LEN) + return false; + if (boost::algorithm::split_regex(tokens_header_body, header_body[0], boost::regex("\r\n")) + .size() This member belongs to split_regex (e.g., simply attach it to previous line). > @@ -220,47 +274,71 @@ void HTTPProxyHandler::HandleStreamRequestComplete( stream); GetOwner()->AddHandler(connection); connection->I2PConnect( - reinterpret_cast<const std::uint8_t*>(m_Request.data()), - m_Request.size()); + reinterpret_cast<const uint8_t*>(m_Protocol.m_Request.c_str()), Please use `std::size_t` and the standard library for all types when possible. Calling `size_t` directly is deprecated and the only reason this compiles now is because of deprecations elsewhere that have included `stddef.h`. > - m_UserAgent = "MYOB/6.66 (AN/ON)"; - m_Request += "User-Agent: " + m_UserAgent + "\r\n"; - // Append remaining original request - m_Request.append(reinterpret_cast<const char *>(buf), len); + + // find and remove/adjust headers + auto it_user_agent = std::find_if( + m_HeaderMap.begin(), + m_HeaderMap.end(), + [](std::pair<std::string, std::string> arg) { + boost::trim_left(arg.first); + boost::trim_right(arg.first); + return arg.first == "User-Agent"; + }); + if (it_user_agent != m_HeaderMap.end()) // found + it_user_agent->second = " MYOB/6.66 (AN/ON)"; >" MYOB/6.66 (AN/ON)" Watch out for the whitespace in strings, regardless of if they are trimmed or not. > return true; } -bool HTTPProxyHandler::ExtractIncomingRequest() { +bool HTTPProtocol::ExtractIncomingRequest() { >HTTPProtocol I truly don't agree nor understand why you want to call this class `HTTPProtocol`. It's confusing and I don't see the point. > #include <boost/asio.hpp> - Please don't remove lines like this unless absolutely needed. > - done, - } m_State; - - /// @brief Sets state set by handled data - void SetState( - const State& state); + /// @var m_JumpService + /// @brief Address helpers for base64 jump service + const std::array<std::string, 4> m_JumpService { + { + "?i2paddresshelper=", + "&i2paddresshelper=", + "?kovrijumpservice=", + "&kovrijumpservice=", + } + }; Please don't mix style changes like this, it was fine the way it was. > /// @brief Performs regex, sets address/port/path, validates version /// on request sent from user /// @return true on success bool ExtractIncomingRequest(); - /// @brief Parses path for base64 address, inserts into address book - void HandleJumpService(); + /// @brief Processes original request: extracts, validates, + /// calls jump service, appends original request + /// @return true on success + bool CreateHTTPRequest(); + + const unsigned int HEADERBODY_LEN = 2; + const unsigned int REQUESTLINE_HEADERS_MIN = 1; Please use `enum struct` or `enum` for these two. > + HTTPProxyHandler( + HTTPProxyServerService* parent, + std::shared_ptr<boost::asio::ip::tcp::socket> socket) + : I2PServiceHandler(parent), + m_Socket(socket) { + } + + ~HTTPProxyHandler() { + Terminate(); + } + + void set_result(boost::optional<boost::system::error_code> * a, + boost::system::error_code b ) { + if (b == 0) + a->reset(b); + } >set_result ? > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > PROFITS; OR BUSINESS // + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, // + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF // + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. // + * // + * Parts of the project are originally copyright (c) 2013-2015 The PurpleI2P Project // + */ +#define BOOST_TEST_DYN_LINK +#include <boost/test/unit_test.hpp> +#include "client/proxy/http.h" + +BOOST_AUTO_TEST_SUITE(HTTPPProtocolTests) + +BOOST_AUTO_TEST_CASE(Short) { + kovri::client::HTTPProtocol tmp; + std::string tmpData = "GET anonimal.i2p http/1.1"; I use my garlicsite as a reference-point to show that I've been in a certain area. Certainly you don't want me to take credit for your work? Why not `guzzi.i2p`? Also, doesn't the RFC specify uppercase HTTP? > + kovri::client::HTTPProtocol tmp; + std::string tmpData = "GET http/1.1"; + BOOST_CHECK(!tmp.HandleData(tmpData)); +} +BOOST_AUTO_TEST_CASE(noHeadersAtAll) { + kovri::client::HTTPProtocol tmp; + std::string tmpData = "\r\n"; + BOOST_CHECK(!tmp.HandleData(tmpData)); +} +BOOST_AUTO_TEST_CASE(ok) { + kovri::client::HTTPProtocol tmp; + std::string tmpData = "GET anonimal.i2p "; + tmpData+="http/1.1\r\nUser-Agent: dummy\r\n\r\n"; + BOOST_CHECK(tmp.HandleData(tmpData)); +} + You *could* stand to make these tests more robust. I can provide details on IRC. > - } - break; - default: - LOG(error) << "HTTPProxyHandler: invalid state: " << m_State; - HTTPRequestFailed(status_t::internal_server_error); - return false; - } - buf++; - len--; - if (m_State == static_cast<std::size_t>(State::done)) - return CreateHTTPRequest(buf, len); +void HTTPProxyHandler::HandleSockRecv( + const boost::system::error_code& error, + std::size_t bytes_transferred) { + if (error) { + LOG(debug) << "HTTPSockRecv: error sock read: " << bytes_transferred; See above about logging class name/function name. > LOG(debug) << "HTTPProxyHandler: method is: " << m_Method << " request is: " << m_URL; - // Set defaults and regexp + This isn't needed. Please try to be more concise with your commits. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #528 (review)

[anonimal]

In addition to the aforementioned, always build and run unit-tests if you do work in that area because this PR will break the build (run make tests).

[anonimal]

num_additional_bytes is unsigned. Research the difference between signed and unsigned types (and overflow + underflow for both types) to learn why this line makes no sense. Even if this type was signed (but it's not so don't get caught up with what I'm about to say), you don't want to test this way; you'll usually want to throw a class from std::exception.

[guzzijones]

i understand.

[anonimal]

Still doesn't prevent overflow if bytes transferred is greater than buffer size. Ensure that this will never happen.

[guzzijones]

i understand.

[anonimal]

See my comment above.

[guzzijones]

i understand.

[guzzijones]

I will go through tonight and add todos. I will go through tonight and run cpplint and test implemetation again. This push was for review. (others work during the day time). :)

[anonimal]

As discussed with @guzzijones on IRC, even though there are remaining issues to resolve (and he will resolve them with time), this PR will fix our headers issue and implements POST at the same time - so we can merge this now while he continues his TODO work.

[anonimal]

Resolves #507.