support the CONNECT method

[klzgrad]

Fix #2748.

[googlebot]

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[klzgrad]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[codecov]

Codecov Report

Merging #2761 into master will increase coverage by 0.17%.
The diff coverage is 88.24%.

@@            Coverage Diff             @@
##           master    #2761      +/-   ##
==========================================
+ Coverage   86.52%   86.69%   +0.17%     
==========================================
  Files         128      128              
  Lines        9960    10013      +53     
==========================================
+ Hits         8617     8680      +63     
+ Misses       1010     1006       -4     
+ Partials      333      327       -6     

Impacted Files	Coverage Δ
http3/request.go	92.98% <88.24%> (+1.87%)	⬆️
logging/multiplex.go	100.00% <0.00%> (ø)
qlog/packet_header.go	94.87% <0.00%> (ø)
internal/ackhandler/sent_packet_history.go	100.00% <0.00%> (ø)
qlog/event.go	96.17% <0.00%> (+0.09%)	⬆️
packet_unpacker.go	86.11% <0.00%> (+0.11%)	⬆️
session.go	77.53% <0.00%> (+0.18%)	⬆️
qlog/qlog.go	95.59% <0.00%> (+0.20%)	⬆️
internal/ackhandler/sent_packet_handler.go	73.91% <0.00%> (+0.81%)	⬆️
internal/handshake/updatable_aead.go	97.06% <0.00%> (+5.72%)	⬆️

---

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 bed802a...d77c439. Read the comment docs.

[klzgrad]

I don't understand this warning, "string CONNECT has 3 occurrences, make it a constant (goconst)." Isn't there only one occurrence?

[marten-seemann]

Thank you @klzgrad! This looks very good to me, I just found two very minor nits (see comments).

I also have a few questions from reading the spec:

• The ":authority" pseudo-header field contains the host and port to connect to (equivalent to the authority-form of the request-target of CONNECT requests; see Section 3.2.3 of [HTTP11])

We're currently just copying over the :authority field. Am I right to assume that the upstream proxy implementation will perform a check that this field actually contains a host and port?

Furthermore, I'm wondering about this paragraph:

Once the CONNECT method has completed, only DATA frames are permitted to be sent on the stream. Extension frames MAY be used if specifically permitted by the definition of the extension. Receipt of any other frame type MUST be treated as a connection error of type H3_FRAME_UNEXPECTED.

Do we need to check for this?

[klzgrad]

Am I right to assume that the upstream proxy implementation will perform a check that this field actually contains a host and port?

caddyserver/forwardproxy does this. In abstract, we are passing a host and port in a single authority string to the upstream proxy, and to actually perform the connection the proxy always has to first parse the string into a host and port. Checking its validity seems more suitable in that place.

only DATA frames are permitted to be sent on the stream

Currently checking of this part is done in this code,

https://github.com/lucas-clemente/quic-go/blob/c5a132f158532ee8fbc0442f7c258c929fd77aab/http3/body.go#L50-L72

Not strictly conforming, as it merely ignores HEADERS frame, but the non-conformance is on the same level with regular HTTP requests. As in https://tools.ietf.org/id/draft-ietf-quic-http-29.html#section-4.1-6:

Receipt of an invalid sequence of frames MUST be treated as a connection error of type H3_FRAME_UNEXPECTED (Section 8). In particular, a DATA frame before any HEADERS frame, or a HEADERS or DATA frame after the trailing HEADERS frame is considered invalid.

I hope fixing all that won't be required in this PR.

[marten-seemann]

That makes sense. We can fix that in a separate issue.

[marten-seemann]

LGTM now. Thank you @klzgrad!