-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log host information from websocket requests #134
Conversation
src/network/http/websocket.cpp
Outdated
T _ws_connection; | ||
// cache the value of the remote IP | ||
std::string last_forward_header_key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get why we need this. Will the key change after a while? Will the header change after the connection is established? Will the connection be reopened after closed? Will the object be reused for a new connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is not necessary. The header key should be set in the websocket_server before any connections are established, and it shouldn't change after that, and even if it is changed the change need not apply to existing connections. KISS, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to handle HTTP connections with connection: keep-alive
header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. http_handler always closes the connection after sending a response, and for a connection that has been upgraded to websocket protocol there will be no more http requests either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it means the connection: keep-alive
header is ignored. Are we going to support it in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, the http_handler is called for every http request. So we create and close a websocket_connection
for each request, not for each actual connection. (This also means I was wrong above - we don't close the http connection after sending the response, but only our own websocket_connection.)
So even in the presence of a keep-alive http connection, the code in its current form should retrieve the remote address separately for each request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to page 21 of RFC 6455, to establish a websocket connection, the request must include a connection
header with the upgrade
keyword inside, however, the RFC doesn't mention "keep-alive", so I think it's implementation-dependent.
The client's opening handshake consists of the following parts.
...
3. An |Upgrade| header field containing the value "websocket",
treated as an ASCII case-insensitive value.
4. A |Connection| header field that includes the token "Upgrade",
treated as an ASCII case-insensitive value.
...
When the connection is upgraded to a websocket connection, there is only one set of HTTP request headers attached to the connection, all following request messages must have the same original IP address (neither IP address nor forward-key would change), so it's best to get the address in set_open_handler()
and use it in set_message_handler()
, but not put all code in set_message_handler()
.
When the connection is a pure HTTP connection, there may be several requests received in the connection, we may probably need to handle connection: keep-alive
header. If a new websocket_connection object is created for each request, then the header can be ignored. Anyway, we want to extract real IP addresses of the clients and log them, so we need to update set_http_handler()
.
Please rebase |
The funcionality is already covered by the websocket_client class
Code synced from connect(), added todo to refactor later
This fixes /bitshares/bitshares-core/issues/844
This also replace PR #45 that got tangled as I merged instead of rebased.
This will log the remote IP address when messages come in. For those working with a proxy, the proxy can add a header key and value that will be used to log the remote's information instead of the proxy's information.
2 small programs (ws_test_client, ws_test_server) were also added to assist with testing. If we think they are no longer useful, I will remove them.