Implement a HTTP(s), JSON-RPC and Websocket stack for query services #493
Conversation
libbitcoin subscriptions and query services, which extend existing zeromq services. Allow mbedtls to be conditionally compiled, which allows optional SSL support. Re-generate build artifacts.
|
This is an improved version of #449 in that it improves performance considerably, has the addition of JSON-RPC support, and does not have the licensing issues due to the third-party dependency previously used. |
|
MSVC++ 2017 warnings and errors: |
|
Updated, but untested on MSVC++ ... |
|
I'll work through these: |
evoskuil
left a comment
evoskuil
left a comment
There was a problem hiding this comment.
I'm working through some of these issues myself now, as well as eliminating a good amount of unnecessary casting. I'll post this as a partial update to the PR later today.
|
|
||
| #include <ostream> | ||
| #include <bitcoin/bitcoin.hpp> | ||
| #include <bitcoin/server/define.hpp> |
There was a problem hiding this comment.
No strong reason, they didn't appear to be required. I did not consider external include uses, so it's probably safer to retain them.
There was a problem hiding this comment.
The bc namespace is used (requires bitcoin.hpp) and the BCS_API symbol is used (requires define.hpp).
| secure_notification_worker_(authenticator_, *this, true), | ||
| public_notification_worker_(authenticator_, *this, false) | ||
| public_notification_worker_(authenticator_, *this, false), | ||
| #ifdef WITH_MBEDTLS |
There was a problem hiding this comment.
I think it would be better to isolate the WITH_MBEDTLS conditions to lower level code (e.g. not in server_node and to simply prevent intialization if it is not configured.
| { | ||
| auto data_length = static_cast<uint16_t>(length); | ||
| *reinterpret_cast<uint16_t*>(start) = | ||
| boost::endian::endian_reverse(data_length); |
There was a problem hiding this comment.
This is an endian bug, since it assumes a platform endianness (LE) in order to achieve a desired wire endianness (BE), but on a BE platform the wire serialization will be LE.
|
|
||
| // Clear the query_work_map for this connection before removal. | ||
| query_work_map.clear(); | ||
| connections_.erase(it); |
There was a problem hiding this comment.
This is a thread safety bug, since connections_ is unguarded but used concurrently throughout the class.
| LocalFree(buffer); | ||
| return error_string; | ||
| #else | ||
| return { strerror(last_error()) }; |
There was a problem hiding this comment.
Thread safety: std::strerror is not required to be thread safe.
| FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | | ||
| FORMAT_MESSAGE_IGNORE_INSERTS, nullptr, dw, | ||
| MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), | ||
| reinterpret_cast<LPTSTR>(&buffer), 0, nullptr); |
There was a problem hiding this comment.
Unicode bug: the FormatMessage() and T-String (TSTR) macros compile as unicode (UCS-2) but the buffer is UTF-8.
| // boost parsing helpers included from json_parser.hpp. | ||
| // See: https://svn.boost.org/trac10/ticket/12621 | ||
| #include <functional> | ||
| using namespace std::placeholders; |
There was a problem hiding this comment.
Avoid declaring using namespace in headers.
| namespace server { | ||
| namespace http { | ||
|
|
||
| using namespace boost::property_tree; |
There was a problem hiding this comment.
Avoid declaring using namespace in headers.
|
@evoskuil Thanks for taking a look at this, and good finds! Will review and re-test the incoming update. |
|
It's a huge amount of work, there's bound to be stuff like this, but I'm excited about it. |
| struct in_addr address; | ||
| std::memcpy(&address, &ip, sizeof(address)); | ||
|
|
||
| const auto ip_address = std::string(inet_ntoa(address)); |
There was a problem hiding this comment.
The address computation is dead code, as only the hostname string is used. What is the intent?
There was a problem hiding this comment.
Hrm, the ip_address comparison is missing, but should be used similarly to the hostname comparison.
There was a problem hiding this comment.
It's not clear to me that this is an important security check. What exactly does it prevent?
There was a problem hiding this comment.
I'm not claiming that it actually secures anything, because it's a field that can easily be spoofed. It is supposed to be checked/validated however:
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Origin
https://tools.ietf.org/html/rfc6455
See section 1.6:
"The WebSocket Protocol uses the origin model used by web browsers to
restrict which web pages can contact a WebSocket server when the
WebSocket Protocol is used from a web page. Naturally, when the
WebSocket Protocol is used by a dedicated client directly (i.e., not
from a web page through a web browser), the origin model is not
useful, as the client can provide any arbitrary origin string."
And:
https://tools.ietf.org/html/rfc6455#section-10.2
I chose to do the validation because the libbitcoin use-cases envisioned are both from browsers and standalone applications. In the standalone application case, it's trivial to fake with something like this (nodejs), as stated above:
var query_socket = new WebSocket(query_url,
{
origin: 'https://localhost:9000',
rejectUnauthorized: false
});
|
|
||
| const auto ip = resolve_hostname({ hostname.data() }); | ||
| struct in_addr address; | ||
| std::memcpy(&address, &ip, sizeof(address)); |
There was a problem hiding this comment.
This is an endianness bug as it copies from the address of an integral value into a multi-segment structure.
| if (origin.empty()) | ||
| return false; | ||
|
|
||
| if (gethostname(hostname.data(), max_hostname_length) != 0) |
There was a problem hiding this comment.
This is a unicode bug as it will only obtain hostname using the ANSI code page on Win32, but UTF-8 unicode is expected in the return.
| } | ||
|
|
||
| int32_t written = connection->write(buffer.data(), read); | ||
| if (written < 0) |
There was a problem hiding this comment.
Based on the subsequent log message it appears that this condition should be:
if (written != read)...
There was a problem hiding this comment.
Good find. I believe that check as originally written is ok (and the log could be adjusted since it is really showing the write failure, not the amount of bytes written), but then the bug is that file_transfer.offset should be incremented by written, not read. It's not technically required that all of that write complete (or be buffered) at once.
There was a problem hiding this comment.
On second thought, it may be better to adjust that check as you specified (although the log message could still be adjusted) because otherwise we'd have to account for seeking backwards by the amount difference (read - written) to prevent data corruption in the stream.
|
|
||
| return ((origin.find("127.0.0.1") != std::string::npos) || | ||
| (origin.find(hostname_string) != std::string::npos) || | ||
| (origin.find("localhost") != std::string::npos)); |
There was a problem hiding this comment.
These tests would match values such as 127.0.0.12 and foo_localhost42.
| if (buffered_length >= high_water_mark_) | ||
| { | ||
| // Drain the buffered data. | ||
| while (!write_buffer_.empty()) |
There was a problem hiding this comment.
This throttles the server in order to satisfy a slow client. When high water is hit for a client new to-be-buffered messages should be dropped.
|
After many rounds of feedback and re-factoring, this PR is being replaced by libbitcoin/libbitcoin-protocol#193 and #497 |
3 participants
Implement a HTTP(s), JSON-RPC, and Websocket stack/interface for libbitcoin subscriptions and query services, which extend existing zeromq services.
Allow mbedtls to be conditionally compiled, which allows optional SSL support.
Re-generate build artifacts.
Resolves #153