Skip to content

Commit 2952465

Browse files
committed
Revert forked Director logic post go 1.23 + CVE fix
Golang patched the 100-continue bug this was working around. We've reverted our custom fix, but kept integration tests to ensure it doesn't regress.
1 parent 8efd1ed commit 2952465

File tree

7 files changed

+53
-345
lines changed

7 files changed

+53
-345
lines changed

src/code.cloudfoundry.org/gorouter/config/config.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -484,14 +484,13 @@ type Config struct {
484484
LoadBalance string `yaml:"balancing_algorithm,omitempty"`
485485
LoadBalanceAZPreference string `yaml:"balancing_algorithm_az_preference,omitempty"`
486486

487-
DisableKeepAlives bool `yaml:"disable_keep_alives"`
488-
MaxIdleConns int `yaml:"max_idle_conns,omitempty"`
489-
MaxIdleConnsPerHost int `yaml:"max_idle_conns_per_host,omitempty"`
490-
MaxRequestHeaderBytes int `yaml:"max_request_header_bytes"`
491-
MaxResponseHeaderBytes int `yaml:"max_response_header_bytes"`
492-
MaxRequestHeaders int `yaml:"max_request_headers"`
493-
MaxResponseHeaders int `yaml:"max_response_headers"`
494-
KeepAlive100ContinueRequests bool `yaml:"keep_alive_100_continue_requests"`
487+
DisableKeepAlives bool `yaml:"disable_keep_alives"`
488+
MaxIdleConns int `yaml:"max_idle_conns,omitempty"`
489+
MaxIdleConnsPerHost int `yaml:"max_idle_conns_per_host,omitempty"`
490+
MaxRequestHeaderBytes int `yaml:"max_request_header_bytes"`
491+
MaxResponseHeaderBytes int `yaml:"max_response_header_bytes"`
492+
MaxRequestHeaders int `yaml:"max_request_headers"`
493+
MaxResponseHeaders int `yaml:"max_response_headers"`
495494

496495
HTTPRewrite HTTPRewrite `yaml:"http_rewrite,omitempty"`
497496

src/code.cloudfoundry.org/gorouter/config/config_test.go

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -889,14 +889,6 @@ backends:
889889
Expect(config.DisableKeepAlives).To(BeFalse())
890890
})
891891

892-
It("sets KeepAlive100ContinueRequests", func() {
893-
var b = []byte("keep_alive_100_continue_requests: true")
894-
err := config.Initialize(b)
895-
Expect(err).ToNot(HaveOccurred())
896-
897-
Expect(config.KeepAlive100ContinueRequests).To(BeTrue())
898-
})
899-
900892
It("sets MaxIdleConns", func() {
901893
var b = []byte("max_idle_conns: 200")
902894
err := config.Initialize(b)

src/code.cloudfoundry.org/gorouter/handlers/proxy_picker.go

Lines changed: 0 additions & 33 deletions
This file was deleted.

src/code.cloudfoundry.org/gorouter/handlers/proxy_picker_test.go

Lines changed: 0 additions & 84 deletions
This file was deleted.

src/code.cloudfoundry.org/gorouter/integration/main_test.go

Lines changed: 45 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,20 +1193,23 @@ var _ = Describe("Router Integration", func() {
11931193
runningApp.Stop()
11941194
})
11951195

1196-
var testRequest = func(body string) int {
1196+
var testRequest = func(header, body string) net.Conn {
11971197
conn, err := net.Dial("tcp", fmt.Sprintf("localhost:%d", proxyPort))
11981198
Expect(err).ToNot(HaveOccurred())
11991199
conn.SetReadDeadline(time.Now().Add(20 * time.Second))
12001200

12011201
connWriter := bufio.NewWriter(conn)
12021202

1203-
connWriter.Write([]byte(body))
1203+
connWriter.Write([]byte(header))
12041204
connWriter.Flush()
12051205

1206-
connReader := bufio.NewReader(conn)
1207-
resp, err := http.ReadResponse(connReader, &http.Request{})
1208-
Expect(err).NotTo(HaveOccurred())
1209-
return resp.StatusCode
1206+
if body != "" {
1207+
time.Sleep(100 * time.Millisecond)
1208+
connWriter.Write([]byte(body))
1209+
connWriter.Flush()
1210+
1211+
}
1212+
return conn
12101213
}
12111214

12121215
It("resets response for new request", func() {
@@ -1219,18 +1222,32 @@ var _ = Describe("Router Integration", func() {
12191222
go func() {
12201223
defer close(badRequestsDone)
12211224
defer GinkgoRecover()
1222-
for i := 0; i < 100; i++ {
1223-
statusCode := testRequest(
1224-
"OPTIONS / HTTP/1.1\r\n" +
1225-
fmt.Sprintf("Host: %s\r\n", appRoute) +
1226-
"Expect: 100-Continue\r\n" +
1227-
"Content-Type: text/plain\r\n" +
1228-
fmt.Sprintf("Content-Length: %d\r\n", 5) +
1229-
"\r\n" +
1230-
"hello",
1225+
for i := range 100 {
1226+
conn := testRequest(
1227+
"OPTIONS / HTTP/1.1\r\n"+
1228+
fmt.Sprintf("Host: %s\r\n", appRoute)+
1229+
"Expect: 100-Continue\r\n"+
1230+
"Content-Type: text/plain\r\n"+
1231+
fmt.Sprintf("Content-Length: %d\r\n", 5)+
1232+
"\r\n",
1233+
"hello",
12311234
)
1232-
Expect(statusCode).To(Equal(http.StatusMethodNotAllowed))
1233-
time.Sleep(50 * time.Millisecond)
1235+
responseReader := bufio.NewReader(conn)
1236+
1237+
var lines []string
1238+
for range 3 {
1239+
line, err := responseReader.ReadString('\n')
1240+
Expect(err).ToNot(HaveOccurred())
1241+
lines = append(lines, line)
1242+
}
1243+
conn.Close()
1244+
if lines[0] == "HTTP/1.1 100 Continue\r\n" {
1245+
Expect(lines[2]).To(Equal("HTTP/1.1 405 Method Not Allowed\r\n"))
1246+
} else {
1247+
Expect(lines[0]).To(Equal("HTTP/1.1 405 Method Not Allowed\r\n"))
1248+
}
1249+
1250+
time.Sleep(25 * time.Millisecond)
12341251
if i == 0 {
12351252
close(badRequestsStarted)
12361253
}
@@ -1244,15 +1261,21 @@ var _ = Describe("Router Integration", func() {
12441261
go func() {
12451262
defer close(goodRequestsDone)
12461263
defer GinkgoRecover()
1247-
for i := 0; i < 10; i++ {
1248-
statusCode := testRequest(
1249-
"GET / HTTP/1.1\r\n" +
1250-
fmt.Sprintf("Host: %s\r\n", appRoute) +
1264+
for range 50 {
1265+
conn := testRequest(
1266+
"GET / HTTP/1.1\r\n"+
1267+
fmt.Sprintf("Host: %s\r\n", appRoute)+
12511268
"\r\n",
1269+
"",
12521270
)
1253-
Expect(statusCode).To(Equal(http.StatusOK))
1271+
responseReader := bufio.NewReader(conn)
1272+
line, err := responseReader.ReadString('\n')
1273+
Expect(err).ToNot(HaveOccurred())
1274+
conn.Close()
1275+
Expect(line).To(Equal("HTTP/1.1 200 OK\r\n"))
12541276
time.Sleep(100 * time.Millisecond)
12551277
}
1278+
12561279
}()
12571280

12581281
Eventually(goodRequestsDone, "10s", "1s").Should(BeClosed())

src/code.cloudfoundry.org/gorouter/proxy/proxy.go

Lines changed: 1 addition & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -127,23 +127,6 @@ func NewProxy(
127127
ModifyResponse: p.modifyResponse,
128128
}
129129

130-
// by default, we should close 100-continue requests for safety
131-
// this requires a second proxy because Director and Rewrite cannot coexist
132-
// additionally, Director() is called before hop-by-hop headers are sanitized
133-
// whereas Rewrite is after, and this is where `Connection: close` can be added
134-
expect100ContinueRProxy := &httputil.ReverseProxy{
135-
Rewrite: p.setupProxyRequestClose100Continue,
136-
Transport: prt,
137-
FlushInterval: 50 * time.Millisecond,
138-
BufferPool: p.bufferPool,
139-
ModifyResponse: p.modifyResponse,
140-
}
141-
142-
// if we want to not force close 100-continue requests, use the normal rproxy
143-
if cfg.KeepAlive100ContinueRequests {
144-
expect100ContinueRProxy = rproxy
145-
}
146-
147130
routeServiceHandler := handlers.NewRouteService(routeServiceConfig, registry, logger, errorWriter)
148131

149132
zipkinHandler := handlers.NewZipkin(cfg.Tracing.EnableZipkin, logger)
@@ -191,7 +174,7 @@ func NewProxy(
191174
})
192175
n.Use(routeServiceHandler)
193176
n.Use(p)
194-
n.Use(handlers.NewProxyPicker(rproxy, expect100ContinueRProxy))
177+
n.UseHandler(rproxy)
195178

196179
return n
197180
}
@@ -277,44 +260,6 @@ func (p *proxy) setupProxyRequest(target *http.Request) {
277260
target.Header.Del(router_http.CfAppInstance)
278261
}
279262

280-
func (p *proxy) setupProxyRequestClose100Continue(target *httputil.ProxyRequest) {
281-
reqInfo, err := handlers.ContextRequestInfo(target.In)
282-
if err != nil {
283-
log.Panic(p.logger, "request-info-err", log.ErrAttr(err))
284-
return
285-
}
286-
reqInfo.BackendReqHeaders = target.Out.Header
287-
288-
target.Out.URL.Scheme = "http"
289-
target.Out.URL.Host = target.In.Host
290-
target.Out.URL.ForceQuery = false
291-
target.Out.URL.Opaque = target.In.RequestURI
292-
293-
if strings.HasPrefix(target.In.RequestURI, "//") {
294-
path := escapePathAndPreserveSlashes(target.In.URL.Path)
295-
target.Out.URL.Opaque = "//" + target.In.Host + path
296-
297-
if len(target.In.URL.Query()) > 0 {
298-
target.Out.URL.Opaque = target.Out.URL.Opaque + "?" + target.In.URL.Query().Encode()
299-
}
300-
}
301-
target.Out.URL.RawQuery = ""
302-
303-
setRequestXRequestStart(target.Out)
304-
target.Out.Header.Del(router_http.CfAppInstance)
305-
306-
// always set connection close on 100-continue requests
307-
target.Out.Header.Set("Connection", "close")
308-
309-
target.Out.Header["X-Forwarded-For"] = target.In.Header["X-Forwarded-For"]
310-
target.Out.Header["Forwarded"] = target.In.Header["Forwarded"]
311-
// Takes care of setting the X-Forwarded-For header properly. Also sets the X-Forwarded-Proto
312-
// which we overwrite again.
313-
target.SetXForwarded()
314-
target.Out.Header["X-Forwarded-Proto"] = target.In.Header["X-Forwarded-Proto"]
315-
target.Out.Header["X-Forwarded-Host"] = target.In.Header["X-Forwarded-Host"]
316-
}
317-
318263
func setRequestXRequestStart(request *http.Request) {
319264
if _, ok := request.Header[http.CanonicalHeaderKey("X-Request-Start")]; !ok {
320265
request.Header.Set("X-Request-Start", strconv.FormatInt(time.Now().UnixNano()/1e6, 10))

0 commit comments

Comments
 (0)