Repository: quic-go/quic-go
Pull request #4450 (2024-04-20 09:58:40)

http3: check server SETTINGS before sending an Extended CONNECT request
As suggested by @sukunrt in https://github.com/quic-go/quic-go/pull/4355#pullrequestreview-1927658506.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4450?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
Attention: Patch coverage is `84.61538%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 84.68%. Comparing base [(`18422ad`)](https://app.codecov.io/gh/quic-go/quic-go/commit/18422ad1c44726209a7e854a473d61b1c78bfdfc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`8c3290b`)](https://app.codecov.io/gh/quic-go/quic-go/pull/4450?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

| [Files](https://app.codecov.io/gh/quic-go/quic-go/pull/4450?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) | Patch % | Lines |
|---|---|---|
| [http3/client.go](https://app.codecov.io/gh/quic-go/quic-go/pull/4450?src=pr&el=tree&filepath=http3%2Fclient.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go#diff-aHR0cDMvY2xpZW50Lmdv) | 80.00% | [2 Missing :warning: ](https://app.codecov.io/gh/quic-go/quic-go/pull/4450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4450      +/-   ##
==========================================
+ Coverage   84.66%   84.68%   +0.01%     
==========================================
  Files         152      152              
  Lines       14372    14384      +12     
==========================================
+ Hits        12168    12180      +12     
  Misses       1701     1701              
  Partials      503      503              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4450?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).


Patch:
diff --git a/http3/client.go b/http3/client.go
index a32844e3..501e3db5 100644
--- a/http3/client.go
+++ b/http3/client.go
@@ -2,6 +2,7 @@ package http3
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -180,6 +181,21 @@ func (c *SingleDestinationRoundTripper) roundTrip(req *http.Request) (*http.Resp
 		}
 	}
 
+	// It is only possible to send an Extended CONNECT request once the SETTINGS were received.
+	// See section 3 of RFC 8441.
+	if isExtendedConnectRequest(req) {
+		connCtx := c.Connection.Context()
+		// wait for the server's SETTINGS frame to arrive
+		select {
+		case <-c.hconn.ReceivedSettings():
+		case <-connCtx.Done():
+			return nil, context.Cause(connCtx)
+		}
+		if !c.hconn.Settings().EnableExtendedConnect {
+			return nil, errors.New("http3: server didn't enable Extended CONNECT")
+		}
+	}
+
 	str, err := c.Connection.OpenStreamSync(req.Context())
 	if err != nil {
 		return nil, err
diff --git a/http3/request_writer.go b/http3/request_writer.go
index 995b8d7a..e83e9cc8 100644
--- a/http3/request_writer.go
+++ b/http3/request_writer.go
@@ -65,6 +65,10 @@ func (w *requestWriter) writeHeaders(wr io.Writer, req *http.Request, gzip bool)
 	return err
 }
 
+func isExtendedConnectRequest(req *http.Request) bool {
+	return req.Method == http.MethodConnect && req.Proto != "" && req.Proto != "HTTP/1.1"
+}
+
 // copied from net/transport.go
 // Modified to support Extended CONNECT:
 // Contrary to what the godoc for the http.Request says,
@@ -83,7 +87,7 @@ func (w *requestWriter) encodeHeaders(req *http.Request, addGzipHeader bool, tra
 	}
 
 	// http.NewRequest sets this field to HTTP/1.1
-	isExtendedConnect := req.Method == http.MethodConnect && req.Proto != "" && req.Proto != "HTTP/1.1"
+	isExtendedConnect := isExtendedConnectRequest(req)
 
 	var path string
 	if req.Method != http.MethodConnect || isExtendedConnect {


Test patch:
diff --git a/http3/client_test.go b/http3/client_test.go
index 2c7eb4bf..7adc62e8 100644
--- a/http3/client_test.go
+++ b/http3/client_test.go
@@ -375,6 +375,73 @@ var _ = Describe("Client", func() {
 			conn.EXPECT().CloseWithError(gomock.Any(), gomock.Any()).MaxTimes(1)
 			close(done)
 		})
+
+		It("checks the server's SETTINGS before sending an Extended CONNECT request", func() {
+			done := make(chan struct{})
+			conn := mockquic.NewMockEarlyConnection(mockCtrl)
+			conn.EXPECT().OpenUniStream().DoAndReturn(func() (quic.SendStream, error) {
+				<-done
+				return nil, errors.New("test done")
+			}).MaxTimes(1)
+			b := quicvarint.Append(nil, streamTypeControlStream)
+			b = (&settingsFrame{ExtendedConnect: true}).Append(b)
+			r := bytes.NewReader(b)
+			controlStr := mockquic.NewMockStream(mockCtrl)
+			controlStr.EXPECT().Read(gomock.Any()).DoAndReturn(r.Read).AnyTimes()
+			conn.EXPECT().AcceptUniStream(gomock.Any()).Return(controlStr, nil)
+			conn.EXPECT().AcceptUniStream(gomock.Any()).DoAndReturn(func(context.Context) (quic.ReceiveStream, error) {
+				<-done
+				return nil, errors.New("test done")
+			})
+			conn.EXPECT().HandshakeComplete().Return(handshakeChan)
+			conn.EXPECT().Context().Return(context.Background())
+			conn.EXPECT().OpenStreamSync(gomock.Any()).Return(nil, errors.New("test error"))
+
+			rt := &SingleDestinationRoundTripper{Connection: conn}
+			_, err := rt.RoundTrip(&http.Request{
+				Method: http.MethodConnect,
+				Proto:  "connect",
+				Host:   "localhost",
+			})
+			Expect(err).To(MatchError("test error"))
+
+			// test shutdown
+			conn.EXPECT().CloseWithError(gomock.Any(), gomock.Any()).MaxTimes(1)
+			close(done)
+		})
+
+		It("rejects Extended CONNECT requests if the server doesn't enable it", func() {
+			done := make(chan struct{})
+			conn := mockquic.NewMockEarlyConnection(mockCtrl)
+			conn.EXPECT().OpenUniStream().DoAndReturn(func() (quic.SendStream, error) {
+				<-done
+				return nil, errors.New("test done")
+			}).MaxTimes(1)
+			b := quicvarint.Append(nil, streamTypeControlStream)
+			b = (&settingsFrame{Datagram: true}).Append(b)
+			r := bytes.NewReader(b)
+			controlStr := mockquic.NewMockStream(mockCtrl)
+			controlStr.EXPECT().Read(gomock.Any()).DoAndReturn(r.Read).AnyTimes()
+			conn.EXPECT().AcceptUniStream(gomock.Any()).Return(controlStr, nil)
+			conn.EXPECT().AcceptUniStream(gomock.Any()).DoAndReturn(func(context.Context) (quic.ReceiveStream, error) {
+				<-done
+				return nil, errors.New("test done")
+			})
+			conn.EXPECT().HandshakeComplete().Return(handshakeChan)
+			conn.EXPECT().Context().Return(context.Background())
+
+			rt := &SingleDestinationRoundTripper{Connection: conn}
+			_, err := rt.RoundTrip(&http.Request{
+				Method: http.MethodConnect,
+				Proto:  "connect",
+				Host:   "localhost",
+			})
+			Expect(err).To(MatchError("http3: server didn't enable Extended CONNECT"))
+
+			// test shutdown
+			conn.EXPECT().CloseWithError(gomock.Any(), gomock.Any()).MaxTimes(1)
+			close(done)
+		})
 	})
 
 	Context("Doing requests", func() {
diff --git a/integrationtests/self/multiplex_test.go b/integrationtests/self/multiplex_test.go
index 7b202c71..0641f8b6 100644
--- a/integrationtests/self/multiplex_test.go
+++ b/integrationtests/self/multiplex_test.go
@@ -249,6 +249,7 @@ var _ = Describe("Multiplexing", func() {
 			defer GinkgoRecover()
 			ticker := time.NewTicker(time.Millisecond / 10)
 			defer ticker.Stop()
+			var wroteFirstPacket bool
 			for {
 				select {
 				case <-ticker.C:
@@ -258,11 +259,18 @@ var _ = Describe("Multiplexing", func() {
 				b := make([]byte, packetLen)
 				rand.Read(b[1:]) // keep the first byte set to 0, so it's not classified as a QUIC packet
 				_, err := tr1.WriteTo(b, tr2.Conn.LocalAddr())
+				// The first sendmsg call on a new UDP socket sometimes errors on Linux.
+				// It's not clear why this happens.
+				// See https://github.com/golang/go/issues/63322.
+				if err != nil && !wroteFirstPacket && isPermissionError(err) {
+					_, err = tr1.WriteTo(b, tr2.Conn.LocalAddr())
+				}
 				if ctx.Err() != nil { // ctx canceled while Read was executing
 					return
 				}
 				Expect(err).ToNot(HaveOccurred())
 				sentPackets.Add(1)
+				wroteFirstPacket = true
 			}
 		}()
 
diff --git a/integrationtests/self/self_suite_linux_test.go b/integrationtests/self/self_suite_linux_test.go
new file mode 100644
index 00000000..7baa1f48
--- /dev/null
+++ b/integrationtests/self/self_suite_linux_test.go
@@ -0,0 +1,21 @@
+//go:build linux
+
+package self_test
+
+import (
+	"errors"
+	"os"
+
+	"golang.org/x/sys/unix"
+)
+
+// The first sendmsg call on a new UDP socket sometimes errors on Linux.
+// It's not clear why this happens.
+// See https://github.com/golang/go/issues/63322.
+func isPermissionError(err error) bool {
+	var serr *os.SyscallError
+	if errors.As(err, &serr) {
+		return serr.Syscall == "sendmsg" && serr.Err == unix.EPERM
+	}
+	return false
+}
diff --git a/integrationtests/self/self_suite_others_test.go b/integrationtests/self/self_suite_others_test.go
new file mode 100644
index 00000000..c0080d35
--- /dev/null
+++ b/integrationtests/self/self_suite_others_test.go
@@ -0,0 +1,7 @@
+//go:build !linux
+
+package self_test
+
+func isPermissionError(err error) bool {
+	return false
+}