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

http3: support HTTP CONNECT and CONNECT-UDP (masque) protocols #4392

Closed
wants to merge 1 commit into from

Conversation

max-b
Copy link

@max-b max-b commented Mar 28, 2024

We'd like to propose these changes in order for us to support client http3 CONNECT and CONNECT-UDP.

We've rolled up a few changes into this single PR:

  • Adding a Proxy method to the http3.RoundTripper
  • Allowing access to the underlying quic.Connection from a hijackableBody
  • Some slight adjustments to the handling of methods in the http3 Client/RoundTripper
  • Adding support for the draft03 version of RFC 9297
    • We think this is a reasonable approach, in particular since it's the behavior of quiche and h2o

We're currently using quic-go along with these changes for our implementation of a MASQUE proxy client: Invisv-Privacy/masque#4

I'm aware that we've not had any previous interaction w/ y'all so we're very open to any/all feedback/changes/etc.

I think that this would address the client sides of #3370 and #3543

Copy link

google-cla bot commented Mar 28, 2024

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).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@marten-seemann
Copy link
Member

Hi Max! Thank you for your PR!

quic-go will gain MASQUE capabilities soon(-ish). I just opened a tracking issue for that: #4393.

The way we’ll implement it will look a bit different though.
For example, we want to fully support HTTP datagrams (#3522), in a way where the HTTP/3 layer deals with prepending the Quarter Stream ID, and dispatches datagrams to their respective streams. I’ve been working towards that, and shipped a number of changes to the http3 package in the v0.42 release, in preparation for that.

Regarding backwards compatibility with old versions of the datagram draft, we won’t add that to quic-go, given that the RFC has shipped for quite a while now. If I understand correctly though, all you seem to need is a different code point in the SETTINGS. Does the current API already cover this? It should be possible to define additional settings code points using the Other map.

Regarding the Proxy addition to the RoundTripper, this looks reasonable to me at first glance (especially since the standard library http.Transport seems to do something similar, but I have to admit I haven’t thought about this a lot. I also see a OnProxyConnectResponse response, which I don’t understand what it’s used for yet. I’d prefer to continue this discussion on the issue though.

@max-b
Copy link
Author

max-b commented Mar 28, 2024

Thanks so much for your interest and response! #4393 seems like an exciting direction for the project :)

Regarding backwards compatibility with old versions of the datagram draft, we won’t add that to quic-go, given that the RFC has shipped for quite a while now. If I understand correctly though, all you seem to need is a different code point in the SETTINGS. Does the current API already cover this? It should be possible to define additional settings code points using the Other map.

I might not have the most full understanding of the api and how the Other map works, but because the draft code point has semantic meaning re: the frame settings I'm not sure the api supports our use case?

https://github.com/Invisv-Privacy/quic-go-upstream/blob/66efcd13532305e856773dd484adec8e86ee469a/http3/frames.go#L152-L160

		case settingDatagramDraft03:
			if readDatagramDraft03 {
				return nil, fmt.Errorf("duplicate setting: %d", id)
			}
			readDatagramDraft03 = true
			if val != 0 && val != 1 {
				return nil, fmt.Errorf("invalid value for Draft03 SETTINGS_H3_DATAGRAM: %d", val)
			}
			frame.Datagram = val == 1

https://github.com/Invisv-Privacy/quic-go-upstream/blob/66efcd13532305e856773dd484adec8e86ee469a/http3/client.go#L232-L239

			sf, ok := f.(*settingsFrame)
			if !ok {
				conn.CloseWithError(quic.ApplicationErrorCode(ErrCodeMissingSettings), "")
				return
			}
			if !sf.Datagram {
				return
			}

Regarding the Proxy addition to the RoundTripper, this looks reasonable to me at first glance (especially since the standard library http.Transport seems to do something similar, but I have to admit I haven’t thought about this a lot.

We'd be happy to break this piece out into a separate PR if that would be helpful?

I also see a OnProxyConnectResponse response, which I don’t understand what it’s used for yet. I’d prefer to continue this discussion on the issue though.

golang/go#54299 and https://go-review.googlesource.com/c/go/+/447216 seem helpful. The functionality isn't quite parallel though as that method is actually responsible for the functionality of http/socks/etc proxying. It's not super clear to me how the OnProxyConnectResponse would fit in this case. To be honest, if you're trying to parallel golang's net/http api, the Proxy method might need to wait on you actually implementing the underlying functionality.

Thanks!

@marten-seemann
Copy link
Member

Regarding backwards compatibility with old versions of the datagram draft, we won’t add that to quic-go, given that the RFC has shipped for quite a while now. If I understand correctly though, all you seem to need is a different code point in the SETTINGS. Does the current API already cover this? It should be possible to define additional settings code points using the Other map.

I might not have the most full understanding of the api and how the Other map works, but because the draft code point has semantic meaning re: the frame settings I'm not sure the api supports our use case?

You're right. You could negotiate the extension, but quic-go wouldn't recognize it as having been negotiated, and therefore reject datagrams.
How much do you care about the draft version? Do you expect to continue supporting for a long time?

@marten-seemann
Copy link
Member

Regarding the Proxy addition to the RoundTripper, this looks reasonable to me at first glance (especially since the standard library http.Transport seems to do something similar, but I have to admit I haven’t thought about this a lot.

We'd be happy to break this piece out into a separate PR if that would be helpful?

Yes, definitely! I'm not sure I fully understand this field (even in the standard library) yet: What happens if you return a socks5 URL? Doesn't this mean we now need to support socks5? What if http is returned? Do we now need to branch out to the standard library to do an HTTP request on TCP?
Maybe this relates to the larger question of whether this package should implement some kind of Happy Eyeballs v3 to dial both QUIC and TCP.
To be clear, this doesn't all need to be solved by your PR. I'm fine just adding proxying for HTTP/3 for now. Just want to know what the way forward is, before we commit to an API.

@max-b
Copy link
Author

max-b commented Apr 2, 2024

You're right. You could negotiate the extension, but quic-go wouldn't recognize it as having been negotiated, and therefore reject datagrams. How much do you care about the draft version? Do you expect to continue supporting for a long time?

We originally developed our client targeting h2o while it was still only implementing the draft. Since Fall of last year they've added support for the finalized rfc: h2o/h2o#3278 We're currently only using the draft version, but in the future we do expect we'll eventually update to use the finalized spec, perhaps if/when h2o decides to drop draft support. We'd prefer to have the extra settings code point here and both quiche and h2o implement it this way, but it's obviously up to you and if you choose not to we will eventually move to the finalize rfc version.

@marten-seemann
Copy link
Member

CONNECT-UDP is now being worked on in https://github.com/quic-go/masque-go.

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

3 participants