Skip to content
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

Merged
merged 25 commits into from
May 2, 2020
Merged

Log host information from websocket requests #134

merged 25 commits into from
May 2, 2020

Conversation

jmjatlanta
Copy link

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.

T _ws_connection;
// cache the value of the remote IP
std::string last_forward_header_key;
Copy link
Member

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?

Copy link

@pmconrad pmconrad May 28, 2019

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.

Copy link
Member

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?

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.

Copy link
Member

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?

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.

Copy link
Member

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().

@pmconrad
Copy link

Please rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants