jakartaee / websocket

Jakarta WebSocket
https://projects.eclipse.org/projects/ee4j.websocket
Other
62 stars 43 forks source link

How to handle missing extensions? #340

Open markt-asf opened 4 years ago

markt-asf commented 4 years ago

The process of negotiating handshakes is clearly defined in section 3.1.3

In the opening handshake, the client supplies a list of extensions that it would like to use. The default server configuration selects from those extensions the ones it supports, and places them in the same order as requested by the client.

What isn't discussed is what happens if ClientEndpointConfig#getExtensions() returns one or more extensions that the client side code does not recognise. An equivalent question applies to ServerEndpointConfig#getExtensions().

The TCK currently assumes that if the ClientEndpointConfig or ServerEndpointConfig declares an extension, it will be supported. The TCK then defines 3 random extensions and assumes both the client and the server will ignore the fact they don't recognise the extensions (since they can't possibly support them). Assuming that an unknown extension will be ignored could be problematic. If a genuine extension is declared by both the client and server and those endpoints are deployed in an environment where only one side recognises the extension it is likely that attempts to use the endpoints will fail as one endpoint tries to use an extension the other has declared support for but doesn't actually understand.

I'm currently thinking through what a fix for this might look like. I suspect it will have to wait for Jakarta EE 10 but maybe not.

joakime commented 4 years ago

I would like to think that what is declared in your annotations is unconnected to what is available on the container.

So declaring, for example, "permessage-deflate" means you would like to use it, but not mandate it's usage.

For a Client Endpoint it offers that extension on the handshake only if the container supports it.

For a Server Endpoint it negotiates that extension on the handshake only if the container supports it AND the client offered it on the handshake.

markt-asf commented 4 years ago

Hmm. The extension registry only has two entries. Extensions haven't really taken off the way they might have done. Not really the topic of this issue but in terms of context I don't think the number of extensions justifies extending the WebSocket API to provide a standard API for extension implementations.

I would argue that extensions are inherently optional. The WebSocket handshake includes the possibility that no extensions will be agreed. If an endpoint wants to require an extension they can intercept the handshake, examine the agreed extensions and cancel the connection if they don't like what they see.

Extensions cannot currently be defined in @ClientEndpoint or @ServerEndpoint although #327 looks to change that.

My current thinking is that we make the following additions to the Jakarta WebSocket specification:

Assuming there is consensus for the above or something like it, I think updating some of the existing tests in the TCK is going to be tricky.

joakime commented 4 years ago

Extensions have a few different view points with slightly different behavior.

Client

From a Client Endpoint point of view. A developer would need to know ...

For the list of extensions available on the container, some new APIs to interrogate the list of registered / available extensions by name would be enough.

For the client handshake request, it needs to have the ability to include a list extension configurations (not just names, but extensions with configuration parameters). That can be declared in @ClientEndpoint annotations, in ClientEndpointConfig.Configurator, and even as a parameter in the WebSocketContainer.connect() methods. It should also be possible to declare a "default" list of extension configs to use during Client connect regardless of the Client Endpoint being used. (This seems like a good fit for a WebSocketContainer API) If we allow multiple of them to be used, we need to be careful to specify how the order of extensions on the list.

We also need the client side to pay attention to the handshake response from the server. If the server responded with a handshake but was missing a required extension, the client should terminate with close status code 1010. (aka "required extension")

The "forbidden extension" behavior of a client endpoint would be a declaration by that endpoint to never offer a specific extension on the handshake request, even if the container supports it, even if the container level configuration says to offer it.

Server

From a Server Endpoint point of view it needs to know.

For the list of extensions available on the container, some new APIs to interrogate the list of registered / available extensions by name would be enough.

For the server handshake request, it needs a variety of behaviors.

Protocol

From a protocol point of view, the client extensions offer can include multiple entries for a single named extension, but with different configurations. The offered list order is important. It's up to the server to select which of the offered extensions it's going to use.

markt-asf commented 3 years ago

Coming back to this as it is an open bug I'd like to try and close for 2.1.

While I agree with @joakime's view of the complete set of required functionality, I think the very small number of extensions - really just permessage-deflate - means a smaller scale change to the API should meet the significant majority of user requirements.

I am currently thinking along these lines:

How to install support for an extension for both the client and the server would be container specific.

I can put together a PR for this but I wanted to gather some feedback before I did so.

joakime commented 3 years ago

Jetty has multiple extensions available btw.

Jetty dropped perframe-compress though, it was barely supported.

The autobahn websocket library (used for the official websocket support TCK) also supports ...

joakime commented 3 years ago

If you look around, you'll also find the following PMCE implementations already present in various websocket libraries.

markt-asf commented 3 years ago

Thanks, I wasn't aware of that. Tomcat hasn't had any requests (that I recall) to implement any of those.

I'm not sure to what extent this complicates matters. My reading of RFC 7692 is that the server should accept the first PMCE that it can support and ignore the others. If that is extended to all compression extensions then I think my proposal could still work. And there remains the option to customize ServerEndpointConfig.Configurator.getNegotiatedExtensions() if necessary.

I guess the question is does adding something alone the lines of my proposal meet enough use cases to make it worth adding to the 2.1 spec? Additional configuration such as extensions that should always be offered regardless of what is configured for the endpoint would remain container specific.

markt-asf commented 1 year ago

Any objections to proceeding along the lines I set out on 2021-10-07, a different approach or would we prefer to drop this issue?

joakime commented 1 year ago

@markt-asf there are many messages on here from 2021-10-07, are you referring to proposal with the following ?

markt-asf commented 1 year ago

Yes

joakime commented 1 year ago
  • New annotations on the client side to mirror ClientEndpointConfig.getExtensions(). Clarify that:

    • multiple extensions may be requested and
    • multiple configurations for the same extension may be requested with the most preferred config acceptable to the server being used

Mostly correct. The Client cannot know what the "most preferred config" of the server is. Or are you just paraphrasing the normal websocket handshake process that is outlined by the spec? https://datatracker.ietf.org/doc/html/rfc6455#section-9.1

  • An request for an extension and associated configuration will only be included in the opening handshake of the extension is included in WebSocketContainer#getInstalledExtensions()

:+1: but this check is only performed by-name (the configuration parameters are not considered for this step)

  • New annotation on the server side to mirror ServerEndpointConfig.getExtensions().

I don't see the value of this in an Annotation. How do you see this being used? Can you give an example?

  • Clarify that parameters are ignored on the server side. Configuration of acceptable ranges and/or combinations of parameters is container specific.

The first part reads poorly, or can be interpreted to mean things we don't want it to mean. Parameters are not considered during can-extension-be-used steps.

The server side extensions can be viewed as a map of Extension name to Extension parameters.

Replace "Configuration of acceptable ranges and/or combinations of parameters is container specific." with "Determination of acceptable parameters is Extension specific."

We have a few things to think about here.

  1. Is the configuration well formed?
    Eg: bad config permessage-deflate; x_mode=widget vs a good one permessage-deflate or permessage-deflate; client_max_window_bits.
  2. Is the configuration provided supported during the handshake? a client asks for permessage-deflate; client_max_window_bits during handshake, but the server doesn't support it and drops it from the handshake. (is this extension required by the client, and should the connection be failed by the client? or is the client ok with the extension being optional?) or the server supports a limited sub-set so it returns only permessage-deflate per the permessage-deflate spec. (this is the extension making this call, not the client nor server)

If the server side receives an extension configuration during handshake stage the that is not well formed, then the implementation MUST fail the connection per https://datatracker.ietf.org/doc/html/rfc6455#section-9.1 But if the ServerEndPointConfig has such a situation, then what? I favor a DeploymentException. The ClientEndpointConfig could only trigger an exception during handshake, as the configured extensions are only considered/evaluated during handshake.

  • The list of installed extensions passed to ServerEndpointConfig.Configurator.getNegotiatedExtensions() will be the union of the extensions defined for the endpoint and ServerContainer#getInstalledExtensions()

Sure, but only via the name of the extension, parameters in a configuration are not considered for this union.

joakime commented 1 year ago
  • multiple configurations for the same extension may be requested with the most preferred config acceptable to the server being used

Also, when it comes to multiple configurations of the same extension, this is controlled by the extension. It is not something that a higher level codebase (like in the server) can do anything about. The various extension specs detail how to handle multiple configurations.

Now that I think about it, the Extension developers will need to know prior Extensions in the negotiation step. (eg: permessage-deflate is fine with multiple configurations being offered by the client, but the client doesn't like having multiple negotiations be present) The jakarta websocket implementations probably need to be more careful about multiple extensions declaring a need for the same RSV bit too.

markt-asf commented 1 year ago

Mostly correct. The Client cannot know what the "most preferred config" of the server is. Or are you just paraphrasing the normal websocket handshake process that is outlined by the spec? https://datatracker.ietf.org/doc/html/rfc6455#section-9.1

I was just paraphrasing. We can reference the RFC in the updated spec/Javadoc langauge.

  • New annotation on the server side to mirror ServerEndpointConfig.getExtensions().

I don't see the value of this in an Annotation. How do you see this being used? Can you give an example?

Having looked at it again, I don't see any value either. I'll drop this.

  • Clarify that parameters are ignored on the server side. Configuration of acceptable ranges and/or combinations of parameters is container specific.

The first part reads poorly, or can be interpreted to mean things we don't want it to mean. Parameters are not considered during can-extension-be-used steps.

The server side extensions can be viewed as a map of Extension name to Extension parameters.

Replace "Configuration of acceptable ranges and/or combinations of parameters is container specific." with "Determination of acceptable parameters is Extension specific."

Will do.

We have a few things to think about here.

  1. Is the configuration well formed? Eg: bad config permessage-deflate; x_mode=widget vs a good one permessage-deflate or permessage-deflate; client_max_window_bits.
  2. Is the configuration provided supported during the handshake? a client asks for permessage-deflate; client_max_window_bits during handshake, but the server doesn't support it and drops it from the handshake. (is this extension required by the client, and should the connection be failed by the client? or is the client ok with the extension being optional?) or the server supports a limited sub-set so it returns only permessage-deflate per the permessage-deflate spec. (this is the extension making this call, not the client nor server)

If the server side receives an extension configuration during handshake stage the that is not well formed, then the implementation MUST fail the connection per https://datatracker.ietf.org/doc/html/rfc6455#section-9.1 But if the ServerEndPointConfig has such a situation, then what? I favor a DeploymentException. The ClientEndpointConfig could only trigger an exception during handshake, as the configured extensions are only considered/evaluated during handshake.

Happy with DeploymentException if the configuration is not well-formed.

If the configuration requested by the client isn't supported by the server then the extension is not used unless the extension specifies different behaviour.

If the client requires a configuration the server doesn't support then the client can fail the connection during Configurator.afterRespnse().

  • The list of installed extensions passed to ServerEndpointConfig.Configurator.getNegotiatedExtensions() will be the union of the extensions defined for the endpoint and ServerContainer#getInstalledExtensions()

Sure, but only via the name of the extension, parameters in a configuration are not considered for this union.

Ack.

I think I have enough to draft a PR. I'll try and work on that today.