machinezone / IXWebSocket

websocket and http client and server library, with TLS support and very few dependencies
BSD 3-Clause "New" or "Revised" License
528 stars 171 forks source link

Client authorization when connecting to server? #260

Closed ludekvodicka closed 3 years ago

ludekvodicka commented 3 years ago

In libWebsocket, there is special handler for checking client headers on the server-side called LWS_CALLBACK_FILTER_PROTOCOL_CONNECTION . In this handler, it's possible to read headers and if necessary, return HTTP_STATUS_UNAUTHORIZED and closed the connection.

In IxWebsocket, I can send HTTP headers via

   ix::WebSocketHttpHeaders headers;
   headers["Authorization"] = userAndPasswordBase64;
   webSocket.setExtraHeaders(headers);

and then read these headers in open handler (msg->type == ix::WebSocketMessageType::Open) from msg->openInfo.headers struct. This works great.

But how to signalize, that authorization failed? I know I can return status and message via close() :

websocket->close(code, "Authorization failed");

but when I set code = 401, I'm receiving "unknow code" on the client-side. So I checked ix::WebSocketCloseConstants but there is no code for authorization failed.

I also checked a link referred from the code (https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent) but they also don't have any authorization-failed header.

Currently, I'm returning ix::WebSocketCloseConstants::kInternalErrorCode but I believe there has to be a better way how to return this code.

Thanks a lot.

PS: As a side note, the integration of your library is really fluent and easy compared to libwebsocket ;-)

bsergean commented 3 years ago
  1. Indeed support for what you're trying to do support is missing ; it's a valid use case and maybe it can be implemented. (wrote this long part first, I think you can skip to (2))

I have a websocket service running on the server below, which also require a basic form of authentication. If the url isn't correct a 403 http error is returned right away. The server is coded in python with a library that let me do the kind of things you're trying to do btw / see code here -> https://github.com/machinezone/cobra/blob/master/cobras/server/app.py#L139)

ixwebsocket comes with a cli tool called ws which I just used like that to reproduce your problem.

ws connect 'wss://bavarde.jeanserge.com/v2/?appkey=invalid_appkey'

However this is the error I get:

Connection error: Expecting status 101 (Switching Protocol), got 403 status connecting to wss://bavarde.jeanserge.com/v2/?appkey=invalid_appkey, HTTP Status line: HTTP/1.1 403 Forbidden

Which is different from the normal errors which can be handled through the normal socket, as in the example below:

        else if (msg->type == ix::WebSocketMessageType::Error)
        {
            ss << "Connection error: " << msg->errorInfo.reason << std::endl;
            ss << "#retries: " << msg->errorInfo.retries << std::endl;
            ss << "Wait time(ms): " << msg->errorInfo.wait_time << std::endl;
            ss << "HTTP Status: " << msg->errorInfo.http_status << std::endl;
            log(ss.str());
        }

=== This is where we do the websocket handshake, where potentially we can get this 401 error.

    WebSocketInitResult status = _ws.connectToUrl(_url, headers, timeoutSecs);
    if (!status.success)
    {

        // FIXME here we should invoke the callback with type == error.
        return status;
    }

Here I could return the error code. But that's on the client side.

On the server side, we would need a new hook to be able to return something equivalent to what libwebsocket does, HTTP_STATUS_UNAUTHORIZED or similar.

    server.setOnConnectionCallback(
        [remoteUrl, remoteUrlsMapping](std::weak_ptr<ix::WebSocket> webSocket,
                                       std::shared_ptr<ConnectionState> connectionState) {
            auto state = std::dynamic_pointer_cast<ProxyConnectionState>(connectionState);
            auto remoteIp = connectionState->getRemoteIp();

            xxxx / this is called when a connection is established (before or after handshake ... can't remember)
            so here we would need a way to inspect the headers (Authorization) in your case, and close the connection
            or rather return an http error code, 401, xxx

This will also require some work but is not impossible. Not sure when I'll get to this though, and if it's needed because ...

  1. I think you can handle this authorization in a different way, and this is what you're trying to do, by doing it at the WebSocket level, after the handshake, and not at the http level (early)

The deal with websocket error code is that some are reserved. So maybe your problem is simply that you are using the wrong one, you would have to use a value higher than 4000 and below 4999. 401 is probably rejected

https://github.com/Luka967/websocket-close-codes

4000 - 4999 No Yes Available for applications

I just tried to have a client close with error 4500, and the server received it. It should work equally well the other way around.

On Dec 17, 2020, at 1:59 PM, Ludek Vodicka notifications@github.com wrote:

In libWebsocket, there is special handler for checking client headers on the server-side called LWS_CALLBACK_FILTER_PROTOCOL_CONNECTION . In this handler, it's possible to read headers and if necessary, return HTTP_STATUS_UNAUTHORIZED and closed the connection.

In IxWebsocket, I can send HTTP headers via

ix::WebSocketHttpHeaders headers; headers["Authorization"] = userAndPasswordBase64; webSocket.setExtraHeaders(headers); and then read these headers in open handler (msg->type == ix::WebSocketMessageType::Open) from msg->openInfo.headers struct. This works great.

But how to signalize, that authorization failed? I know I can return status and message via close() :

websocket->close(code, "Authorization failed"); but when I set code = 401, I'm receiving "unknow code" on the client-side. So I checked ix::WebSocketCloseConstants but there is no code for authorization failed.

I also checked a link referred from the code (https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent) but they also don't have any authorization-failed header.

Currently, I'm returning ix::WebSocketCloseConstants::kInternalErrorCode but I believe there have to be a better way how to return this code.

Thanks a lot.

PS: As a side node, integration of your library is really fluent and easy compared to libwebsocket ;-)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/machinezone/IXWebSocket/issues/260, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UJ6HKG6XAUYDCF4UCLSVJ5MZANCNFSM4VAJ4EQA.

bsergean commented 3 years ago

See 461a645

Hope it helps.

On Dec 17, 2020, at 10:23 PM, Benjamin Sergeant bsergean@gmail.com wrote:

  1. Indeed support for what you're trying to do support is missing ; it's a valid use case and maybe it can be implemented. (wrote this long part first, I think you can skip to (2))

I have a websocket service running on the server below, which also require a basic form of authentication. If the url isn't correct a 403 http error is returned right away. The server is coded in python with a library that let me do the kind of things you're trying to do btw / see code here -> https://github.com/machinezone/cobra/blob/master/cobras/server/app.py#L139 https://github.com/machinezone/cobra/blob/master/cobras/server/app.py#L139)

ixwebsocket comes with a cli tool called ws which I just used like that to reproduce your problem.

ws connect 'wss://bavarde.jeanserge.com/v2/?appkey=invalid_appkey wss://bavarde.jeanserge.com/v2/?appkey=invalid_appkey'

However this is the error I get:

Connection error: Expecting status 101 (Switching Protocol), got 403 status connecting to wss://bavarde.jeanserge.com/v2/?appkey=invalid_appkey wss://bavarde.jeanserge.com/v2/?appkey=invalid_appkey, HTTP Status line: HTTP/1.1 403 Forbidden

Which is different from the normal errors which can be handled through the normal socket, as in the example below:

        else if (msg->type == ix::WebSocketMessageType::Error)
        {
            ss << "Connection error: " << msg->errorInfo.reason << std::endl;
            ss << "#retries: " << msg->errorInfo.retries << std::endl;
            ss << "Wait time(ms): " << msg->errorInfo.wait_time << std::endl;
            ss << "HTTP Status: " << msg->errorInfo.http_status << std::endl;
            log(ss.str());
        }

=== This is where we do the websocket handshake, where potentially we can get this 401 error.

    WebSocketInitResult status = _ws.connectToUrl(_url, headers, timeoutSecs);
    if (!status.success)
    {

        // FIXME here we should invoke the callback with type == error.
        return status;
    }

Here I could return the error code. But that's on the client side.

On the server side, we would need a new hook to be able to return something equivalent to what libwebsocket does, HTTP_STATUS_UNAUTHORIZED or similar.

    server.setOnConnectionCallback(
        [remoteUrl, remoteUrlsMapping](std::weak_ptr<ix::WebSocket> webSocket,
                                       std::shared_ptr<ConnectionState> connectionState) {
            auto state = std::dynamic_pointer_cast<ProxyConnectionState>(connectionState);
            auto remoteIp = connectionState->getRemoteIp();

            xxxx / this is called when a connection is established (before or after handshake ... can't remember)
            so here we would need a way to inspect the headers (Authorization) in your case, and close the connection
            or rather return an http error code, 401, xxx

This will also require some work but is not impossible. Not sure when I'll get to this though, and if it's needed because ...

  1. I think you can handle this authorization in a different way, and this is what you're trying to do, by doing it at the WebSocket level, after the handshake, and not at the http level (early)

The deal with websocket error code is that some are reserved. So maybe your problem is simply that you are using the wrong one, you would have to use a value higher than 4000 and below 4999. 401 is probably rejected

https://github.com/Luka967/websocket-close-codes https://github.com/Luka967/websocket-close-codes

4000 - 4999 No Yes Available for applications

I just tried to have a client close with error 4500, and the server received it. It should work equally well the other way around.

On Dec 17, 2020, at 1:59 PM, Ludek Vodicka <notifications@github.com mailto:notifications@github.com> wrote:

In libWebsocket, there is special handler for checking client headers on the server-side called LWS_CALLBACK_FILTER_PROTOCOL_CONNECTION . In this handler, it's possible to read headers and if necessary, return HTTP_STATUS_UNAUTHORIZED and closed the connection.

In IxWebsocket, I can send HTTP headers via

ix::WebSocketHttpHeaders headers; headers["Authorization"] = userAndPasswordBase64; webSocket.setExtraHeaders(headers); and then read these headers in open handler (msg->type == ix::WebSocketMessageType::Open) from msg->openInfo.headers struct. This works great.

But how to signalize, that authorization failed? I know I can return status and message via close() :

websocket->close(code, "Authorization failed"); but when I set code = 401, I'm receiving "unknow code" on the client-side. So I checked ix::WebSocketCloseConstants but there is no code for authorization failed.

I also checked a link referred from the code (https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent) but they also don't have any authorization-failed header.

Currently, I'm returning ix::WebSocketCloseConstants::kInternalErrorCode but I believe there have to be a better way how to return this code.

Thanks a lot.

PS: As a side node, integration of your library is really fluent and easy compared to libwebsocket ;-)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/machinezone/IXWebSocket/issues/260, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UJ6HKG6XAUYDCF4UCLSVJ5MZANCNFSM4VAJ4EQA.

ludekvodicka commented 3 years ago

You're the best! Thanks a lot for the detailed answer and such fast implementation. I'm going to test it right now.

bsergean commented 3 years ago

If this does not cut it for you, and you need a pure HTTP approach like libwesocket please tell. It will be harder but it should be feasible.

On Dec 18, 2020, at 6:37 AM, Ludek Vodicka notifications@github.com wrote:

You're the best! Thanks a lot for the detailed answer and such fast implementation. I'm going to test it right now.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/machinezone/IXWebSocket/issues/260#issuecomment-748117613, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UNUHAORWPI2UTHO6H3SVNSKVANCNFSM4VAJ4EQA.

ludekvodicka commented 3 years ago

It's not necessary in this case. Both, the server and the client are my applications so I can control return codes and the logic around. In case you will implement it anytime in the future, I will probably use it, but it's not a must-have for me now.

I'm really satisfied with how easy was to integrate your component into my use-case. The only missing feature (we already discussed it some time ago) is the ability to have multiple streams in a single thread to save system resources. But because Binance already implemented live subscribing/unsubscribing via WS commands, I'm able to achieve this logic in this way.

bsergean commented 3 years ago

Ok great I won't bother for now.

I have actually been working on a different approach for the library which uses libuv (through its C++ wrapper uvw) and can achieve multiple connections (many) on a single thread thanks to libuv epoll etc...

Big caveat is that it does not support SSL (and that will be hard to add), there's no server yet it's very alpha.

2 examples:

https://github.com/bsergean/uvweb/blob/master/cli/UvwebWsClient.cpp https://github.com/bsergean/uvweb/blob/master/uvweb/PulsarClient.cpp

On Dec 18, 2020, at 9:28 AM, Ludek Vodicka notifications@github.com wrote:

It's not necessary in this case. Both, the server and the client are my applications so I can control return codes and the logic around. In case you will implement it anytime in the future, I will probably use it, but it's not a must-have for me now.

I'm really satisfied with how easy was to integrate your component into my use-case. The only missing feature (we already discussed it some time ago) is the ability to have multiple streams in a single thread to save system resources. But because Binance already implemented live subscribing/unsubscribing via WS commands, I'm able to achieve this logic in this way.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/machinezone/IXWebSocket/issues/260#issuecomment-748218903, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UNKSD24ROJPDKZCCRDSVOGNRANCNFSM4VAJ4EQA.

ludekvodicka commented 3 years ago

libuv approach with multiple connections per thread sounds very good. But I'm not sure how significant will be a limitation to unsecured connections only. I'm not sure if these data providers (no matter if exchanges or other data sources) provide also non-SSL connections.

ludekvodicka commented 3 years ago

Hi, Today I'm testing an IXWebsocket library in my main application and unfortunately, the authorization solution doesn't work for 100%. The problem is that closing the connection in the open event (on the server-side) sometimes behaves differently.

Probably depending on the threads state, sometime client receive "Abnormal closure (1006)" instead of 4001 when closed via sessionInfo.websocket->close(4001, "Authorization failed");

The second issue, sometimes when close() is executed from the Open handler, the Close handler isn't executed at all. This is somehow related to auto-reconnection, because I was able to limit this almost to 0 when disabled autoreconnection.

The third (minor) issue is, that although I immediately close the connection in the server ix::WebSocketMessageType::Open event, the client already started to proceed ix::WebSocketMessageType::Open on the client-side.

I believe all of these issues would be solve via standalone synchronous ix::WebSocketMessageType::Authorize event, where when the server returns false, client and server will not start proceeding with the Open handler. But I understand that this will need more work to implement.

I hope I describe it as understandable as possible. Unfortunately, it's not possible to create some simple test-case, because this behavior occurs only occasionally (depending on platform linux/osx/windows as same as on the cpu load, etc.).

ludekvodicka commented 3 years ago

A quick update about my progress. I decided to give a try to another Websocket library uWebsockets. They have the only server-side part so I implemented server as uWebsocket and client as IXWebsocket. So far everything seems great.

uWebsockets has an "upgrade" handler where is possible to check HTTP headers and return 401 as unauthorized and your library supports headers for clients, so was able to successfully implement complete authorization. All our tests are running successfully with this combination (uWebsockets + IXWebsocket client).

As a side note, not sure why, but when I switch to IXWebsocket server, one test takes ~2 sec, when switched to uWebsockets, one test takes ~0.2sec. It's probably because of the different threading model but maybe there are some other bottlenecks in your server-side. I'm writing it to you only as feedback.

We probably stick with this solution, because your client library is easy to use and quick and their server supports all features we currently need.

bsergean commented 3 years ago

Sounds good. Maybe I'll implement the 'upgrade' handler in server mode one day.

On Dec 22, 2020, at 1:25 PM, Ludek Vodicka notifications@github.com wrote:

A quick update about my progress. I decided to give a try to another Websocket library uWebsockets. They have the only server-side part so I implemented server as uWebsocket and client as IXWebsocket. So far everything seems great.

uWebsockets has an "upgrade" handler where is possible to check HTTP headers and return 401 as unauthorized and your library supports headers for clients, so was able to successfully implement complete authorization. All our tests are running successfully with this combination (uWebsockets + IXWebsocket client).

As a side note, not sure why, but when I switch to IXWebsocket server, one test takes ~2 sec, when switched to uWebsockets, one test takes ~0.2sec. It's probably because of the different threading model but maybe there are some other bottlenecks in your server-side. I'm writing it to you only as feedback.

We probably stick with this solution, because your client library is easy to use and quick and their server supports all features we currently need.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/machinezone/IXWebSocket/issues/260#issuecomment-749783284, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2O6UIYDN4XNMIOIHPZLI3SWEFGNANCNFSM4VAJ4EQA.

github-actions[bot] commented 3 years ago

Stale issue message