strip transport headers from function response (#1489)

we weren't stripping transport headers from function response before copying
to the client response. this means e.g. a function could send a "Connection:
close" when fn wants to use a keep alive.

this also moved this to common since we're doing it in 3 places. I know that
it's not the ideal ideal big O performance and we can pedant about it, but
we're talking about 7 things here. I'm open to ideas about what this should
look like, I ripped it out of the stdlib and it seems okay. changed the
trigger stuff to use this too, as it was copied. added tests to cover all 3 of
these spots.

the only behavior change is no longer stripping 'authorization' header when
passing headers from the client request into the function. authorization seems
like a useful header for users that want to use auth in their functions, I
think if we need to use this one in the backend we should more carefully strip
out the parts we use before passing them on instead of relying on the agent to
strip the header to not leak anything into a function.

closes #1488

Author: rdallman

api/agent/agent.go

@@ -605,16 +605,6 @@ func (s *hotSlot) exec(ctx context.Context, call *call) error {
 	return err
 }
 
-var removeHeaders = map[string]bool{
-	"connection":        true,
-	"keep-alive":        true,
-	"trailer":           true,
-	"transfer-encoding": true,
-	"te":                true,
-	"upgrade":           true,
-	"authorization":     true,
-}
-
 func createUDSRequest(ctx context.Context, call *call) *http.Request {
 	req, err := http.NewRequest("POST", "http://localhost/call", call.req.Body)
 	if err != nil {
@@ -625,12 +615,13 @@ func createUDSRequest(ctx context.Context, call *call) *http.Request {
 	// it properly and close connections at the end, e.g. when using UDS.
 	req = req.WithContext(ctx)
 
+	// remove transport headers before passing to function
+	common.StripHopHeaders(call.req.Header)
+
 	req.Header = make(http.Header)
 	for k, vs := range call.req.Header {
-		if !removeHeaders[strings.ToLower(k)] {
-			for _, v := range vs {
-				req.Header.Add(k, v)
-			}
+		for _, v := range vs {
+			req.Header.Add(k, v)
 		}
 	}
 
@@ -723,6 +714,9 @@ func (s *hotSlot) writeResp(ctx context.Context, max uint64, resp *http.Response
 
 	rw = newSizerRespWriter(max, rw)
 
+	// remove transport headers before copying to client response
+	common.StripHopHeaders(resp.Header)
+
 	// WARNING: is the following header copy safe?
 	// if we're writing directly to the response writer, we need to set headers
 	// and only copy the body. resp.Write would copy a full

api/common/io_utils.go

@@ -2,9 +2,51 @@ package common
 
 import (
 	"io"
+	"net/http"
 	"sync"
 )
 
+// StripHopHeaders removes transport related headers.
+func StripHopHeaders(hdr http.Header) {
+	// Remove hop-by-hop headers to the backend. Especially
+	// important is "Connection" because we want a persistent
+	// connection, regardless of what the client sent to us.
+	for _, h := range hopHeaders {
+		hv := hdr.Get(h)
+		if hv == "" {
+			continue
+		}
+		if h == "Te" && hv == "trailers" {
+			// Issue 21096: tell backend applications that
+			// care about trailer support that we support
+			// trailers. (We do, but we don't go out of
+			// our way to advertise that unless the
+			// incoming client request thought it was
+			// worth mentioning)
+			continue
+		}
+		hdr.Del(h)
+	}
+}
+
+// Hop-by-hop headers. These are removed when sent to the backend.
+// As of RFC 7230, hop-by-hop headers are required to appear in the
+// Connection header field. These are the headers defined by the
+// obsoleted RFC 2616 (section 13.5.1) and are used for backward
+// compatibility.
+// stolen: net/http/httputil/reverseproxy.go
+var hopHeaders = []string{
+	"Connection",
+	"Proxy-Connection", // non-standard but still sent by libcurl and rejected by e.g. google
+	"Keep-Alive",
+	"Proxy-Authenticate",
+	"Proxy-Authorization",
+	"Te",      // canonicalized version of "TE"
+	"Trailer", // not Trailers per URL above; https://www.rfc-editor.org/errata_search.php?eid=4522
+	"Transfer-Encoding",
+	"Upgrade",
+}
+
 // NoopReadWriteCloser implements io.ReadWriteCloser, discarding all bytes, Read always returns EOF
 type NoopReadWriteCloser struct{}
 

api/server/runner_fninvoke_test.go

@@ -164,8 +164,11 @@ func TestFnInvokeRunnerExecution(t *testing.T) {
 
 	srv := testServer(ds, rnr, ServerTypeFull, LimitRequestBody(32256))
 
+	inStripHeaders := map[string][]string{"Keep-Alive": {"true"}}
+
 	expHeaders := map[string][]string{"Content-Type": {"application/json; charset=utf-8"}}
 	expCTHeaders := map[string][]string{"Content-Type": {"foo/bar"}}
+	expStripHeaders := map[string][]string{"Keep-Alive": {""}}
 
 	// Checking for EndOfLogs currently depends on scheduling of go-routines (in docker/containerd) that process stderr & stdout.
 	// Therefore, not testing for EndOfLogs for hot containers (which has complex I/O processing) anymore.
@@ -186,41 +189,46 @@ func TestFnInvokeRunnerExecution(t *testing.T) {
 	bigoutput := `{"echoContent": "_TRX_ID_", "isDebug": true, "trailerRepeat": 1000}` // 1000 trailers to exceed 2K
 
 	bighdroutput := `{"echoContent": "_TRX_ID_", "isDebug": true, "returnHeaders": {"zoo": ["` + strings.Repeat("a", 1024) + `"]}}` // big header to exceed
+	striphdr := `{"echoContent": "_TRX_ID_", "isDebug": true, "returnHeaders": {"Keep-Alive": ["true"]}}`                           // this should get stripped
+	striphdrin := `{"echoContent": "_TRX_ID_", "isDebug": true, "expectHeaders": {"Keep-Alive": [""]}}`                             // this should get stripped
 
 	smalloutput := `{"echoContent": "_TRX_ID_", "isDebug": true, "responseContentType":"application/json; charset=utf-8", "trailerRepeat": 1}` // 1 trailer < 2K
 
 	testCases := []struct {
 		path               string
 		body               string
+		headers            map[string][]string
 		method             string
 		expectedCode       int
 		expectedHeaders    map[string][]string
 		expectedErrSubStr  string
 		expectedLogsSubStr []string
 	}{
-		{"/invoke/http_stream_fn_id", ok, http.MethodPost, http.StatusOK, expHeaders, "", nil},
-		// NOTE: we can't test bad response framing anymore easily (eg invalid http response), should we even worry about it?
-		{"/invoke/http_stream_fn_id", respTypeLie, http.MethodPost, http.StatusOK, expCTHeaders, "", nil},
-		{"/invoke/http_stream_fn_id", crasher, http.MethodPost, http.StatusBadGateway, expHeaders, "error receiving function response", nil},
-		// XXX(reed): we could stop buffering function responses so that we can stream things?
-		{"/invoke/http_stream_fn_id", bighdroutput, http.MethodPost, http.StatusBadGateway, nil, "function response header too large", nil},
-		{"/invoke/http_stream_fn_id", bigoutput, http.MethodPost, http.StatusBadGateway, nil, "function response too large", nil},
-		{"/invoke/http_stream_fn_id", smalloutput, http.MethodPost, http.StatusOK, expHeaders, "", nil},
-		// XXX(reed): meh we really should try to get oom out, but maybe it's better left to the logs?
-		{"/invoke/http_stream_fn_id", oomer, http.MethodPost, http.StatusBadGateway, nil, "error receiving function response", nil},
-		{"/invoke/http_stream_fn_id", bigbuf, http.MethodPost, http.StatusRequestEntityTooLarge, nil, "", nil},
-
-		{"/invoke/dne_fn_id", ``, http.MethodPost, http.StatusNotFound, nil, "pull access denied", nil},
-		{"/invoke/dnereg_fn_id", ``, http.MethodPost, http.StatusBadGateway, nil, "connection refused", nil},
-
-		// XXX(reed): what are these?
-		{"/invoke/http_stream_fn_id", multiLog, http.MethodPost, http.StatusOK, nil, "", multiLogExpectHot},
-
-		{"/invoke/fail_fn_quick", ok, http.MethodPost, http.StatusBadGateway, nil, "container failed to initialize", nil},
-		{"/invoke/fail_fn_timeout", ok, http.MethodPost, http.StatusGatewayTimeout, nil, "Container initialization timed out", nil},
-		{"/invoke/fn_id", ok, http.MethodPut, http.StatusMethodNotAllowed, nil, "Method not allowed", nil},
-
-		{"/invoke/bigmem", ok, http.MethodPost, http.StatusBadRequest, nil, "cannot be allocated", nil},
+		{"/invoke/http_stream_fn_id", ok, nil, http.MethodPost, http.StatusOK, expHeaders, "", nil},
+		// NOTE: nil, we can't test bad response framing anymore easily (eg invalid http response), should we even worry about it?
+		{"/invoke/http_stream_fn_id", respTypeLie, nil, http.MethodPost, http.StatusOK, expCTHeaders, "", nil},
+		{"/invoke/http_stream_fn_id", crasher, nil, http.MethodPost, http.StatusBadGateway, expHeaders, "error receiving function response", nil},
+		// XXX(reed): nil, we could stop buffering function responses so that we can stream things?
+		{"/invoke/http_stream_fn_id", bighdroutput, nil, http.MethodPost, http.StatusBadGateway, nil, "function response header too large", nil},
+		{"/invoke/http_stream_fn_id", striphdr, nil, http.MethodPost, http.StatusOK, expStripHeaders, "", nil},
+		{"/invoke/http_stream_fn_id", striphdrin, inStripHeaders, http.MethodPost, http.StatusOK, nil, "", nil},
+		{"/invoke/http_stream_fn_id", bigoutput, nil, http.MethodPost, http.StatusBadGateway, nil, "function response too large", nil},
+		{"/invoke/http_stream_fn_id", smalloutput, nil, http.MethodPost, http.StatusOK, expHeaders, "", nil},
+		// XXX(reed): nil, meh we really should try to get oom out, but maybe it's better left to the logs?
+		{"/invoke/http_stream_fn_id", oomer, nil, http.MethodPost, http.StatusBadGateway, nil, "error receiving function response", nil},
+		{"/invoke/http_stream_fn_id", bigbuf, nil, http.MethodPost, http.StatusRequestEntityTooLarge, nil, "", nil},
+
+		{"/invoke/dne_fn_id", ``, nil, http.MethodPost, http.StatusNotFound, nil, "pull access denied", nil},
+		{"/invoke/dnereg_fn_id", ``, nil, http.MethodPost, http.StatusBadGateway, nil, "connection refused", nil},
+
+		// XXX(reed): nil, nil, what are these?
+		{"/invoke/http_stream_fn_id", multiLog, nil, http.MethodPost, http.StatusOK, nil, "", multiLogExpectHot},
+
+		{"/invoke/fail_fn_quick", ok, nil, http.MethodPost, http.StatusBadGateway, nil, "container failed to initialize", nil},
+		{"/invoke/fail_fn_timeout", ok, nil, http.MethodPost, http.StatusGatewayTimeout, nil, "Container initialization timed out", nil},
+		{"/invoke/fn_id", ok, nil, http.MethodPut, http.StatusMethodNotAllowed, nil, "Method not allowed", nil},
+
+		{"/invoke/bigmem", ok, nil, http.MethodPost, http.StatusBadRequest, nil, "cannot be allocated", nil},
 	}
 
 	callIds := make([]string, len(testCases))
@@ -229,7 +237,11 @@ func TestFnInvokeRunnerExecution(t *testing.T) {
 		t.Run(fmt.Sprintf("Test_%d_%s", i, strings.Replace(test.path, "/", "_", -1)), func(t *testing.T) {
 			trx := fmt.Sprintf("_trx_%d_", i)
 			body := strings.NewReader(strings.Replace(test.body, "_TRX_ID_", trx, 1))
-			_, rec := routerRequest(t, srv.Router, test.method, test.path, body)
+			req := createRequest(t, test.method, test.path, body)
+			if test.headers != nil {
+				req.Header = test.headers
+			}
+			_, rec := routerRequest2(t, srv.Router, req)
 			respBytes, _ := ioutil.ReadAll(rec.Body)
 			respBody := string(respBytes)
 			maxBody := len(respBody)

api/server/runner_httptrigger.go

@@ -3,11 +3,11 @@ package server
 import (
 	"fmt"
 	"net/http"
-	"net/textproto"
 	"strconv"
 	"strings"
 
 	"github.com/fnproject/fn/api"
+	"github.com/fnproject/fn/api/common"
 	"github.com/fnproject/fn/api/models"
 	"github.com/gin-gonic/gin"
 	"go.opencensus.io/tag"
@@ -135,15 +135,6 @@ func (trw *triggerResponseWriter) WriteHeader(serviceStatus int) {
 	trw.inner.WriteHeader(finalStatus)
 }
 
-var skipTriggerHeaders = map[string]bool{
-	"Connection":        true,
-	"Keep-Alive":        true,
-	"Trailer":           true,
-	"Transfer-Encoding": true,
-	"TE":                true,
-	"Upgrade":           true,
-}
-
 func reqURL(req *http.Request) string {
 	if req.URL.Scheme == "" {
 		if req.TLS == nil {
@@ -164,12 +155,11 @@ func (s *Server) ServeHTTPTrigger(c *gin.Context, app *models.App, fn *models.Fn
 	// transpose trigger headers into the request
 	req := c.Request
 	headers := make(http.Header, len(req.Header))
+
+	// remove transport headers before decorating headers
+	common.StripHopHeaders(req.Header)
+
 	for k, vs := range req.Header {
-		// should be generally unnecessary but to be doubly sure.
-		k = textproto.CanonicalMIMEHeaderKey(k)
-		if skipTriggerHeaders[k] {
-			continue
-		}
 		switch k {
 		case "Content-Type":
 		default:

api/server/runner_httptrigger_test.go

@@ -262,11 +262,13 @@ func TestTriggerRunnerExecution(t *testing.T) {
 	// these tests are such a pita it's easier to comment most of them out. instead of fixing it i'm doing this fuck me yea
 	_, _, _, _, _, _, _, _, _, _, _ = expHeaders, expCTHeaders, multiLogExpectHot, crasher, oomer, ok, respTypeLie, multiLog, bigoutput, smalloutput, statusChecker
 
-	fooHeader := map[string][]string{"Content-Type": {"application/hateson"}, "Test-Header": {"foo"}}
+	// Keep-Alive should get stripped, Content-Type should not get framed, Test-Header should get framed
+	fooHeader := map[string][]string{"Content-Type": {"application/hateson"}, "Test-Header": {"foo"}, "Keep-Alive": {"true"}}
 	expFooHeaders := map[string][]string{"Content-Type": {"application/hateson"}, "Return-Header": {"foo", "bar"}}
 	expFooHeadersBody := `{"echoContent": "_TRX_ID_",
 		"expectHeaders": {
 			"Content-Type":["application/hateson"],
+			"Keep-Alive":[""],
 			"Test-Header":["foo"]
 		},
 		"returnHeaders": {