Add config.ForwardedRequestHeader (#295)

config.ForwardedRequestHeader allows amppkg to copy specified headers
from serveHTTP request to a fetch request. Hop-by-hop headers,
conditional request headers and Via cannot be included in it.

Author: shigeki

amppkg.example.toml

@@ -58,6 +58,10 @@ KeyFile = './pems/privkey.pem'
 # locking; consider this especially when utilizing network-mounted storage.
 OCSPCache = '/tmp/amppkg-ocsp'
 
+# The list of request header names to be forwarded in a fetch request.
+# Hop-by-hop headers, conditional request headers and Via cannot be included.
+ForwardedRequestHeaders = []
+
 # This is a simple level of validation, to guard against accidental
 # misconfiguration of the reverse proxy that sits in front of the packager.
 #

cmd/amppkg/main.go

@@ -127,7 +127,7 @@ func main() {
 	}
 
 	packager, err := signer.New(certs[0], key, config.URLSet, rtvCache, certCache.IsHealthy,
-		overrideBaseURL, /*requireHeaders=*/!*flagDevelopment)
+		overrideBaseURL, /*requireHeaders=*/!*flagDevelopment, config.ForwardedRequestHeaders)
 	if err != nil {
 		die(errors.Wrap(err, "building packager"))
 	}

cmd/gateway_server/server.go

@@ -112,7 +112,7 @@ func (s *gatewayServer) GenerateSXG(ctx context.Context, request *pb.SXGRequest)
 		},
 	}
 
-	packager, err := signer.New(certs[0], privateKey, urlSets, s.rtvCache, shouldPackage, signUrl, false)
+	packager, err := signer.New(certs[0], privateKey, urlSets, s.rtvCache, shouldPackage, signUrl, false, []string{})
 
 	if err != nil {
 		return errorToSXGResponse(err), nil

packager/signer/signer.go

@@ -45,16 +45,6 @@ const userAgent = "Mozilla/5.0 (Linux; Android 6.0.1; Nexus 5X Build/MMB29P) " +
 	"AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.96 Mobile " +
 	"Safari/537.36 (compatible; amppackager/0.0.0; +https://github.com/ampproject/amppackager)"
 
-// Conditional request headers that ServeHTTP may receive and need to be sent with fetchURL.
-// https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests#Conditional_headers
-var conditionalRequestHeaders = map[string]bool{
-	"If-Match":            true,
-	"If-None-Match":       true,
-	"If-Modified-Since":   true,
-	"If-Unmodified-Since": true,
-	"If-Range":            true,
-}
-
 // Advised against, per
 // https://tools.ietf.org/html/draft-yasskin-httpbis-origin-signed-exchanges-impl-00#section-4.1
 // and blocked in http://crrev.com/c/958945.
@@ -113,34 +103,6 @@ var getTransformerRequest = func(r *rtv.RTVCache, s, u string) *rpb.Request {
 // in that it allows multiple slashes, as well as initial and terminal slashes.
 var protocol = regexp.MustCompile("^[!#$%&'*+\\-.^_`|~0-9a-zA-Z/]+$")
 
-// The following hop-by-hop headers should be removed even when not specified
-// in Connection, for backwards compatibility with downstream servers that were
-// written against RFC 2616, and expect gateways to behave according to
-// https://tools.ietf.org/html/rfc2616#section-13.5.1. (Note: "Trailers" is a
-// typo there; should be "Trailer".)
-//
-// Connection header should also be removed per
-// https://tools.ietf.org/html/rfc7230#section-6.1.
-//
-// Proxy-Connection should also be deleted, per
-// https://github.com/WICG/webpackage/pull/339.
-var legacyHeaders = []string{"Connection", "Keep-Alive", "Proxy-Authenticate", "Proxy-Authorization", "Proxy-Connection", "TE", "Trailer", "Transfer-Encoding", "Upgrade"}
-
-// Remove hop-by-hop headers, per https://tools.ietf.org/html/rfc7230#section-6.1.
-func removeHopByHopHeaders(resp *http.Response) {
-	if connections, ok := resp.Header[http.CanonicalHeaderKey("Connection")]; ok {
-		for _, connection := range connections {
-			headerNames := util.Comma.Split(connection, -1)
-			for _, headerName := range headerNames {
-				resp.Header.Del(headerName)
-			}
-		}
-	}
-	for _, headerName := range legacyHeaders {
-		resp.Header.Del(headerName)
-	}
-}
-
 // Gets all values of the named header, joined on comma.
 func GetJoined(h http.Header, name string) string {
 	if values, ok := h[http.CanonicalHeaderKey(name)]; ok {
@@ -168,6 +130,7 @@ type Signer struct {
 	shouldPackage   func() bool
 	overrideBaseURL *url.URL
 	requireHeaders  bool
+	forwardedRequestHeaders []string
 }
 
 func noRedirects(req *http.Request, via []*http.Request) error {
@@ -176,14 +139,14 @@ func noRedirects(req *http.Request, via []*http.Request) error {
 
 func New(cert *x509.Certificate, key crypto.PrivateKey, urlSets []util.URLSet,
 	rtvCache *rtv.RTVCache, shouldPackage func() bool, overrideBaseURL *url.URL,
-	requireHeaders bool) (*Signer, error) {
+	requireHeaders bool, forwardedRequestHeaders []string) (*Signer, error) {
 	client := http.Client{
 		CheckRedirect: noRedirects,
 		// TODO(twifkak): Load-test and see if default transport settings are okay.
 		Timeout: 60 * time.Second,
 	}
 
-	return &Signer{cert, key, &client, urlSets, rtvCache, shouldPackage, overrideBaseURL, requireHeaders}, nil
+	return &Signer{cert, key, &client, urlSets, rtvCache, shouldPackage, overrideBaseURL, requireHeaders, forwardedRequestHeaders}, nil
 }
 
 func (this *Signer) fetchURL(fetch *url.URL, serveHTTPReq *http.Request) (*http.Request, *http.Response, *util.HTTPError) {
@@ -195,6 +158,14 @@ func (this *Signer) fetchURL(fetch *url.URL, serveHTTPReq *http.Request) (*http.
 		return nil, nil, util.NewHTTPError(http.StatusInternalServerError, "Error building request: ", err)
 	}
 	req.Header.Set("User-Agent", userAgent)
+	// copy forwardedRequestHeaders
+	for _, header := range this.forwardedRequestHeaders {
+		if http.CanonicalHeaderKey(header) == "Host" {
+			req.Host = serveHTTPReq.Host
+		} else if value := GetJoined(serveHTTPReq.Header, header); value != "" {
+			req.Header.Set(header, value)
+		}
+	}
 	// Golang's HTTP parser appears not to validate the protocol it parses
 	// from the request line, so we do so here.
 	if protocol.MatchString(serveHTTPReq.Proto) {
@@ -206,7 +177,7 @@ func (this *Signer) fetchURL(fetch *url.URL, serveHTTPReq *http.Request) (*http.
 		req.Header.Set("Via", via)
 	}
 	// Set conditional headers that were included in ServeHTTP's Request.
-	for header := range conditionalRequestHeaders {
+	for header := range util.ConditionalRequestHeaders {
 		if value := GetJoined(serveHTTPReq.Header, header); value != "" {
 			req.Header.Set(header, value)
 		}
@@ -215,7 +186,7 @@ func (this *Signer) fetchURL(fetch *url.URL, serveHTTPReq *http.Request) (*http.
 	if err != nil {
 		return nil, nil, util.NewHTTPError(http.StatusBadGateway, "Error fetching: ", err)
 	}
-	removeHopByHopHeaders(resp)
+	util.RemoveHopByHopHeaders(resp.Header)
 	return req, resp, nil
 }
 

packager/signer/signer_test.go

@@ -62,7 +62,8 @@ type SignerSuite struct {
 }
 
 func (this *SignerSuite) new(urlSets []util.URLSet) *Signer {
-	handler, err := New(pkgt.Certs[0], pkgt.Key, urlSets, &rtv.RTVCache{}, func() bool { return this.shouldPackage }, nil, true)
+	forwardedRequestHeaders := []string{"Host", "X-Foo"}
+	handler, err := New(pkgt.Certs[0], pkgt.Key, urlSets, &rtv.RTVCache{}, func() bool { return this.shouldPackage }, nil, true, forwardedRequestHeaders)
 	this.Require().NoError(err)
 	// Accept the self-signed certificate generated by the test server.
 	handler.client = this.httpsClient
@@ -78,6 +79,10 @@ func (this *SignerSuite) getP(t *testing.T, handler pkgt.AlmostHandler, target s
 		"AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}}, params)
 }
 
+func (this *SignerSuite) getFRH(t *testing.T, handler pkgt.AlmostHandler, target string, host string, header http.Header) *http.Response {
+	return pkgt.GetBHHP(t, handler, target, host, header, httprouter.Params{})
+}
+
 func (this *SignerSuite) getB(t *testing.T, handler pkgt.AlmostHandler, target string, body string) *http.Response {
 	return pkgt.GetBH(t, handler, target, strings.NewReader(body), http.Header{
 		"AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}})
@@ -101,6 +106,17 @@ func (this *SignerSuite) httpSignURL() string {
 	return u.String()
 }
 
+func (this *SignerSuite) certSubjectCN() string {
+	return pkgt.Certs[0].Subject.CommonName
+}
+
+func (this *SignerSuite) httpSignURL_CertSubjectCN() string {
+	u, err := url.Parse(this.certSubjectCN())
+	this.Require().NoError(err)
+	u.Scheme = "https"
+	return u.String()
+}
+
 func (this *SignerSuite) httpsURL() string {
 	return this.tlsServer.URL
 }
@@ -185,6 +201,54 @@ func (this *SignerSuite) TestSimple() {
 	this.Assert().Equal(append(payloadPrefix.Bytes(), transformedBody...), exchange.Payload)
 }
 
+func (this *SignerSuite) TestFetchSignWithForwardedRequestHeaders() {
+	urlSets := []util.URLSet{{
+		Sign:  &util.URLPattern{[]string{"https"}, "", this.certSubjectCN(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
+		Fetch: &util.URLPattern{[]string{"http"}, "", this.httpHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, boolPtr(true)},
+	}}
+	this.fakeHandler = func(resp http.ResponseWriter, req *http.Request) {
+		// Host and X-Foo headers are forwarded with forwardedRequestHeaders
+		this.Assert().Equal("www.example.com", req.Host)
+		this.Assert().Equal("foo", req.Header.Get("X-Foo"))
+		this.Assert().Equal("", req.Header.Get("X-Bar"))
+		this.lastRequest = req
+		resp.Header().Set("Content-Type", "text/html")
+		resp.Write(fakeBody)
+	}
+	// Request with ForwardedRequestHeaders
+	header := http.Header{"AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion},
+		"X-Foo": {"foo"}, "X-Bar": {"bar"}}
+	resp := this.getFRH(this.T(), this.new(urlSets),
+		"/priv/doc?fetch="+url.QueryEscape(this.httpURL()+fakePath)+"&sign="+url.QueryEscape(this.httpSignURL_CertSubjectCN()+fakePath),
+		"www.example.com", header)
+	this.Assert().Equal(fakePath, this.lastRequest.URL.String())
+	this.Assert().Equal(userAgent, this.lastRequest.Header.Get("User-Agent"))
+	this.Assert().Equal("1.1 amppkg", this.lastRequest.Header.Get("Via"))
+	this.Assert().Equal(http.StatusOK, resp.StatusCode, "incorrect status: %#v", resp)
+	this.Assert().Equal(fmt.Sprintf(`google;v="%d"`, transformer.SupportedVersions[0].Max), resp.Header.Get("AMP-Cache-Transform"))
+	this.Assert().Equal("nosniff", resp.Header.Get("X-Content-Type-Options"))
+	this.Assert().Equal("Accept, AMP-Cache-Transform", resp.Header.Get("Vary"))
+	exchange, err := signedexchange.ReadExchange(resp.Body)
+	this.Require().NoError(err)
+	this.Assert().Equal(this.httpSignURL_CertSubjectCN()+fakePath, exchange.RequestURI)
+	this.Assert().Equal("GET", exchange.RequestMethod)
+	this.Assert().Equal(http.Header{}, exchange.RequestHeaders)
+	this.Assert().Equal(200, exchange.ResponseStatus)
+	this.Assert().Equal(
+		[]string{"content-encoding", "content-length", "content-security-policy", "content-type", "date", "digest", "x-content-type-options"},
+		headerNames(exchange.ResponseHeaders))
+	this.Assert().Equal("text/html", exchange.ResponseHeaders.Get("Content-Type"))
+	this.Assert().Equal("text/html", exchange.ResponseHeaders.Get("Content-Type"))
+	this.Assert().Equal("nosniff", exchange.ResponseHeaders.Get("X-Content-Type-Options"))
+	this.Assert().Contains(exchange.SignatureHeaderValue, "validity-url=\""+this.httpSignURL_CertSubjectCN()+"/amppkg/validity\"")
+	this.Assert().Contains(exchange.SignatureHeaderValue, "integrity=\"digest/mi-sha256-03\"")
+	this.Assert().Contains(exchange.SignatureHeaderValue, "cert-url=\""+this.httpSignURL_CertSubjectCN()+"/amppkg/cert/"+pkgt.CertName+"\"")
+	this.Assert().Contains(exchange.SignatureHeaderValue, "cert-sha256=*"+pkgt.CertName+"=*")
+	var payloadPrefix bytes.Buffer
+	binary.Write(&payloadPrefix, binary.BigEndian, uint64(miRecordSize))
+	this.Assert().Equal(append(payloadPrefix.Bytes(), transformedBody...), exchange.Payload)
+}
+
 func (this *SignerSuite) TestParamsInPostBody() {
 	urlSets := []util.URLSet{{
 		Sign:  &util.URLPattern{[]string{"https"}, "", this.httpHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},

packager/testing/testing.go

@@ -66,14 +66,18 @@ func GetP(t *testing.T, handler AlmostHandler, target string, params httprouter.
 }
 
 func GetBH(t *testing.T, handler AlmostHandler, target string, body io.Reader, headers http.Header) *http.Response {
-	return GetBHP(t, handler, target, body, headers, httprouter.Params{})
+	return GetBHP(t, handler, target, "", body, headers, httprouter.Params{})
 }
 
 func GetHP(t *testing.T, handler AlmostHandler, target string, headers http.Header, params httprouter.Params) *http.Response {
-	return GetBHP(t, handler, target, nil, headers, params)
+	return GetBHP(t, handler, target, "", nil, headers, params)
 }
 
-func GetBHP(t *testing.T, handler AlmostHandler, target string, body io.Reader, headers http.Header, params httprouter.Params) *http.Response {
+func GetBHHP(t *testing.T, handler AlmostHandler, target string, host string, headers http.Header, params httprouter.Params) *http.Response {
+	return GetBHP(t, handler, target, host, nil, headers, params)
+}
+
+func GetBHP(t *testing.T, handler AlmostHandler, target string, host string, body io.Reader, headers http.Header, params httprouter.Params) *http.Response {
 	rec := httptest.NewRecorder()
 	method := ""
 	if body != nil {
@@ -86,6 +90,9 @@ func GetBHP(t *testing.T, handler AlmostHandler, target string, body io.Reader,
 			req.Header.Add(name, value)
 		}
 	}
+	if host != "" {
+		req.Host = host
+	}
 	handler.ServeHTTP(rec, req, params)
 	return rec.Result()
 }

packager/util/config.go

@@ -29,6 +29,7 @@ type Config struct {
 	CertFile  string // This must be the full certificate chain.
 	KeyFile   string // Just for the first cert, obviously.
 	OCSPCache string
+	ForwardedRequestHeaders []string
 	URLSet    []URLSet
 }
 
@@ -142,6 +143,15 @@ func ValidateFetchURLPattern(pattern *URLPattern) error {
 	return nil
 }
 
+func ValidateForwardedRequestHeaders(hs []string) error {
+	for _, h := range hs {
+		if msg := haveInvalidForwardedRequestHeader(h); msg != "" {
+			return errors.Errorf("ForwardedRequestHeaders must not %s", msg)
+		}
+	}
+	return nil
+}
+
 // ReadConfig reads the config file specified at --config and validates it.
 func ReadConfig(configBytes []byte) (*Config, error) {
 	tree, err := toml.LoadBytes(configBytes)
@@ -166,6 +176,11 @@ func ReadConfig(configBytes []byte) (*Config, error) {
 	if config.OCSPCache == "" {
 		return nil, errors.New("must specify OCSPCache")
 	}
+	if len(config.ForwardedRequestHeaders) > 0 {
+		if err := ValidateForwardedRequestHeaders(config.ForwardedRequestHeaders); err != nil {
+			return nil, err
+		}
+	}
 	ocspDir := filepath.Dir(config.OCSPCache)
 	if stat, err := os.Stat(ocspDir); os.IsNotExist(err) || !stat.Mode().IsDir() {
 		return nil, errors.Errorf("OCSPCache parent directory must exist: %s", ocspDir)

packager/util/config_test.go

@@ -51,6 +51,70 @@ func TestMinimalValidConfig(t *testing.T) {
 	}, *config)
 }
 
+func TestForwardedRequestHeader(t *testing.T) {
+	config, err := ReadConfig([]byte(`
+		CertFile = "cert.pem"
+		KeyFile = "key.pem"
+		OCSPCache = "/tmp/ocsp"
+		ForwardedRequestHeaders = ["X-Foo", "X-Bar"]
+		[[URLSet]]
+		  [URLSet.Sign]
+		    Domain = "example.com"
+	`))
+	require.NoError(t, err)
+	assert.Equal(t, Config{
+		Port:      8080,
+		CertFile:  "cert.pem",
+		KeyFile:   "key.pem",
+		OCSPCache: "/tmp/ocsp",
+		ForwardedRequestHeaders: []string{"X-Foo", "X-Bar"},
+		URLSet: []URLSet{{
+			Sign: &URLPattern{
+				Domain:  "example.com",
+				PathRE:  stringPtr(".*"),
+				QueryRE: stringPtr(""),
+				MaxLength: 2000,
+			},
+		}},
+	}, *config)
+}
+
+func TestForwardedRequestHeadersHaveHopByHopHeader(t *testing.T) {
+	assert.Contains(t, errorFrom(ReadConfig([]byte(`
+		CertFile = "cert.pem"
+		KeyFile = "key.pem"
+		OCSPCache = "/tmp/ocsp"
+		ForwardedRequestHeaders = ["X-Foo", "X-Bar", "connection"]
+		[[URLSet]]
+		  [URLSet.Sign]
+		    Domain = "example.com"
+	`))), "ForwardedRequestHeaders must not have hop-by-hop header of connection")
+}
+
+func TestForwardedRequestHeadersHaveConditionalRequestHeader(t *testing.T) {
+	assert.Contains(t, errorFrom(ReadConfig([]byte(`
+		CertFile = "cert.pem"
+		KeyFile = "key.pem"
+		OCSPCache = "/tmp/ocsp"
+		ForwardedRequestHeaders = ["X-Foo", "X-Bar", "if-match"]
+		[[URLSet]]
+		  [URLSet.Sign]
+		    Domain = "example.com"
+	`))), "ForwardedRequestHeaders must not have conditional request header of if-match")
+}
+
+func TestForwardedRequestHeadersHaveDisallowedHeader(t *testing.T) {
+	assert.Contains(t, errorFrom(ReadConfig([]byte(`
+		CertFile = "cert.pem"
+		KeyFile = "key.pem"
+		OCSPCache = "/tmp/ocsp"
+		ForwardedRequestHeaders = ["X-Foo", "X-Bar", "via"]
+		[[URLSet]]
+		  [URLSet.Sign]
+		    Domain = "example.com"
+	`))), "ForwardedRequestHeaders must not include request header of via")
+}
+
 func TestOCSPDirDoesntExist(t *testing.T) {
 	assert.Contains(t, errorFrom(ReadConfig([]byte(`
 		CertFile = "cert.pem"