headers icon indicating copy to clipboard operation
headers copied to clipboard

Add `SecWebsocketExtensions`

Open kazk opened this issue 4 years ago • 15 comments

I'm working on https://github.com/seanmonstar/warp/issues/770 and noticed this is missing.

kazk avatar Sep 13 '21 20:09 kazk

Does this type by itself provide any use? I imagine you need to do more with it than simply materialize an opaque value from the map.

This header is used to negotiate the usage of WebSocket extensions (extension itself and its parameters). The server needs to be able to:

  1. Iterate offer(s) (offers are separated by commas)
  2. Find a first valid offer for each extension by checking parameters in the offer (parameters are semicolon separated, the first element is the name of the extension)
  3. Include the agreed parameters for each extension in the response header

The header value depends on the WebSocket extension. As far as I know, the only standardized extension at the moment is permessage-deflate and it's used like this:

# Just the name of the extension (default parameters)
Sec-WebSocket-Extensions: permessage-deflate

# With parameters
Sec-WebSocket-Extensions: permessage-deflate; client_no_context_takeover; client_max_window_bits=10;

# With multiple sets of parameters separated by comma (the first valid set is accepted)
Sec-WebSocket-Extensions:
  permessage-deflate; client_no_context_takeover; server_max_window_bits=10,
  permessage-deflate; client_no_context_takeover; client_max_window_bits,
  permessage-deflate; client_no_context_takeover

In tungstenite (doesn't use this crate at the moment), I'm doing something naive like this:

// Parse `Sec-WebSocket-Extensions` offer/response. Doesn't support quoted values.
fn parse_header(exts: &str) -> Vec<(Cow<'_, str>, Vec<Param<'_>>)> {
    let mut collected = Vec::new();
    for ext in exts.split(',') {
        let mut parts = ext.split(';');
        if let Some(name) = parts.next().map(str::trim) {
            let mut params = Vec::new();
            for p in parts {
                let mut kv = p.splitn(2, '=');
                if let Some(key) = kv.next().map(str::trim) {
                    let param = if let Some(value) = kv.next().map(str::trim) {
                        Param::new(key).with_value(value)
                    } else {
                        Param::new(key)
                    };
                    params.push(param);
                }
            }
            collected.push((Cow::from(name), params));
        }
    }
    collected
}

#[derive(Clone, Debug, Eq, PartialEq)]
struct Param<'a> {
    name: Cow<'a, str>,
    value: Option<Cow<'a, str>>,
}

#[test]
fn test_parse_extensions_header() {
    let extensions = "permessage-deflate; client_max_window_bits; server_max_window_bits=10, permessage-deflate; client_max_window_bits";
    assert_eq!(
        parse_header(extensions),
        vec![
            (
                Cow::from("permessage-deflate"),
                vec![
                    Param::new("client_max_window_bits"),
                    Param::new("server_max_window_bits").with_value("10")
                ]
            ),
            (Cow::from("permessage-deflate"), vec![Param::new("client_max_window_bits")])
        ]
    );
}

So, maybe pub struct SecWebsocketExtensions(FlatCsv<Comma>); with some helper methods (using FlatCsv<SemiColon>)?

kazk avatar Sep 14 '21 00:09 kazk

I'd like to take over pushing permessage-deflate through the WebSockets stack.

Which review points have to be addressed in order to get this PR merged?

ilammy avatar Oct 17 '22 09:10 ilammy

@ilammy I was waiting on a response from @seanmonstar for https://github.com/hyperium/headers/pull/88#discussion_r711390478.

Once this is merged, I already have a branch using it, which is a rebased version of my original tungstenite-rs PR.

kazk avatar Oct 17 '22 22:10 kazk

@kazk @seanmonstar I'm humble ask to return to review this PR as dependent functionality in tungstenite-rs extremely important for my team, but in same time we have strict policy don't use libraries from branches, so we have to wait for finalized published version.

gallowsmaker avatar Jan 05 '23 02:01 gallowsmaker

@gallowsmaker I can't do anything for this PR, but I'll open a new PR for tungstenite-rs so we can start reviewing the new version while we wait for this. It will also clarify what needs to be done. Looks like I need to rebase again and there are some conflicts, but I should be able to open it soon.

I've been using the original version in production without any issues, but I'd love to get these merged and move on.

kazk avatar Jan 06 '23 04:01 kazk

@seanmonstar just checking if you're still able to review this?

nicknotfun avatar Feb 03 '23 19:02 nicknotfun

It would be super nice to get this merged - any hope for this to progress?

nakedible-p avatar Apr 12 '23 22:04 nakedible-p

I'm waiting

zy010101 avatar Aug 18 '23 08:08 zy010101

@seanmonstar @kazk bump. What's the blocker here? What help do you need?

ilammy avatar Nov 13 '23 12:11 ilammy

any update ? I'm waiting too ....

xxaier avatar Nov 22 '23 19:11 xxaier

I'm waiting too

AdrianEddy avatar Nov 22 '23 19:11 AdrianEddy

This PR is highly needed feature. It is blocker for us for using permessage-deflate. What can I do, to speedup merging of this?

kaszperro avatar Dec 18 '23 15:12 kaszperro

@seanmonstar are there any plans for this to be merged, or do you require any changes to be made? Getting this merged unblocks a PR in tungstenite that provides permessage-deflate support for websockets.

ddimaria avatar Feb 01 '24 18:02 ddimaria

@seanmonstar What would it take to get this merged? I'm willing to sponsor this work

AdrianEddy avatar Apr 02 '24 01:04 AdrianEddy

I know there's a lot of interest here. I've describe this a few times in chat, but never here. So, let me try:

  • This crate tries to expose built-in header types which expose as little internal details as possible. The reason for that is to try to limit mistakes that occurred when this was hyper::header in older versions, and changes to the exposed internals would mean breaking changes.
  • It's not clear whether SecWebsocketExtensions should be optimized for parsing, or for mutating.
    • That changes whether it should internally hold onto a flattened string, and parse on the fly of fn iter() -> impl Iterator, or if it should store everything parsed at construction in a list.
    • Unfortunately, that internal decision seems like it will leak, since the iterator would either provide WebsocketExtension<'a>s referring to the flat string, or &WebsocketExtension if an owned item inside a list.
    • Most asking for this feature are more interested in it just working (understandably), than the maintenance details afterwards.
  • This doesn't need to block people. This is the most important thing that is missed. Since the crate is based on a trait Header, any other code is able to declare its own typed header and impl Header for MySecWebSocketExtensions.
    • That code can start in tungstenite, or wherever else that's wanting this, and the design and performance of the type can be tested and tweaked there.
    • Once those questions are answered and tested, we can merge something here for longer term maintenance.

seanmonstar avatar Apr 02 '24 20:04 seanmonstar