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

fix(client): Do not strip path and scheme components from URIs for HTTP/2 Extended CONNECT requests #3242

Merged
merged 4 commits into from Aug 3, 2023

Conversation

bdbai
Copy link

@bdbai bdbai commented Jun 9, 2023

As per RFC 8441:

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.

I was trying to implement WebSocket handshake based on HTTP/2 Extended CONNECT. The current implementation crafted a request which was considered invalid by the server (HAProxy 2.8):

image

Compared to the example illustrated in RFC 8441, the pseudo headers :scheme and :path were missing.

After the fix, the client could successfully establish a WebSocket tunnel to the server.

image

@bdbai bdbai marked this pull request as draft June 9, 2023 17:31
@bdbai bdbai marked this pull request as ready for review June 9, 2023 17:37
@bdbai bdbai changed the title Do not strip path and scheme components from URIs for HTTP/2 CONNECT requests fix(client): Do not strip path and scheme components from URIs for HTTP/2 Extended CONNECT requests Jun 9, 2023
bdbai added a commit to YtFlow/YtFlowCore that referenced this pull request Jun 9, 2023
- Auto probe and fallback
- Pending hyperium/hyper#3242 to work
  - hyper 人合了麻烦踢一下谢谢茄子
@seanmonstar seanmonstar requested a review from nox June 12, 2023 18:06
@bdbai
Copy link
Author

bdbai commented Jul 2, 2023

Hi @nox , can I nudge a little bit for reviewing this PR?👀

Copy link
Contributor

@nox nox left a comment

Choose a reason for hiding this comment

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

Would be nice to have a test but this looks good to me.

@bdbai
Copy link
Author

bdbai commented Jul 7, 2023

@nox I have added two integrations tests for client CONNECT requests.

@seanmonstar Since the Extended CONNECT requests only apply to h2, I've made a few changes to the helper macro in the integration test so that certain tests will not run in http/1 mode. Please take a look and see if you are okay with this approach. Thanks!

@bdbai bdbai requested a review from nox July 7, 2023 17:35
@seanmonstar
Copy link
Member

I'm happy to get this in with the test, thanks so much! Looks like CI noticed a combination of features that fail to compile, so probably a cfg(feature = ...) flag is needed in a new place.

@bdbai
Copy link
Author

bdbai commented Aug 3, 2023

@seanmonstar I've updated it with the feature flags. Hopefully it will pass this time.

@seanmonstar seanmonstar merged commit 45aa624 into hyperium:0.14.x Aug 3, 2023
19 checks passed
@seanmonstar
Copy link
Member

Thank you for being patient with us!

@DDtKey
Copy link
Contributor

DDtKey commented Aug 7, 2023

Shouldn't these changes be reflected in 1.0.0 version? (or perhaps hyper-utils)
I'm a bit concerned that some changes may be missed in the newer versions

cc @seanmonstar

@seanmonstar
Copy link
Member

Yes, a similar thing could be made a PR to https://github.com/hyperium/hyper-util

kbudde pushed a commit to kbudde/bench-metrics that referenced this pull request Dec 30, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [hyper](https://hyper.rs)
([source](https://togithub.com/hyperium/hyper)) | dependencies | patch |
`0.14.27` -> `0.14.28` |

---

### Release Notes

<details>
<summary>hyperium/hyper (hyper)</summary>

###
[`v0.14.28`](https://togithub.com/hyperium/hyper/releases/tag/v0.14.28)

[Compare
Source](https://togithub.com/hyperium/hyper/compare/v0.14.27...v0.14.28)

#### Features

- **body:** deprecate to_bytes() and aggregate()
([#&#8203;3466](https://togithub.com/hyperium/hyper/issues/3466))
([7f382ad6](https://togithub.com/hyperium/hyper/commit/7f382ad64326e1470912feb310d348fd79099c44))
- **client:** add `conn::http1::Connection::without_shutdown()` method
([#&#8203;3431](https://togithub.com/hyperium/hyper/issues/3431))
([ad504977](https://togithub.com/hyperium/hyper/commit/ad504977b520a9582e5516a08b2f1028ef1b5e45))
- **server:** add `Builder::local_addr()`
([#&#8203;3278](https://togithub.com/hyperium/hyper/issues/3278))
([d342c2c7](https://togithub.com/hyperium/hyper/commit/d342c2c714498d33891fa285a3c9ae991dc34769))

#### Bug Fixes

-   **client:**
- panic when pool idle timeout set to zero
([#&#8203;3365](https://togithub.com/hyperium/hyper/issues/3365))
([34d38008](https://togithub.com/hyperium/hyper/commit/34d38008499de37d9b5b65440b3123ccd05c7510))
- divide by zero error when DNS returns no addrs
([#&#8203;3355](https://togithub.com/hyperium/hyper/issues/3355))
([41eaf204](https://togithub.com/hyperium/hyper/commit/41eaf2042b8169d3dd067d49cfdbdaaf36678903))
- Do not strip `path` and `scheme` components from URIs for HTTP/2
Extended CONNEC
([45aa6249](https://togithub.com/hyperium/hyper/commit/45aa62494127066c63c987a57cc5eae2c5361886))
- early respond from server shouldn't propagate reset error
([#&#8203;3274](https://togithub.com/hyperium/hyper/issues/3274))
([aac6760e](https://togithub.com/hyperium/hyper/commit/aac6760e032050dd47f5dbd32f852bf1ede9312b),
closes [#&#8203;2872](https://togithub.com/hyperium/hyper/issues/2872))
-   **http1:**
- add internal limit for chunked extensions
([#&#8203;3495](https://togithub.com/hyperium/hyper/issues/3495))
([344a8782](https://togithub.com/hyperium/hyper/commit/344a87822951a46d252843ccc0b48e62988fc85b))
- reject chunked headers missing a digit
([#&#8203;3494](https://togithub.com/hyperium/hyper/issues/3494))
([5eca028f](https://togithub.com/hyperium/hyper/commit/5eca028f4142e3e73f6d6188a4076f4db292b252))

#### New Contributors

- [@&#8203;bdbai](https://togithub.com/bdbai) made their first
contribution in
[hyperium/hyper#3242
- [@&#8203;gngpp](https://togithub.com/gngpp) made their first
contribution in
[hyperium/hyper#3355

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/kbudde/bench-metrics).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMDMuMSIsInVwZGF0ZWRJblZlciI6IjM3LjEwMy4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
leftwo pushed a commit to oxidecomputer/crucible that referenced this pull request May 14, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [hyper](https://hyper.rs)
([source](https://togithub.com/hyperium/hyper)) | workspace.dependencies
| patch | `0.14.27` -> `0.14.28` |

---

### Release Notes

<details>
<summary>hyperium/hyper (hyper)</summary>

###
[`v0.14.28`](https://togithub.com/hyperium/hyper/releases/tag/v0.14.28)

[Compare
Source](https://togithub.com/hyperium/hyper/compare/v0.14.27...v0.14.28)

#### Features

- **body:** deprecate to_bytes() and aggregate()
([#&#8203;3466](https://togithub.com/hyperium/hyper/issues/3466))
([7f382ad6](https://togithub.com/hyperium/hyper/commit/7f382ad64326e1470912feb310d348fd79099c44))
- **client:** add `conn::http1::Connection::without_shutdown()` method
([#&#8203;3431](https://togithub.com/hyperium/hyper/issues/3431))
([ad504977](https://togithub.com/hyperium/hyper/commit/ad504977b520a9582e5516a08b2f1028ef1b5e45))
- **server:** add `Builder::local_addr()`
([#&#8203;3278](https://togithub.com/hyperium/hyper/issues/3278))
([d342c2c7](https://togithub.com/hyperium/hyper/commit/d342c2c714498d33891fa285a3c9ae991dc34769))

#### Bug Fixes

-   **client:**
- panic when pool idle timeout set to zero
([#&#8203;3365](https://togithub.com/hyperium/hyper/issues/3365))
([34d38008](https://togithub.com/hyperium/hyper/commit/34d38008499de37d9b5b65440b3123ccd05c7510))
- divide by zero error when DNS returns no addrs
([#&#8203;3355](https://togithub.com/hyperium/hyper/issues/3355))
([41eaf204](https://togithub.com/hyperium/hyper/commit/41eaf2042b8169d3dd067d49cfdbdaaf36678903))
- Do not strip `path` and `scheme` components from URIs for HTTP/2
Extended CONNEC
([45aa6249](https://togithub.com/hyperium/hyper/commit/45aa62494127066c63c987a57cc5eae2c5361886))
- early respond from server shouldn't propagate reset error
([#&#8203;3274](https://togithub.com/hyperium/hyper/issues/3274))
([aac6760e](https://togithub.com/hyperium/hyper/commit/aac6760e032050dd47f5dbd32f852bf1ede9312b),
closes [#&#8203;2872](https://togithub.com/hyperium/hyper/issues/2872))
-   **http1:**
- add internal limit for chunked extensions
([#&#8203;3495](https://togithub.com/hyperium/hyper/issues/3495))
([344a8782](https://togithub.com/hyperium/hyper/commit/344a87822951a46d252843ccc0b48e62988fc85b))
- reject chunked headers missing a digit
([#&#8203;3494](https://togithub.com/hyperium/hyper/issues/3494))
([5eca028f](https://togithub.com/hyperium/hyper/commit/5eca028f4142e3e73f6d6188a4076f4db292b252))

#### New Contributors

- [@&#8203;bdbai](https://togithub.com/bdbai) made their first
contribution in
[hyperium/hyper#3242
- [@&#8203;gngpp](https://togithub.com/gngpp) made their first
contribution in
[hyperium/hyper#3355

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

None yet

4 participants