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

Feature/ws over h2 #220

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from
Draft

Conversation

drymus
Copy link

@drymus drymus commented Apr 25, 2023

Description

  • Add h2 CONNECT specs
  • Update h2 binding to support CONNECT

Fixes # (issue)

.header("date", "Wed, 01 Feb 2017 19:12:46 GMT")
.build()}

write "Hello, world"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a newline at end of file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These scripts should move to rfc8441 directory.

break;
} else {
streamError = Http2ErrorCode.PROTOCOL_ERROR;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are just tracking the presence and count of the pseudo-headers, so they can be checked in decodeHeaders. So we likely need to add a count for protocol and include in the check.

Note we are validating the pseudo-headers themselves, not the values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I misread this part of the spec in rfc8441:

The :protocol pseudo-header field MUST be included in the CONNECT request, and it MUST have a value of websocket to initiate a WebSocket connection on an HTTP/2 stream

I now think it means that if we have a CONNECT request, before we create a websocket connection we must ensure that the request also contains the :protocol pseudo-header and it must have the value websocket.

Maybe we don't need to do any validation in the http2 binding in relation to a connect request.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we find a better place for it, I have added a check to allow a :protocol pseudo-header in a CONNECT request which is otherwise not allowed by rfc7540. This seems to make sense to be in the http2 binding because the binding is always advertising the ENABLE_CONNECT_PROTOCOL setting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, just make sure you are doing the simple count here, and the enforcement in decodeHeaders, aligned with approach for other checks.

.header(":status", "200")
.header("server", "CERN/3.0 libwww/2.17")
.header("date", "Wed, 01 Feb 2017 19:12:46 GMT")
.build()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use http:matchBeginEx when reading, and only include the headers that matter per spec, such as :status above.

.header("date", "Wed, 01 Feb 2017 19:12:46 GMT")
.build()}

write "Hello, world"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going with websocket handshake, then payload should either be omitted or follow websocket framing instead of Hello, world.

@drymus drymus force-pushed the feature/ws-over-h2 branch 5 times, most recently from 931ca9f to a971c15 Compare April 30, 2023 10:46
write ${client125}
write option mask [0x00 0x00 0x00 0x00]

read [0x82 0x7d] ${client125}
Copy link
Author

@drymus drymus May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect the first octet to be 0x81 for text but the server is returning 0x82 (binary). I couldn't find any tests that demonstrate receiving plain text from the engine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that text is a subset of binary, we default to binary to ensure correctness.

However, if WsDataEx on DATA frame has flags 0x81 then we send a text frame instead.

You can see this in action via ws.echo example. Sending a text frame causes a text frame to be echoed back, because the WsDataEx for the decoded text frame, is sent back from echo binding, and then ws binding encodes DATA as a text frame.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, missing newline at EOF.

Copy link
Contributor

@jfallows jfallows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, when you are done making the changes, please also test end-to-end using a local build with ws.echo example and changing values file to set the image tag to develop-SNAPSHOT, similar to mqtt.kafka.reflect example.

Looks like Chrome first shipped support for WebSocket over HTTP/2 in v91.
https://chromestatus.com/feature/6251293127475200

[0x01] # HEADERS frame
[0x04] # END_HEADERS
[0x00 0x00 0x00 0x01] # stream_id=1
[0x88] # HTTP2 status 200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at end of file.

write ${client125}
write option mask [0x00 0x00 0x00 0x00]

read [0x82 0x7d] ${client125}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that text is a subset of binary, we default to binary to ensure correctness.

However, if WsDataEx on DATA frame has flags 0x81 then we send a text frame instead.

You can see this in action via ws.echo example. Sending a text frame causes a text frame to be echoed back, because the WsDataEx for the decoded text frame, is sent back from echo binding, and then ws binding encodes DATA as a text frame.

@@ -6258,8 +6274,16 @@ private void validatePseudoHeaders(
scheme++;
break;
default:
streamError = Http2ErrorCode.PROTOCOL_ERROR;
return;
if (HEADER_NAME_PROTOCOL.equals(name.getStringWithoutLengthUtf8(0, name.capacity())))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of performing a UTF-8 decode to String then comparing to String constant, please create a DirectBuffer constant for HEADER_NAME_PROTOCOL and compare without the need for a decode.

Note: one easy way to create the right DirectBuffer constant is new String8FW(":protocol").value().

break;
} else {
streamError = Http2ErrorCode.PROTOCOL_ERROR;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, just make sure you are doing the simple count here, and the enforcement in decodeHeaders, aligned with approach for other checks.

scheme,
authority,
path)::onNetMessage;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the vastly different approaches to the handshake, suggest you avoid all the if/else logic and instead have separate Ws11Server and Ws2Server, both extending generic abstract WsServer if there is enough overlap in implementation beyond the handshake.


connected

# connection established
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at EOF.

write ${client125}
write option mask [0x00 0x00 0x00 0x00]

read [0x82 0x7d] ${client125}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at EOF.

read [0x81 0xfd]
read [0..125]

write [0x82 0x7d] ${client125}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at EOF.


connected

# connection established
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at EOF.


connected

# connection established
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at EOF.

@drymus drymus changed the base branch from develop to main May 21, 2023 09:28
@drymus drymus changed the base branch from main to develop May 21, 2023 09:29
@drymus drymus force-pushed the feature/ws-over-h2 branch 2 times, most recently from 967218e to 5821774 Compare June 1, 2023 08:34
@drymus drymus force-pushed the feature/ws-over-h2 branch 2 times, most recently from 46b0f5b to 52782f6 Compare July 5, 2023 12:56
@drymus drymus requested a review from jfallows July 5, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants