http3: adding connect support#17877
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Looks good! I have a few questions around the semantics we're supporting.
| google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2; | ||
|
|
||
| // Allows proxying Websocket and other upgrades over HTTP/3 connect. | ||
| bool allow_upgrade_connect = 5; |
There was a problem hiding this comment.
To confirm my interpretation of this config this only allows RFC 8441 style CONNECT which requires both :path and :protocol. It does not enable RFC 7540 style CONNECT (without :path or :protocol). RFC 8441 requires SETTINGS_ENABLE_CONNECT_PROTOCOL be received by the client before sending the extended CONNECT. Presumably Envoy sends this setting? I wonder if we should call out the RFCs here (and possibly in the corresponding HTTP/2 config). What do you think?
All of this being said, RFC 8841 is an extension to HTTP/2 not an extension to HTTP/3. HTTP/2 settings are not automatically valid for HTTP/3.
Settings need to be defined separately for HTTP/2 and HTTP/3. The IDs of settings defined in [HTTP2] have been reserved for simplicity. Note that the settings identifier space in HTTP/3 is substantially larger (62 bits versus 16 bits), so many HTTP/3 settings have no equivalent HTTP/2 code point. See Section 11.2.2.
I think we need to write a doc describing how to do port RFC 8841 to HTTP/2. (Similar docs need to be written for the ORIGIN and ALT_SVC frames, fwiw. https://www.ietf.org/archive/id/draft-bishop-httpbis-altsvc-quic-01.html ) In particular we need to define a value for SETTINGS_ENABLE_CONNECT_PROTOCOL in HTTP/3. I've started that process here:
and will work with httpbis folks to push this forward.
There was a problem hiding this comment.
yeah we could just land standard CONNECT support but I think folks are going to want websocket over h3 if they have it for H2.
I'll link to the H2 spec and your draft and tag it alpha. Alternately I can just add support for standard connect (block anything with path, regardless of protocol) and wait on the RFC. thoughts?
There was a problem hiding this comment.
I think either approach is fine, but I share your intuition that folks will want H3 websockets if they have H2 websockets. I think linking to the draft and tagging it as alpha is fine. BTW, here's the IETF url for the draft https://datatracker.ietf.org/doc/html/draft-hamilton-httpbis-h3-websockets if you want to link to it.
source/common/http/header_utility.cc
Outdated
| headers.Path() && !headers.Protocol()) { | ||
| // Path header should only be present for CONNECT for upgrade style CONECT. | ||
| return absl::InvalidArgumentError( | ||
| absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get())); |
There was a problem hiding this comment.
Should this be Protocol.get() instead of Host.get()?
There was a problem hiding this comment.
yep, nice catch.
source/common/http/header_utility.cc
Outdated
| absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get())); | ||
| } | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.validate_connect") && | ||
| headers.Path() && !headers.Protocol()) { |
There was a problem hiding this comment.
It looks like scheme is also required, interestingly. Should we check for that too?
o On requests that contain the :protocol pseudo-header field, the
:scheme and :path pseudo-header fields of the target URI (see
Section 5) MUST also be included.
There was a problem hiding this comment.
From this code it looks like we allow CONNECT here without protocol (and hence without path). Is that right? If so, my earlier assumption that we only supported 8841 is definitely wrong :)
There was a problem hiding this comment.
scheme is default required for all HTTP/3 so I believe is validated elsewhere.
Path is required in the HCM unless it's CONNECT, but all envoy route matching asserts a path except for the special connect matchers, so Envoy will 404 unless you explicitly try to support CONNECT.
There was a problem hiding this comment.
The :scheme header is prohibited from vanilla CONNECT requests in HTTP/3:
A CONNECT request MUST be constructed as follows:
* The ":scheme" and ":path" pseudo-header fields are omitted
Is something validating that :scheme is present for CONNECT requests elsewhere? If so, is that perhaps incorrect?
There was a problem hiding this comment.
Is !headers.Path() && headers.Protocol() also invalid?
There was a problem hiding this comment.
Protocol and path MUST both be sent for extended CONNECT. So if one is sent but not the other, yes, I think that's invalid,
There was a problem hiding this comment.
fixed and tested.
| if (Http::HeaderUtility::requestHeadersValid(*headers) != absl::nullopt) { | ||
| if (Http::HeaderUtility::requestHeadersValid(*headers) != absl::nullopt || | ||
| Http::HeaderUtility::checkRequiredRequestHeaders(*headers) != Http::okStatus() || | ||
| (headers->Protocol() && !http3_options_.allow_upgrade_connect())) { |
There was a problem hiding this comment.
I could be reading this wrong, but it looks like if the request method is CONNECT but it doesn't have the :protocol header, we will consider the request valid. Is that right?
There was a problem hiding this comment.
yeah CONNECT either has to have no path, or path + protocol.
The former is a standard CONNECT request, and will be blocked at the HCM unless there's CONNECT-specific route matchers (so off by default). The latter gets transformed into an HTTP/1.1 style upgrade, and won't look like it's a CONNECT for the duration of the filter chain, will match normal routes so is disabled by default using a separate knob.
https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/upgrades
which now that I think about it I should update.
test/config/utility.cc
Outdated
| hcm.add_upgrade_configs()->set_upgrade_type("CONNECT"); | ||
| hcm.mutable_http2_protocol_options()->set_allow_connect(true); | ||
| if (http3) { | ||
| hcm.mutable_http3_protocol_options()->set_allow_upgrade_connect(true); |
There was a problem hiding this comment.
It looks like the h2 and h3 option are named differently:
set_allow_connect()
set_allow_upgrade_connect()
Would it make sense to use the same name in both places?
There was a problem hiding this comment.
I think the H2 option is (sadly) misnamed because HTTP/2 allows standard connect without a special knob, and just disallows the upgrade style connect.
danzh2010
left a comment
There was a problem hiding this comment.
Thanks for working on this feature! I'm also confused about what the expected CONNECT request should look like. Did we document it for H2 somewhere?
source/common/http/header_utility.cc
Outdated
| } | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.validate_connect") && | ||
| headers.Path() && !headers.Protocol()) { | ||
| // Path header should only be present for CONNECT for upgrade style CONECT. |
|
Also I owe you sending that setting with HTTP/3 and how about a rename from upgrade_connect to extended_connect? |
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Also I owe you sending that setting with HTTP/3 and how about a rename from upgrade_connect to extended_connect?
Sounds perfect to me. That matches the IETF terminology for this H3 behavior and since this is an H3 protocol option, this seems like the right terminology.
| google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2; | ||
|
|
||
| // Allows proxying Websocket and other upgrades over HTTP/3 connect. | ||
| bool allow_upgrade_connect = 5; |
There was a problem hiding this comment.
I think either approach is fine, but I share your intuition that folks will want H3 websockets if they have H2 websockets. I think linking to the draft and tagging it as alpha is fine. BTW, here's the IETF url for the draft https://datatracker.ietf.org/doc/html/draft-hamilton-httpbis-h3-websockets if you want to link to it.
source/common/http/header_utility.cc
Outdated
| absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get())); | ||
| } | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.validate_connect") && | ||
| headers.Path() && !headers.Protocol()) { |
There was a problem hiding this comment.
The :scheme header is prohibited from vanilla CONNECT requests in HTTP/3:
A CONNECT request MUST be constructed as follows:
* The ":scheme" and ":path" pseudo-header fields are omitted
Is something validating that :scheme is present for CONNECT requests elsewhere? If so, is that perhaps incorrect?
|
hm, I actually think I can't set custom settings. @danzh2010 am I right in thinking this will need an upstream quiche change given FillSettingsFrame seems to be the way to change settings and doesn't have connect support? |
|
@alyssawilk yes that'll require a QUICHE change. I would suggest that if we change QUICHE to send that h3 setting, then QUICHE should also be validating that CONNECT requests carry the right pseudo-headers. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Is this currently gated on a Quiche change to support the HTTP/3 setting?
| upstream server functionally intact, which means it needs to traverse the HTTP/2+ hop. | ||
|
|
||
| This is accomplished via `Extended CONNECT (RFC8441) <https://tools.ietf.org/html/rfc8441>`_ support, | ||
| This is accomplished for HTTP/1 via `Extended CONNECT (RFC8441) <https://tools.ietf.org/html/rfc8441>`_ support, |
There was a problem hiding this comment.
Nit: RFC 8841 defines Extended CONNECT for HTTP/2, not HTTP/1. The analogous HTTP/1 mechanism is Upgrade https://datatracker.ietf.org/doc/html/rfc7230#section-6.7
|
/wait |
|
it's not gated on the settings given it works without them, but I added a TODO to follow through on that part. |
Ah, ok that SGTM. Presumably no clients will make use of this behavior without the setting being sent but no need to wait for that. LGTM |
danzh2010
left a comment
There was a problem hiding this comment.
it's not gated on the settings given it works without them, but I added a TODO to follow through on that part.
@danzh2010 would you be willing to do the upstream work? I suspect you can do it more easily than I can :-/
Yeah, I can add the custom setting in QUICHE. Should QUICHE also reject the CONNECT request if the setting is not sent or leave it to Envoy? If the former, we might break some envoy deployment which uses CONNECT without receiving the special setting as how it works now.
source/common/http/header_utility.cc
Outdated
| absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get())); | ||
| } | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.validate_connect") && | ||
| headers.Path() && !headers.Protocol()) { |
There was a problem hiding this comment.
Is !headers.Path() && headers.Protocol() also invalid?
| } else { | ||
| cluster->http3_options_ = ConfigHelper::http2ToHttp3ProtocolOptions( | ||
| http2_options.value(), quic::kStreamReceiveWindowLimit); | ||
| cluster->http3_options_.set_allow_extended_connect(true); |
There was a problem hiding this comment.
Does this config on client stream has any effect?
There was a problem hiding this comment.
I assume it'll result in sending the SETTINGS when we have it.
There was a problem hiding this comment.
I thought the ENABLE_CONNECT_PROTOCOL setting was supposed to be sent by server. So the the knob should only be used by server code. No?
There was a problem hiding this comment.
I'd imagine that on the client we'd use this option to fail the extended connect before it's sent? Or is it a no-op on the client?
| bool allow_post_{}; | ||
| }; | ||
|
|
||
| TEST_P(ConnectTerminationIntegrationTest, OriginalStyle) { |
There was a problem hiding this comment.
This test is almost identical with the following Basic and UsingHostMatch tests other than a few settings before initialize(). Can the shared part be extracted into a helper function?
| ASSERT_TRUE(fake_raw_upstream_connection_->close()); | ||
| ASSERT_TRUE(response_->waitForReset()); | ||
| if (downstream_protocol_ == Http::CodecType::HTTP3) { | ||
| // In HTTP/3 the end stream will be sent when the connection is close, and |
There was a problem hiding this comment.
Does this comment mean when the upstream connection is closed, the HCM will send an early response with end_stream set before calling resetStream() in this case?
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Yeah, I can add the custom setting in QUICHE. Should QUICHE also reject the CONNECT request if the setting is not sent or leave it to Envoy? If the former, we might break some envoy deployment which uses CONNECT without receiving the special setting as how it works now.
I think it would be fine to reject malformed CONNECT requests. When the setting is not present it's malformed to have path, scheme or protocol. When the setting is present it's malformed to have path or protocol absent. I think CONNECT + H3 must not yet be used in production so I think it won't cause problems. But Alyssa would know for sure.
source/common/http/header_utility.cc
Outdated
| absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get())); | ||
| } | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.validate_connect") && | ||
| headers.Path() && !headers.Protocol()) { |
There was a problem hiding this comment.
Protocol and path MUST both be sent for extended CONNECT. So if one is sent but not the other, yes, I think that's invalid,
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@mattklein123 I think this is ready for maintainer pass! |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM with some small API/doc questions.
/wait-any
| // <https://datatracker.ietf.org/doc/html/rfc8441>`_ | ||
| // and settings `proposed for HTTP/3 | ||
| // <https://github.com/RyanTheOptimist/httpbis-h3-websockets/blob/main/draft-hamilton-httpbis-h3-websockets.md>`_ | ||
| // [#alpha:] as HTTP/3 CONNECT is not yet an RFC. |
There was a problem hiding this comment.
I'm going to eventually convert this into an annotation. I assume that for a primitive type, if it's set to something other than default, I can determine that the annotation is set in that case but not if it's left to the default?
Basically I'm wondering if we need to make this a WKT vs. a primitive or not for fields. cc @envoyproxy/api-shepherds who know more about proto.
There was a problem hiding this comment.
I'd think it'd be fine if for no other reason than setting it false is the same as not-present and doesn't need an alpha tag. it's really only the behavior when this is explicitly set true that may change depending on the RFC
There was a problem hiding this comment.
OK sounds good. I will see if this case works OK when I implement the feature.
There was a problem hiding this comment.
Yeah, I think we do need in general to make WIP annotated things WKT/optional/messages when the default value is material, but if their absence is a no-op then we can skip.
| "envoy.reloadable_features.unquote_log_string_values", | ||
| "envoy.reloadable_features.upstream_host_weight_change_causes_rebuild", | ||
| "envoy.reloadable_features.use_observable_cluster_name", | ||
| "envoy.reloadable_features.validate_connect", |
There was a problem hiding this comment.
Should this be documented somewhere?
| // the header mechanisms from the `HTTP/2 extended connect RFC | ||
| // <https://datatracker.ietf.org/doc/html/rfc8441>`_ | ||
| // and settings `proposed for HTTP/3 | ||
| // <https://github.com/RyanTheOptimist/httpbis-h3-websockets/blob/main/draft-hamilton-httpbis-h3-websockets.md>`_ |
There was a problem hiding this comment.
Now that the IETF HTTP Working Group has adopted this document, the best link would now be
https://datatracker.ietf.org/doc/draft-ietf-httpbis-h3-websockets/
Mind switching it?
DavidSchinazi
left a comment
There was a problem hiding this comment.
Adding some random thoughts. Sorry I'm late to the party, extended CONNECT just became a dependency for privacy proxies and I saw comments on that fly by :)
| absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get())); | ||
| } | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.validate_connect")) { | ||
| if (headers.Path() && !headers.Protocol()) { |
There was a problem hiding this comment.
As per the spec, when :protocol is included, both :path and :scheme are required. Should we validate both?
There was a problem hiding this comment.
:scheme is required for all non-CONNECT requests and is supposed to be done prior to this point as we expect core protocol invariant to be enforced by the codec (this is pre-RFC and extension so less so for this). If QUICHE isn't doing that enforcement, I'd argue it probably should so thanks for volunteering to pick it up below.
| headers_with_underscores_action_(headers_with_underscores_action) { | ||
| ASSERT(static_cast<uint32_t>(GetReceiveWindow().value()) > 8 * 1024, | ||
| "Send buffer limit should be larger than 8KB."); | ||
| // TODO(alyssawilk, danzh) if http3_options_.allow_extended_connect() is true, |
There was a problem hiding this comment.
FWIW I'm planning on adding support for this SETTING in QUICHE for Privacy Proxy, I plan on always sending it and validating the presence of pseudo-headers in QUICHE
There was a problem hiding this comment.
Thanks for taking this on. We should perhaps discuss this on the QUICHE PR/CL, but I'm not convinced that always sending the setting is the right choice. But let's discuss there.
There was a problem hiding this comment.
@DavidSchinazi Actually I'm already working on a QUICHE change to add this setting per previous discussion. I can take this feature if you don't mind?
There was a problem hiding this comment.
@danzh2010 that's fine by me! From Alyssa's other comment I think QUICHE should validate that the correct headers are present. Please add me as a reviewer on the change
| } else { | ||
| cluster->http3_options_ = ConfigHelper::http2ToHttp3ProtocolOptions( | ||
| http2_options.value(), quic::kStreamReceiveWindowLimit); | ||
| cluster->http3_options_.set_allow_extended_connect(true); |
There was a problem hiding this comment.
I'd imagine that on the client we'd use this option to fail the extended connect before it's sent? Or is it a no-op on the client?
| // and settings `proposed for HTTP/3 | ||
| // <https://github.com/RyanTheOptimist/httpbis-h3-websockets/blob/main/draft-hamilton-httpbis-h3-websockets.md>`_ | ||
| // [#alpha:] as HTTP/3 CONNECT is not yet an RFC. | ||
| bool allow_extended_connect = 5; |
There was a problem hiding this comment.
When we had discussed this last, I was under the impression that we wanted an HTTP/3 option to enable CONNECT altogether. Is that no longer the case? Are we allowing non-extended CONNECT by default?
There was a problem hiding this comment.
Alyssa can correct me, but if I'm remembering our conversation correctly, I believe vanilla CONNECT is controlled by an HCM knob and we did not need a per-protocol vanilla CONNECT knob. (The HTTP/2 option, while named "allow_connect" actually enables extended connect so this HTTP/3 is parallel to the HTTP/3 option but with a more accurate name)
There was a problem hiding this comment.
Yep, we've gone over that a couple of times now :-) Old-style CONNECT gets rejected in the HCM if there's not connect matchers, and so we only need to control extended connect in the protoco-specific options. I tried to also make sure the documentation pretty clear on this point (https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/upgrades#connect-support) but please let me know if you have ideas of how I could improve it!
There was a problem hiding this comment.
Thanks for clarifying!
|
Sorry needs a main merge. /wait |
|
Normally it's all "ugh, merge conflict." |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
This includes validation for upgrade connects per Ryan's offline advice.
n.b. this should be a no-op for HTTP (where there is no mechanism to send both) and HTTP/2 (where nghttp2 validates) so not currently calling out in release notes.
Risk Level: low
Testing: new integration tests
Docs Changes: inline
Release Notes: n/a (quic alpha)
co-author: @DavidSchinazi