search

improbable-eng / grpc-web

gRPC Web implementation for Golang and TypeScript
Apache License 2.0
4.39k stars 436 forks source link

grpcwebproxy websocket implementation fails on multiple subprotocols #1111

Closed niloc132 closed 2 years ago

niloc132 commented 2 years ago

Versions of relevant software used At least 0.12 through latest

What happened Websockets support a mechanism known as "subprotocols" where a connecting client can offer zero to many options, and the server can pick from that list the best suited. The server then should communicate that negotiated protocol back to the client so both sides know which protocol is in use for the duration of the connection as the websocket finishes opening, and both client and server then can proceed sending messages using that protocol.

The websocket spec calls for the Sec-Websocket-Protocol header(s) to be defined by the client (multiple headers can be set, each header entry should be comma separated). The server then must select one of these values, or respond that it does not accept these options. See https://datatracker.ietf.org/doc/html/rfc6455#section-4.1, See https://datatracker.ietf.org/doc/html/rfc6455#section-4.2 for more details.

The use case here is to implement an alternative but compatible websocket client/server (see #1081, #198) that is able to share a single websocket for all active streams. The proposed client implementation would support both grpc-websockets and grpc-websockets-multiplex. Server negotiation should inform the client which implementation will be used.

Presently, grpcwebproxy's websocket implementation when offered multiple protocols will simply fail the connection, rather than pick the acceptable option grpc-websockets.

What you expected to happen If the client websocket contains grpc-websockets as an option, it is expected that the server will respond by opening the websocket selecting that as the negotiated subprotocol. Note that the nhooyr.io/websocket library correctly handles this, but only the IsGrpcWebSocketRequest check incorrectly fails.

How to reproduce it (as minimally and precisely as possible): The simplest way is to modify client/grpc-web/src/transports/websocket/websocket.ts to also offer any other protocol along with grpc-websockets, under the assumption that any server which can use that protocol will respond back with that same string. For example:

    start: (metadata: Metadata) => {
      ws = new WebSocket(webSocketAddress, ["grpc-websockets"]);
      ws.binaryType = "arraybuffer";

Anything else we need to know I'll offer a pull request to fix this. The project github.com/deephaven/deephaven-core has an in-process grpc-web and grpc-websockets proxy, and is seeking to add a grpc-websockets-multiplex proxy for faster non-tls stream initiation (and to avoid exhausting the browser's allowed websocket count). If there is interest, we can provide a ts client here, and see about an update to the Go proxy provided by improbable-eng/grpc-web.

johanbrandhorst commented 2 years ago

Hi, thanks for your issue! This is great, thanks for all the explanation and research. Please note that this repo is in maintenance mode, and we recommend users migrate to the official grpc-web client: https://github.com/grpc/grpc-web. That said, I'm happy to review PRs that fix bugs in the existing code.

niloc132 commented 2 years ago

Thanks for the heads up - we've found the upstream grpc/grpc-web to be lacking with no binary streaming support, and in the absence of tls browsers cannot use h2, so there is a very low limit on the number of concurrent streams (not unlike with websockets). We are trying to reduce our dependence (but not our support for) the improbably-eng/grpc-web grpcwebproxy, but if we should migrate away entirely, perhaps we will need to manage our own implementation entirely.