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.

Sign up for GitHub

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

Jump to bottom

implement HTTP/3 unidirectional stream hijacking #3389

Merged
merged 3 commits into from
Apr 21, 2022
Merged

implement HTTP/3 unidirectional stream hijacking #3389

merged 3 commits into from
Apr 21, 2022
+168 −2

Conversation

hareku
Copy link
Contributor

@hareku hareku commented Apr 20, 2022

Additional implementation of #3362. To support for hijacking unidirectional streams.

@google-cla
Copy link

google-cla bot commented Apr 20, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@hareku hareku marked this pull request as draft April 20, 2022 04:04
@hareku hareku marked this pull request as ready for review April 20, 2022 04:26
@hareku hareku force-pushed the unistream-hijacker branch 3 times, most recently from 6dcd185 to 1e53956 Compare April 20, 2022 05:51
@hareku hareku marked this pull request as draft April 20, 2022 07:30
marten-seemann
marten-seemann requested changes Apr 20, 2022
View reviewed changes
Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

This PR is not correct. Unidirectional streams are marked by their stream type, not by a frame.

http3/client.go Outdated
@@ -44,6 +44,7 @@ type roundTripperOpts struct {
MaxHeaderBytes int64
AdditionalSettings map[uint64]uint64
StreamHijacker func(FrameType, quic.Connection, quic.Stream) (hijacked bool, err error)
UniStreamHijacker func(FrameType, quic.Connection, quic.ReceiveStream) (hijacked bool, err error)
Copy link
Member

Choose a reason for hiding this comment

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

This is not a frame type, it's a stream type. We should have a separate type for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was misunderstanding. I fixed it.

@hareku hareku force-pushed the unistream-hijacker branch 2 times, most recently from 027248f to f1a3510 Compare April 20, 2022 10:34
@hareku hareku marked this pull request as ready for review April 20, 2022 10:48
marten-seemann
marten-seemann reviewed Apr 20, 2022
View reviewed changes
Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

http3/client.go Outdated
str.CancelRead(quic.StreamErrorCode(errorStreamCreationError))
return
}
f, err := parseNextFrame(str, nil)
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to move this block.

http3/client.go Show resolved Hide resolved
http3/client.go Outdated
@@ -44,6 +44,7 @@ type roundTripperOpts struct {
MaxHeaderBytes int64
AdditionalSettings map[uint64]uint64
StreamHijacker func(FrameType, quic.Connection, quic.Stream) (hijacked bool, err error)
UniStreamHijacker func(StreamType, quic.Connection, quic.ReceiveStream) (hijacked bool, err error)
Copy link
Member

Choose a reason for hiding this comment

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

Does this really need an error return value?

Copy link
Contributor Author

@hareku hareku Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it doesn't.

@marten-seemann
Copy link
Member

A unit test for this would also be very nice to have in this PR :)

@codecov
Copy link

codecov bot commented Apr 20, 2022 •
edited
Loading

Codecov Report

Merging #3389 (cb8951a) into master (6d4a694) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3389      +/-   ##
==========================================
+ Coverage   85.39%   85.40%   +0.01%     
==========================================
  Files         135      135              
  Lines        9906     9911       +5     
==========================================
+ Hits         8459     8464       +5     
  Misses       1065     1065              
  Partials      382      382
Impacted Files Coverage Δ
http3/client.go 82.26% <100.00%> (+0.19%) ⬆️
http3/roundtrip.go 93.55% <100.00%> (+0.11%) ⬆️
http3/server.go 72.08% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d4a694...cb8951a. Read the comment docs.

@hareku hareku force-pushed the unistream-hijacker branch 5 times, most recently from 5dfd1d4 to 7413e41 Compare April 21, 2022 05:25
@hareku
Copy link
Contributor Author

hareku commented Apr 21, 2022

Thank you for your review!
I added some unit tests.

marten-seemann
marten-seemann approved these changes Apr 21, 2022
View reviewed changes
http3/server.go Show resolved Hide resolved
http3/server.go Outdated Show resolved Hide resolved
http3/roundtrip.go Outdated Show resolved Hide resolved
http3/server_test.go Outdated Show resolved Hide resolved
http3/client_test.go Outdated Show resolved Hide resolved
hareku and others added 2 commits April 21, 2022 19:35
@hareku @marten-seemann
Apply suggestions from code review
cb5dcf9 
Fixed name consistency.

Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@hareku
rename unistream to unidirectional stream
cb8951a
@hareku
Copy link
Contributor Author

hareku commented Apr 21, 2022 •
edited
Loading

Thank you for your suggestions. I fixed words inconsistency at cb8951a.

@marten-seemann marten-seemann changed the title implement HTTP/3 unistream hijacking implement HTTP/3 unidirectional stream hijacking Apr 21, 2022
@marten-seemann marten-seemann merged commit 1a0d577 into quic-go:master Apr 21, 2022
nmldiegues pushed a commit to chungthuang/quic-go that referenced this pull request Jun 6, 2022
@hareku @marten-seemann @nmldiegues
implement HTTP/3 unidirectional stream hijacking (quic-go#3389)
f296b61 
* implement HTTP/3 unistream hijacking

* Apply suggestions from code review

Fixed name consistency.

Co-authored-by: Marten Seemann <martenseemann@gmail.com>

* rename unistream to unidirectional stream

Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@MarcoPolo MarcoPolo mentioned this pull request Jul 7, 2022
Closed
41 tasks
sudarshan-reddy pushed a commit to sudarshan-reddy/quic-go that referenced this pull request Aug 9, 2022
@hareku @marten-seemann @sudarshan-reddy
implement HTTP/3 unidirectional stream hijacking (quic-go#3389)
10fb3d2 
* implement HTTP/3 unistream hijacking

* Apply suggestions from code review

Fixed name consistency.

Co-authored-by: Marten Seemann <martenseemann@gmail.com>

* rename unistream to unidirectional stream

Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
Closed
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marten-seemann marten-seemann marten-seemann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hareku @marten-seemann