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

How to handle missing extensions? #340

Open
markt-asf opened this issue May 14, 2020 · 13 comments
Open

How to handle missing extensions? #340

markt-asf opened this issue May 14, 2020 · 13 comments
Labels
API (Both) Impacts the client and server API bug Something isn't working
Milestone

Comments

@markt-asf
Copy link
Contributor

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
Copy link
Contributor

joakime commented May 14, 2020

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
Copy link
Contributor Author

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:

  • When creating the opening handshake, any extension specified in the ClientEndpointConfig MUST only be sent to the server if it also appears in WebSocketContainer#getInstalledExtensions().
  • When calling ServerEndpointConfig.Configurator#getNegotiatedExtensions() the list of installed extensions passed to that method MUST be the extensions specified in the ServerEndpointConfig filtered so only those also listed in ServerContainer#getInstalledExtensions() are passed in.

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.

@markt-asf markt-asf added API (Both) Impacts the client and server API bug Something isn't working Jakarta EE 10 labels May 14, 2020
@joakime
Copy link
Contributor

joakime commented May 14, 2020

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

  • is the Extension available on the container?
  • is the handshake request going to include that extension on it's handshake offering?

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.

  • is the Extension it received on the handshake offering available on the container?
  • should I negotiate that extension if offered? (if yes, under what extension specific configuration?)

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.

  • Extensions that belong to the container and are negotiated on behalf of all Server endpoints.
  • Extensions that belong to the ServerEndpoint and are negotiated on behalf of those specific Server endpoints.
  • Extensions that belong to the ServerEndpointConfig and are negotiated on behalf of those specific Server endpoints.
  • Extensions that must never be negotiated, even if offered and the container supports it. (aka "forbidden extension")
  • Extensions that must always be offered, else the connection is terminated. (aka "required extension")

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
Copy link
Contributor Author

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:

  • New annotations on the client side to mirror ClientEndpointConfig.getExtensions(). Clarify that a) multiple extensions may be requested and b) multiple configurations for the same extension may be requested with the most preferred config acceptable to the server being used
  • An request for an extension and associated configuration will only be included in the opening handshake of the extension is included in WebSocketContainer#getInstalledExtensions()
  • New annotation on the server side to mirror ServerEndpointConfig.getExtensions()`.
  • Clarify that parameters are ignored on the server side. Configuration of acceptable ranges and/or combinations of parameters is container specific.
  • The list of installed extensions passed to ServerEndpointConfig.Configurator.getNegotiatedExtensions() will be the union of the extensions defined for the endpoint and ServerContainer#getInstalledExtensions()

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
Copy link
Contributor

joakime commented Oct 7, 2021

Jetty has multiple extensions available btw.

  • permessage-deflate (official compress extension, this is spec https://datatracker.ietf.org/doc/html/rfc7692 implementation of the deflate compression technique)
  • deflate-frame (transitional compress extension, still seen in older browsers, and mobile clients)
  • x-webkit-deflate-frame (original compression extension, still seen in many mobile browsers, and older websocket clients)
  • fragment (auto-fragment extension, seen in many websocket implementations)
  • identity (a no-op extension used to test various extension behaviors)
  • @debug (internal local-side only debug extension)
  • @capture (internal local-side capture of raw frames to disk)

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

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

  • permessage-bzip2
  • permessage-compress
  • permessage-deflate

@joakime
Copy link
Contributor

joakime commented Oct 7, 2021

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

  • permessage-lz4
  • permessage-snappy

@markt-asf
Copy link
Contributor Author

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 markt-asf added this to the backlog milestone May 4, 2022
@markt-asf
Copy link
Contributor Author

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
Copy link
Contributor

joakime commented Nov 17, 2023

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

  • 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
  • An request for an extension and associated configuration will only be included in the opening handshake of the extension is included in WebSocketContainer#getInstalledExtensions()
  • New annotation on the server side to mirror ServerEndpointConfig.getExtensions().
  • Clarify that parameters are ignored on the server side. Configuration of acceptable ranges and/or combinations of parameters is container specific.
  • The list of installed extensions passed to ServerEndpointConfig.Configurator.getNegotiatedExtensions() will be the union of the extensions defined for the endpoint and ServerContainer#getInstalledExtensions()

@markt-asf
Copy link
Contributor Author

Yes

@joakime
Copy link
Contributor

joakime commented Nov 17, 2023

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

👍 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
Copy link
Contributor

joakime commented Nov 17, 2023

  • 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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API (Both) Impacts the client and server API bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants