[net] http2: implement support for extended CONNECT
59 views
Skip to first unread message
Mike Danese (Gerrit)
Jul 6, 2023, 10:07:31 PM7/6/23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Mike Danese has uploaded this change for review.
http2: implement support for extended CONNECT
See https://datatracker.ietf.org/doc/html/rfc8441 for specification.
Server side is fairly straight forward. The server now always advertises
the ENABLE_CONNECT_PROTOCOL=1 setting, and validation is relaxed to
allow the :protocol pseudo-header (passed via req.Header[":protocol"]).
Client side is slightly more complicated. Summary of key changes:
* Extended CONNECT requests on a connection must be delayed until the
initial settings ack is seen. Thus we introduce seenSettingsCh to
ClientConn to support cancelable wait.
* If an extended CONNECT is attempted on a connection that has not
advertised support for extended CONNECT, an error that wraps
http.ErrNotSupported is returned to allow a client to fallback to e.g.
an http/1.1 upgrade.
The client side API is intended to be used by passing e.g. an io.Pipe()
reader as the body in http.NewRequest() then writing to the write side.
This is an alternative to the similar API for http.Response.Body:
> As of Go 1.12, the Body will also implement io.Writer
> on a successful "101 Switching Protocols" response,
> as used by WebSockets and HTTP/2's "h2c" mode.
Using an io.Pipe() is more flexible as it allows the stream to be
half-closed, although it's easy to imagine a simpler mode for when this
functionality is not needed (e.g. if the body passed to http.NewRequest
is nil, resp.Body implements io.ReadWriteCloser).
Fixes: golang/go#53208
Change-Id: Ib29fedfc5cde2779a099025e79bbbdd7b3e3cec7
---
M http2/frame.go
M http2/http2.go
M http2/server.go
M http2/server_test.go
M http2/transport.go
M http2/transport_test.go
6 files changed, 302 insertions(+), 22 deletions(-)
diff --git a/http2/frame.go b/http2/frame.go
index c1f6b90..cd7da7e 100644
--- a/http2/frame.go
+++ b/http2/frame.go
@@ -1487,7 +1487,7 @@
pf := mh.PseudoFields()
for i, hf := range pf {
switch hf.Name {
- case ":method", ":path", ":scheme", ":authority":
+ case ":method", ":path", ":scheme", ":authority", ":protocol":
isRequest = true
case ":status":
isResponse = true
@@ -1495,7 +1495,7 @@
return pseudoHeaderError(hf.Name)
}
// Check for duplicates.
- // This would be a bad algorithm, but N is 4.
+ // This would be a bad algorithm, but N is 5.
// And this doesn't allocate.
for _, hf2 := range pf[:i] {
if hf.Name == hf2.Name {
diff --git a/http2/http2.go b/http2/http2.go
index 6f2df28..efbd555 100644
--- a/http2/http2.go
+++ b/http2/http2.go
@@ -138,6 +138,11 @@
if s.Val < 16384 || s.Val > 1<<24-1 {
return ConnectionError(ErrCodeProtocol)
}
+ case SettingEnableConnectProtocol:
+ // Must be zero or one: https://datatracker.ietf.org/doc/html/rfc8441#section-3
+ if s.Val != 1 && s.Val != 0 {
+ return ConnectionError(ErrCodeProtocol)
+ }
}
return nil
}
@@ -147,21 +152,23 @@
type SettingID uint16
const (
- SettingHeaderTableSize SettingID = 0x1
- SettingEnablePush SettingID = 0x2
- SettingMaxConcurrentStreams SettingID = 0x3
- SettingInitialWindowSize SettingID = 0x4
- SettingMaxFrameSize SettingID = 0x5
- SettingMaxHeaderListSize SettingID = 0x6
+ SettingHeaderTableSize SettingID = 0x1
+ SettingEnablePush SettingID = 0x2
+ SettingMaxConcurrentStreams SettingID = 0x3
+ SettingInitialWindowSize SettingID = 0x4
+ SettingMaxFrameSize SettingID = 0x5
+ SettingMaxHeaderListSize SettingID = 0x6
+ SettingEnableConnectProtocol SettingID = 0x8
)
var settingName = map[SettingID]string{
- SettingHeaderTableSize: "HEADER_TABLE_SIZE",
- SettingEnablePush: "ENABLE_PUSH",
- SettingMaxConcurrentStreams: "MAX_CONCURRENT_STREAMS",
- SettingInitialWindowSize: "INITIAL_WINDOW_SIZE",
- SettingMaxFrameSize: "MAX_FRAME_SIZE",
- SettingMaxHeaderListSize: "MAX_HEADER_LIST_SIZE",
+ SettingHeaderTableSize: "HEADER_TABLE_SIZE",
+ SettingEnablePush: "ENABLE_PUSH",
+ SettingMaxConcurrentStreams: "MAX_CONCURRENT_STREAMS",
+ SettingInitialWindowSize: "INITIAL_WINDOW_SIZE",
+ SettingMaxFrameSize: "MAX_FRAME_SIZE",
+ SettingMaxHeaderListSize: "MAX_HEADER_LIST_SIZE",
+ SettingEnableConnectProtocol: "ENABLE_CONNECT_PROTOCOL",
}
func (s SettingID) String() string {
diff --git a/http2/server.go b/http2/server.go
index 033b6e6..709e50d 100644
--- a/http2/server.go
+++ b/http2/server.go
@@ -76,10 +76,11 @@
// Test hooks.
var (
- testHookOnConn func()
- testHookGetServerConn func(*serverConn)
- testHookOnPanicMu *sync.Mutex // nil except in tests
- testHookOnPanic func(sc *serverConn, panicVal interface{}) (rePanic bool)
+ testHookOnConn func()
+ testHookGetServerConn func(*serverConn)
+ testHookOnPanicMu *sync.Mutex // nil except in tests
+ testHookOnPanic func(sc *serverConn, panicVal interface{}) (rePanic bool)
+ testHookDisableConnectProtocol bool
)
// Server is an HTTP/2 server.
@@ -162,6 +163,13 @@
return 1 << 20
}
+func (s *Server) enableConnectProtocol() uint32 {
+ if testHookDisableConnectProtocol {
+ return 0
+ }
+ return 1
+}
+
func (s *Server) initialStreamRecvWindowSize() int32 {
if s.MaxUploadBufferPerStream > 0 {
return s.MaxUploadBufferPerStream
@@ -901,6 +909,7 @@
{SettingMaxHeaderListSize, sc.maxHeaderListSize()},
{SettingHeaderTableSize, sc.srv.maxDecoderHeaderTableSize()},
{SettingInitialWindowSize, uint32(sc.srv.initialStreamRecvWindowSize())},
+ {SettingEnableConnectProtocol, sc.srv.enableConnectProtocol()},
},
})
sc.unackedSettings++
@@ -2145,7 +2154,11 @@
}
isConnect := rp.method == "CONNECT"
- if isConnect {
+ // Per https://datatracker.ietf.org/doc/html/rfc8441#section-4, extended
+ // CONNECT requests (i.e. those with the protocol pseudo-header) follow the
+ // standard 8.1.2.6 validation requirements.
+ isExtendedConnect := isConnect && f.PseudoValue("protocol") != "" && !testHookDisableConnectProtocol
+ if isConnect && !isExtendedConnect {
if rp.path != "" || rp.scheme != "" || rp.authority == "" {
return nil, nil, sc.countError("bad_connect", streamError(f.StreamID, ErrCodeProtocol))
}
@@ -2167,6 +2180,9 @@
for _, hf := range f.RegularFields() {
rp.header.Add(sc.canonicalHeader(hf.Name), hf.Value)
}
+ if isExtendedConnect {
+ rp.header.Set(":protocol", f.PseudoValue("protocol"))
+ }
if rp.authority == "" {
rp.authority = rp.header.Get("Host")
}
diff --git a/http2/server_test.go b/http2/server_test.go
index cd73291..1e51680 100644
--- a/http2/server_test.go
+++ b/http2/server_test.go
@@ -720,6 +720,125 @@
})
}
+func TestServer_Request_ExtendedConnect(t *testing.T) {
+ reqDone := make(chan struct{})
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+ defer close(reqDone)
+ handleServerPingPong(t, w, r)
+ })
+ defer st.Close()
+
+ var sawEnableConnectProtocolSetting bool
+ st.greetAndCheckSettings(func(s Setting) error {
+ if s.ID != SettingEnableConnectProtocol {
+ return nil
+ }
+ sawEnableConnectProtocolSetting = true
+ if s.Val != 1 {
+ return fmt.Errorf("SettingEnableConnectProtocol = %d; want 1", s.Val)
+ }
+ return nil
+ })
+ if !sawEnableConnectProtocolSetting {
+ t.Errorf("Never saw SettingEnableConnectProtocol")
+ }
+
+ st.writeHeaders(HeadersFrameParam{
+ StreamID: 1,
+ BlockFragment: st.encodeHeader(
+ ":method", "CONNECT",
+ ":protocol", "pingpong"),
+ EndHeaders: true,
+ })
+
+ hf := st.wantHeaders()
+ if hf.StreamEnded() {
+ t.Fatal("unexpected END_STREAM")
+ }
+ if !hf.HeadersEnded() {
+ t.Fatal("want END_HEADERS flag")
+ }
+ if got, want := st.decodeHeader(hf.HeaderBlockFragment()), [][2]string{{":status", "200"}}; !reflect.DeepEqual(got, want) {
+ t.Errorf("Got headers %v; want %v", got, want)
+ }
+
+ st.writeData(1, false, []byte("ping"))
+ df := st.wantData()
+ if !bytes.Equal(df.Data(), []byte("pong")) {
+ t.Errorf("Got data %v; want %v", df.Data(), []byte("pong"))
+ }
+ if hf.StreamEnded() {
+ t.Fatal("unexpected END_STREAM")
+ }
+
+ st.writeData(1, true, nil)
+
+ <-reqDone
+}
+
+func TestServer_Request_ExtendedConnectDisabled(t *testing.T) {
+ testHookDisableConnectProtocol = true
+ t.Cleanup(func() {
+ testHookDisableConnectProtocol = false
+ })
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+ t.Error("Should have resulted in protocol error")
+ })
+ defer st.Close()
+
+ var sawEnableConnectProtocolSetting bool
+ st.greetAndCheckSettings(func(s Setting) error {
+ if s.ID != SettingEnableConnectProtocol {
+ return nil
+ }
+ sawEnableConnectProtocolSetting = true
+ if s.Val != 0 {
+ return fmt.Errorf("SettingEnableConnectProtocol = %d; want 0", s.Val)
+ }
+ return nil
+ })
+ if !sawEnableConnectProtocolSetting {
+ t.Errorf("Never saw SettingEnableConnectProtocol")
+ }
+
+ st.writeHeaders(HeadersFrameParam{
+ StreamID: 1, // clients send odd numbers
+ BlockFragment: st.encodeHeader(
+ ":method", "CONNECT",
+ ":protocol", "pingpong"),
+ EndHeaders: true,
+ })
+
+ st.wantRSTStream(1, ErrCodeProtocol)
+}
+
+func handleServerPingPong(t *testing.T, w http.ResponseWriter, r *http.Request) {
+ t.Helper()
+
+ if r.Method != "CONNECT" {
+ t.Errorf("Method = %q; want GET", r.Method)
+ }
+ if r.Header.Get(":protocol") != "pingpong" {
+ t.Errorf(":protocol = %q; want pingpong", r.Header.Get("Protocol"))
+ }
+ w.WriteHeader(http.StatusOK)
+ w.(http.Flusher).Flush()
+
+ var buf bytes.Buffer
+ if n, err := io.Copy(&buf, io.LimitReader(r.Body, 4)); err != nil || n != 4 {
+ t.Errorf("Read = %d, %v; want 4, nil", n, err)
+ }
+ if got, want := buf.String(), "ping"; got != want {
+ t.Errorf("Read %q; want %q", got, want)
+ }
+ if _, err := io.WriteString(w, "pong"); err != nil {
+ t.Errorf("WriteString = %v; want nil", err)
+ }
+ w.(http.Flusher).Flush()
+
+ io.Copy(io.Discard, r.Body)
+}
+
func TestServer_Request_Get_PathSlashes(t *testing.T) {
testServerRequest(t, func(st *serverTester) {
st.writeHeaders(HeadersFrameParam{
diff --git a/http2/transport.go b/http2/transport.go
index b20c749..879aaf4 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -312,6 +312,7 @@
closing bool
closed bool
seenSettings bool // true if we've seen a settings frame, false otherwise
+ seenSettingsCh chan struct{} // closed if we've seen a settings frame
wantSettingsAck bool // we sent a SETTINGS frame and haven't heard back
goAway *GoAwayFrame // if non-nil, the GoAwayFrame we received
goAwayDebug string // goAway frame's debug data, retained as a string
@@ -329,6 +330,7 @@
peerMaxHeaderListSize uint64
peerMaxHeaderTableSize uint32
initialWindowSize uint32
+ enableConnectProtocol bool
// reqHeaderMu is a 1-element semaphore channel controlling access to sending new requests.
// Write to reqHeaderMu to lock it, read from it to unlock.
@@ -747,6 +749,7 @@
peerMaxHeaderListSize: 0xffffffffffffffff, // "infinite", per spec. Use 2^64-1 instead.
streams: make(map[uint32]*clientStream),
singleUse: singleUse,
+ seenSettingsCh: make(chan struct{}),
wantSettingsAck: true,
pings: make(map[[8]byte]chan struct{}),
reqHeaderMu: make(chan struct{}, 1),
@@ -1173,6 +1176,12 @@
return 0
}
+func (cc *ClientConn) enableConnectProtcolLocked() bool {
+ cc.mu.Lock()
+ defer cc.mu.Unlock()
+ return cc.enableConnectProtocol
+}
+
// checkConnHeaders checks whether req has any invalid connection-level headers.
// per RFC 7540 section 8.1.2.2: Connection-Specific Header Fields.
// Certain headers are special-cased as okay but not transmitted later.
@@ -1370,6 +1379,7 @@
if isConnectionCloseRequest(req) {
cc.doNotReuse = true
}
+ seenSettings := cc.seenSettings
cc.mu.Unlock()
// TODO(bradfitz): this is a copy of the logic in net/http. Unify somewhere?
@@ -1401,6 +1411,17 @@
}
}
+ if !seenSettings && isExtendedConnect(req) {
+ // We need to wait for the first settings frame to be presented on the
+ // server so that extended CONNECT requests don't fail on newly established
+ // connections.
+ select {
+ case <-cc.seenSettingsCh:
+ case <-req.Context().Done():
+ return req.Context().Err()
+ }
+ }
+
// Past this point (where we send request headers), it is possible for
// RoundTrip to return successfully. Since the RoundTrip contract permits
// the caller to "mutate or reuse" the Request after closing the Response's Body,
@@ -1876,7 +1897,7 @@
}
var path string
- if req.Method != "CONNECT" {
+ if req.Method != "CONNECT" || isExtendedConnect(req) {
path = req.URL.RequestURI()
if !validPseudoPath(path) {
orig := path
@@ -1895,7 +1916,11 @@
// potentially pollute our hpack state. (We want to be able to
// continue to reuse the hpack encoder for future requests)
for k, vv := range req.Header {
- if !httpguts.ValidHeaderFieldName(k) {
+ if k == ":protocol" {
+ if !cc.enableConnectProtcolLocked() {
+ return nil, fmt.Errorf("server did not advertise ENABLE_CONNECT_PROTOCOL: %w", http.ErrNotSupported)
+ }
+ } else if !httpguts.ValidHeaderFieldName(k) {
return nil, fmt.Errorf("invalid HTTP header name %q", k)
}
for _, v := range vv {
@@ -1918,7 +1943,7 @@
m = http.MethodGet
}
f(":method", m)
- if req.Method != "CONNECT" {
+ if req.Method != "CONNECT" || isExtendedConnect(req) {
f(":path", path)
f(":scheme", req.URL.Scheme)
}
@@ -2852,6 +2877,13 @@
case SettingHeaderTableSize:
cc.henc.SetMaxDynamicTableSize(s.Val)
cc.peerMaxHeaderTableSize = s.Val
+ case SettingEnableConnectProtocol:
+ if cc.enableConnectProtocol && s.Val == 0 {
+ // A sender MUST NOT send a SETTINGS_ENABLE_CONNECT_PROTOCOL parameter
+ // with the value of 0 after previously sending a value of 1.
+ return ConnectionError(ErrCodeProtocol)
+ }
+ cc.enableConnectProtocol = s.Val == 1
default:
cc.vlogf("Unhandled Setting: %v", s)
}
@@ -2870,6 +2902,7 @@
cc.maxConcurrentStreams = defaultMaxConcurrentStreams
}
cc.seenSettings = true
+ close(cc.seenSettingsCh)
}
return nil
@@ -3126,6 +3159,12 @@
return 0
}
+func isExtendedConnect(req *http.Request) bool {
+ return req.Method == "CONNECT" &&
+ req.Header != nil &&
+ len(req.Header[":protocol"]) > 0
+}
+
func traceGetConn(req *http.Request, hostPort string) {
trace := httptrace.ContextClientTrace(req.Context())
if trace == nil || trace.GetConn == nil {
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 9984848..4a63f3e 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -1189,6 +1189,105 @@
}
}
+func TestTransportExtendedConnect_NilBody(t *testing.T) {
+ reqDone := make(chan struct{})
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+ defer close(reqDone)
+ }, optOnlyServer)
+ defer st.Close()
+
+ req, err := http.NewRequest("CONNECT", st.ts.URL, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ req.Header[":protocol"] = []string{"pingpong"}
+
+ tr := &Transport{TLSClientConfig: tlsConfigInsecure}
+ defer tr.CloseIdleConnections()
+ c := &http.Client{Transport: tr}
+
+ resp, err := c.Do(req)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer resp.Body.Close()
+
+ <-reqDone
+}
+
+func TestTransportExtendedConnect_PipeBody(t *testing.T) {
+ reqDone := make(chan struct{})
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+ defer close(reqDone)
+ handleServerPingPong(t, w, r)
+ }, optOnlyServer)
+ defer st.Close()
+
+ rout, wc := io.Pipe()
+
+ req, err := http.NewRequest("CONNECT", st.ts.URL, rout)
+ if err != nil {
+ t.Fatal(err)
+ }
+ req.Header[":protocol"] = []string{"pingpong"}
+
+ tr := &Transport{TLSClientConfig: tlsConfigInsecure}
+ defer tr.CloseIdleConnections()
+ c := &http.Client{Transport: tr}
+
+ resp, err := c.Do(req)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer resp.Body.Close()
+
+ handleClientPingPong(t, resp.Body, wc)
+ wc.Close()
+ <-reqDone
+}
+
+func handleClientPingPong(t *testing.T, r io.Reader, w io.Writer) {
+ t.Helper()
+ if _, err := io.WriteString(w, "ping"); err != nil {
+ t.Fatal(err)
+ }
+ var buf bytes.Buffer
+ if n, err := io.Copy(&buf, io.LimitReader(r, 4)); err != nil || n != 4 {
+ t.Errorf("Read = %d, %v; want 4, nil", n, err)
+ }
+ if got, want := buf.String(), "pong"; got != want {
+ t.Errorf("Read %q; want %q", got, want)
+ }
+ if buf.String() != "pong" {
+ t.Errorf("Read = %q; want pong", buf.String())
+ }
+}
+
+func TestTransportExtendedConnectDisabled(t *testing.T) {
+ testHookDisableConnectProtocol = true
+ t.Cleanup(func() {
+ testHookDisableConnectProtocol = false
+ })
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+ t.Error("Should have resulted in protocol error")
+ })
+ defer st.Close()
+
+ req, err := http.NewRequest("CONNECT", st.ts.URL, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ req.Header[":protocol"] = []string{"pingpong"}
+
+ tr := &Transport{TLSClientConfig: tlsConfigInsecure}
+ defer tr.CloseIdleConnections()
+ c := &http.Client{Transport: tr}
+
+ if _, err := c.Do(req); !errors.Is(err, http.ErrNotSupported) {
+ t.Fatalf("Should have resulted in not supported error. Got err: %v", err)
+ }
+}
+
type headerType int
const (
To view, visit change 508238. To unsubscribe, or for help writing mail filters, visit settings.
Mike Danese (Gerrit)
Jul 6, 2023, 10:09:21 PM7/6/23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Mike Danese uploaded patch set #2 to this change.
To view, visit change 508238. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Apr 11, 2024, 1:18:52 PM4/11/24
to goph...@pubsubhelper.golang.org, Tom Bergan, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Mike Danese and Tom Bergan
Damien Neil added 5 comments![]( "Open in Gerrit")
Patchset-level comments
File http2/server.go
Line 83, Patchset 2 (Latest):
testHookDisableConnectProtocol boolDamien Neil . unresolved
I believe this needs to be hooked up to a GODEBUG. https://github.com/golang/go/issues/53208#issuecomment-1995111843
Line 2160, Patchset 2 (Latest):
isExtendedConnect := isConnect && f.PseudoValue("protocol") != "" && !testHookDisableConnectProtocolDamien Neil . unresolved
I don't think the `!testHookDisableConnectProtocol` path is right here: If we're disabling extended CONNECT, then we should return an error if the `:authority` header is set.
I think the following section should be broken out into three conditions: CONNECT, Extended CONNECT, and other.
```
switch {
case isConnect && f.PseudoValue("protocol") == "":
// RFC 7540 8.3. The CONNECT Method
if rp.path != "" || rp.scheme != "" || rp.authority == "" {
return nil, nil, sc.countError("bad_connect", streamError(f.StreamID, ErrCodeProtocol))
}
case isConnect:
// RFC 8441 4. The Extended CONNECT Method
if rp.path == "" || (rp.scheme != "https" && rp.scheme != "http") {
return nil, nil, sc.countError("bad_extended_connect", streamError(f.StreamID, ErrCodeProtocol))
}
default:
// RFC 7540 8.1.2.3. Request Pseudo-Header Fields
if rp.method == "" || rp.path == "" || (rp.scheme != "https" && rp.scheme != "http") {
return nil, nil, sc.countError("bad_extended_connect", streamError(f.StreamID, ErrCodeProtocol))
}
}
```
Line 2165, Patchset 2 (Latest):
} else if rp.method == "" || rp.path == "" || (rp.scheme != "https" && rp.scheme != "http") {Damien Neil . unresolved
Should we enforce a scheme of http/https for extended CONNECT, or just the presence of a scheme?
File http2/transport.go
Line 3164, Patchset 2 (Latest):
req.Header != nil &&Damien Neil . unresolved
the `req.Header != nil` isn't necessary, since accessing a nil map is okay
Related details
Attention is currently required from:
- Mike Danese
- Tom Bergan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages