Allow passing of metadata to websocket transport upgrade request

[ngrigoriev]

Version

0.12.0

What happened

URL with the query string at the end got the protobuf package/service/method appended at the end

wss://localhost:7778/ws?access_token=XXXXXXX/hello.HelloService/SayHello

Expected

URL would be constructed correctly, e.g:

wss://localhost:7778/ws/hello.HelloService/SayHello?access_token=XXXXXXX

How to reproduce it (as minimally and precisely as possible):

       grpc.invoke(HelloService.SayHello, {
        request :helloRequest,
        host : "https://localhost:7778/ws?access_token=XXXXXXX",

What I was trying to do: I was exploring the possibilities of passing the OAuth2 token as query parameter, to be consumed by the API Gateway fronting the grpcwebproxy.

I believe that client.ts does somewhat optimistic URL building:

    const url = `${this.props.host}/${this.methodDefinition.service.serviceName}/${this.methodDefinition.methodName}`;

I understand that "host" is normally expected to be the host, but, in fact, everywhere in the code and the documentation it looks like a URL, e.g. schema://host:port at least.

Thus, I believe, it won't hurt if instead of simple concatenation, the value of this.props.host is first parsed as URL, then the path it modified, then the final URL is serialized.

[johanbrandhorst]

Hi Nikolai,

Thanks for your bug report. I think we should take a step back and find a different solution to this than your proposed one. There is already a well defined method for passing arbitrary data down from the client to the server, and that is with grpc metadata. See https://github.com/improbable-eng/grpc-web/blob/master/client/grpc-web-react-example/ts/_proto/examplecom/library/book_service_pb_service.d.ts#L65 for an example of how to pass this information to the server. I do not think we want to make query parameters globally configurable like this. Please consult the grpc-web spec for more information: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md.

[ngrigoriev]

I disagree. Metadata works fine for anything BUT websocket protocol. In case of websocket, as far as I understand, the gRPC metadata is passes after the connection upgrade. You are ignoring the fact that the typical deployment pattern is to use an API Gateway (any one) and usually it is the API Gateway that takes care of the cross-service tasks, including authorization. As long as there is a way to pass the bearer token in the original HTTP request handled by the API Gateway, I would be happy. By Websocket API does not leave too many options.

API Gateway would strip these values and grpcwebproxy would receive the request it expects.

The specification you are pointing to does not explicitely forbid, I think, using of the query strings. It should ignore them, of course. Just like it ignores, for example, the headers it does not understand. Just like it ignores the host name.

I still suggest to fix this code and at least make it correct from common practice point of view, URLs are not constructed by concatenating the strings.

There is a difference between the Websocket endpoint on the client side (URL that the browser connects to) and the API exposed by grpcwebproxy. They do not have to be absolutely the same, as long as the RFC compliance is preserved.

By the way, even today I can use a prefix (like "/ws") on my API Gateway to have a separate endpoint for Websocket access. With different rules etc. And the gateway strips off the prefix when forwarding this request to the upstream (grpcwebproxy).

[johanbrandhorst]

Thanks for giving some more information, the question of how to add parameters to websocket connections is interesting, and something we could try to solve. Note that the websocket transport is non-standard and may not be suitable for production use. In general though, for something that is unique to the websocket, I think we should add some parameters to the websocket transport constructor, rather than compromising the general design.

In general I also think you're abusing the meaning of the "host" variable. It is formatted as a URL, yes, but that doesn't mean that it should be used for anything other than specifying a host. It is not a generic URL field. A host in a URL does not include any path elements, so it is fine to assume that it should be just mywebsite.com or https://mywebsite.com. If anything, we should disallow the use of a scheme in that parameter.

I will reopen this issue to discuss the possibility of adding a parameter to the websocket constructor to pass metadata to the upgrade request.

[ngrigoriev]

Well, it is non-standard but it exists and we have developers who insist on using it, so we need to adapt with the lowest possible cost and in the way that can be supported by the api gateway.

FYI, I am using Ambassador. It has an external authorizer support which, essentially, gets the request headers (in case of websocket - the original HTTP request with "upgrade"). And the authorizer knows how to extract the bearer token from a couple of places: HTTP header (preferred), cookie or a query parameter.

Websocket API does not allow to set any headers (except the protocol one - this is the only alternative, which is also an abuse). So, the only way is to use the URL.

Now, considering that the API Gateway is a known pattern and the gateways usually manipulate the request host/path/etc, I think this is a "valid abuse" - as long as the developer knows what are they doing and as long as they supply the compliant header to grpcwebproxy. I am not calling for changing the protocol or altering grpcwebproxy server - just allowing all valid RFC options for the Websocket endpoint. Endpoint is not necessairy grpcwebproxy itself, can be something fronting it :)

Thanks for considering the alternatives!

[johanbrandhorst]

Now that we've established an angle of attack, would you be interested in implementing this support in the websocket transport? I still think we should try to limit this to the websocket transport if that's the only place we're going to be using it. We could have something that automatically appends a query parameter to the outgoing requests if an option is specified.

[ngrigoriev]

I can try. I would probably not try to bring in the direct mentioning of the "query string" to the API, this is too much. Here is a silly idea. Maybe we can have a way for the transport to have one more opportunity to rewrite the endpoint URL before the client uses it. This way, I could even extend Websocket transport in my own code to WebsocketGatewayTransport (or something like it) and there I would do whatever I need.

[johanbrandhorst]

That sounds fine, but I'd still like it to be a websocket transport parameter, not a generic transport parameter. Does that sound okay?

[ngrigoriev]

Why not, just need to figure out how to make it elegant with Typescript. I use several languages, but Typescript is not one of them :)

Another option would be, in fact, more aligned with what I proposed in the first place. Make "host" a URL, make the Client code more universal by parsing/modifying/serializing the URL. And then enforce the URL format (only schema, host and port) in all transport implementations BUT Websocket one.

[johanbrandhorst]

Any changes to the existing parsing has to be backwards compatible, but what you're proposing sounds like it will just add some extra validation over the current solution. Still think it should be an option, not a matter of disabling the validation in the websocket transport.

[ngrigoriev]

Just keeping this open, I am planning to propose a change soon.

[krish-gh]

@johanbrandhorst wanted to check if you also tried with official https://github.com/grpc/grpc-web package and saw same behaviour?

[johanbrandhorst]

Not sure what you're asking example? The official implementation has no websocket support.

[krish-gh]

Ok, thanks. So just to confirm my understanding, is the addition of experimental websocket in Improbable version is to support bi-directional streaming (only or there is more)? The reason I am asking because firstly, I could not find mention of websoket support in readme and secondly, I am not very clear here about why the OP is trying websocket. Only purpose I could think of is bi-directional or client streaming which grpc-web does not support over http.

Also, I have a customer pointed this particular github issue to be fixed, but the motivation might be different.

[johanbrandhorst]

Yes, the websocket support is only there for bi-directional streaming.

[ngrigoriev]

This may be related to https://github.com/improbable-eng/grpc-web/issues/796 as well, I think an alternative transport would deal with these purely transport-specific requirements.