Update SXG from b2 to b3. (#247)

In addition to the format change enabled by updating WICG/webpackage,
this required a few additional restrictions documented in
WICG/webpackage#350.

cmd/amppkg_test_cache/main.go

@@ -25,7 +25,6 @@ import (
 	"time"
 
 	"github.com/WICG/webpackage/go/signedexchange"
-	"github.com/WICG/webpackage/go/signedexchange/version"
 )
 
 var flagSXG = flag.String("sxg", "test.sxg", "Path to signed-exchange.")
@@ -60,7 +59,7 @@ func main() {
 		exchange.SignatureHeaderValue,
 		fmt.Sprintf(`"; cert-url="https://localhost:%d/test.cert"; cert-sha256=*`, *flagPort))
 	var sxg bytes.Buffer
-	exchange.Write(&sxg, version.Version1b2)
+	exchange.Write(&sxg)
 	sxgReader := bytes.NewReader(sxg.Bytes())
 	http.HandleFunc("/", func(resp http.ResponseWriter, req *http.Request) {
 		if req.URL.RequestURI() != "/" {

packager/accept/accept.go

@@ -9,14 +9,14 @@ import (
 
 // The SXG version that packager can produce. In the future, it may need to be
 // able to produce multiple versions.
-const AcceptedSxgVersion = "b2"
+const AcceptedSxgVersion = "b3"
 
 // The Content-Type for the SXG version that the signer produces.
 const SxgContentType = "application/signed-exchange;v=" + AcceptedSxgVersion
 
 // The enum of the SXG version that the signer produces, for passing to the
 // signedexchange library.
-var SxgVersion = version.Version1b2
+var SxgVersion = version.Version1b3
 
 // True if the given Accept header is one that the packager can satisfy. It
 // must contain application/signed-exchange;v=$V so that the packager knows

packager/accept/accept_test.go

@@ -9,17 +9,18 @@ import (
 func TestCanSatisfy(t *testing.T) {
 	assert.False(t, CanSatisfy(""))
 	assert.False(t, CanSatisfy("*/*"))
-	assert.False(t, CanSatisfy("image/jpeg;v=b2"))
-	// This is a bug, though one which won't occur in practice:
-	assert.False(t, CanSatisfy(`application/signed-exchange;x="a,b";v="b2"`))
+	assert.False(t, CanSatisfy("image/jpeg;v=b3"))
+	assert.False(t, CanSatisfy(`application/signed-exchange;v=b2`))
+	// This is a bug that will be triggered when a UA starts supporting multiple SXG versions:
+	assert.False(t, CanSatisfy(`application/signed-exchange;x="a,b";v="b3"`))
 
-	assert.True(t, CanSatisfy(`application/signed-exchange;v=b2`))
-	assert.True(t, CanSatisfy(`application/signed-exchange;v="b2"`))
-	assert.True(t, CanSatisfy(`application/signed-exchange;v=b2;q=0.8`))
-	assert.True(t, CanSatisfy(`application/signed-exchange;v=b1,application/signed-exchange;v=b2`))
-	assert.True(t, CanSatisfy(`application/signed-exchange;x="v=b1";v="b2"`))
-	assert.True(t, CanSatisfy("*/*, application/signed-exchange;v=b2"))
-	assert.True(t, CanSatisfy("*/* \t,\t application/signed-exchange;v=b2"))
-	// This is the same bug:
-	assert.True(t, CanSatisfy(`application/signed-exchange;x="y,application/signed-exchange;v=b2,z";v=b1`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;v=b3`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;v="b3"`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;v=b3;q=0.8`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;v=b1,application/signed-exchange;v=b3`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;x="v=b1";v="b3"`))
+	assert.True(t, CanSatisfy("*/*, application/signed-exchange;v=b3"))
+	assert.True(t, CanSatisfy("*/* \t,\t application/signed-exchange;v=b3"))
+	// This is the same bug, though one which won't occur in practice:
+	assert.True(t, CanSatisfy(`application/signed-exchange;x="y,application/signed-exchange;v=b3,z";v=b1`))
 }

packager/signer/signer.go

@@ -67,13 +67,16 @@ var conditionalRequestHeaders = map[string]bool{
 var statefulResponseHeaders = map[string]bool{
 	"Authentication-Control":    true,
 	"Authentication-Info":       true,
+	"Clear-Site-Data":           true,
 	"Optional-WWW-Authenticate": true,
 	"Proxy-Authenticate":        true,
 	"Proxy-Authentication-Info": true,
+	"Public-Key-Pins":           true,
 	"Sec-WebSocket-Accept":      true,
 	"Set-Cookie":                true,
 	"Set-Cookie2":               true,
 	"SetProfile":                true,
+	"Strict-Transport-Security": true,
 	"WWW-Authenticate":          true,
 }
 
@@ -124,7 +127,10 @@ var protocol = regexp.MustCompile("^[!#$%&'*+\\-.^_`|~0-9a-zA-Z/]+$")
 //
 // Connection header should also be removed per
 // https://tools.ietf.org/html/rfc7230#section-6.1.
-var legacyHeaders = []string{"Connection", "Keep-Alive", "Proxy-Authenticate", "Proxy-Authorization", "TE", "Trailer", "Transfer-Encoding", "Upgrade"}
+//
+// 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) {
@@ -392,6 +398,14 @@ func (this *Signer) ServeHTTP(resp http.ResponseWriter, req *http.Request, param
 
 		fetchResp.Header.Del("Link") // Ensure there are no privacy-violating Link:rel=preload headers.
 
+		if fetchResp.Header.Get("Variants") != "" || fetchResp.Header.Get("Variant-Key") != "" {
+			// Variants headers (https://tools.ietf.org/html/draft-ietf-httpbis-variants-04) are disallowed by AMP Cache.
+			// We could delete the headers, but it's safest to assume they reflect the downstream server's intent.
+			log.Println("Not packaging because response contains a Variants header.")
+			proxy(resp, fetchResp, nil)
+			return
+		}
+
 		this.serveSignedExchange(resp, fetchResp, signURL, transformVersion)
 
 	case 304:
@@ -465,12 +479,10 @@ func (this *Signer) serveSignedExchange(resp http.ResponseWriter, fetchResp *htt
 		fetchResp.Header.Set("Link", linkHeader)
 	}
 
-	exchange, err := signedexchange.NewExchange(signURL, http.Header{}, fetchResp.StatusCode, fetchResp.Header, []byte(transformed))
-	if err != nil {
-		util.NewHTTPError(http.StatusInternalServerError, "Error building exchange: ", err).LogAndRespond(resp)
-		return
-	}
-	if err := exchange.MiEncodePayload(miRecordSize, accept.SxgVersion); err != nil {
+	exchange := signedexchange.NewExchange(
+		accept.SxgVersion, /*uri=*/signURL.String(), /*method=*/"GET",
+		http.Header{}, fetchResp.StatusCode, fetchResp.Header, []byte(transformed))
+	if err := exchange.MiEncodePayload(miRecordSize); err != nil {
 		util.NewHTTPError(http.StatusInternalServerError, "Error MI-encoding: ", err).LogAndRespond(resp)
 		return
 	}
@@ -497,12 +509,12 @@ func (this *Signer) serveSignedExchange(resp http.ResponseWriter, fetchResp *htt
 		// default is to use getrandom(2) if available, else
 		// /dev/urandom.
 	}
-	if err := exchange.AddSignatureHeader(&signer, accept.SxgVersion); err != nil {
+	if err := exchange.AddSignatureHeader(&signer); err != nil {
 		util.NewHTTPError(http.StatusInternalServerError, "Error signing exchange: ", err).LogAndRespond(resp)
 		return
 	}
 	var body bytes.Buffer
-	if err := exchange.Write(&body, accept.SxgVersion); err != nil {
+	if err := exchange.Write(&body); err != nil {
 		util.NewHTTPError(http.StatusInternalServerError, "Error serializing exchange: ", err).LogAndRespond(resp)
 	}
 

packager/signer/signer_test.go

@@ -27,6 +27,7 @@ import (
 	"testing"
 
 	"github.com/WICG/webpackage/go/signedexchange"
+	"github.com/ampproject/amppackager/packager/accept"
 	"github.com/ampproject/amppackager/packager/rtv"
 	pkgt "github.com/ampproject/amppackager/packager/testing"
 	"github.com/ampproject/amppackager/packager/util"
@@ -74,12 +75,12 @@ func (this *SignerSuite) get(t *testing.T, handler pkgt.AlmostHandler, target st
 
 func (this *SignerSuite) getP(t *testing.T, handler pkgt.AlmostHandler, target string, params httprouter.Params) *http.Response {
 	return pkgt.GetHP(t, handler, target, http.Header{
-		"AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=b2"}}, params)
+		"AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}}, 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=b2"}})
+		"AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}})
 }
 
 func (this *SignerSuite) httpURL() string {
@@ -162,8 +163,9 @@ func (this *SignerSuite) TestSimple() {
 
 	exchange, err := signedexchange.ReadExchange(resp.Body)
 	this.Require().NoError(err)
-	this.Assert().Equal(this.httpSignURL()+fakePath, exchange.RequestURI.String())
-	this.Assert().Equal(http.Header{":method": []string{"GET"}}, exchange.RequestHeaders)
+	this.Assert().Equal(this.httpSignURL()+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"},
@@ -197,7 +199,7 @@ func (this *SignerSuite) TestParamsInPostBody() {
 
 	exchange, err := signedexchange.ReadExchange(resp.Body)
 	this.Require().NoError(err)
-	this.Assert().Equal(this.httpSignURL()+fakePath, exchange.RequestURI.String())
+	this.Assert().Equal(this.httpSignURL()+fakePath, exchange.RequestURI)
 }
 
 func (this *SignerSuite) TestEscapeQueryParamsInFetchAndSign() {
@@ -213,7 +215,7 @@ func (this *SignerSuite) TestEscapeQueryParamsInFetchAndSign() {
 
 	exchange, err := signedexchange.ReadExchange(resp.Body)
 	this.Require().NoError(err)
-	this.Assert().Equal(this.httpSignURL()+fakePath+"?%3Chi%3E", exchange.RequestURI.String())
+	this.Assert().Equal(this.httpSignURL()+fakePath+"?%3Chi%3E", exchange.RequestURI)
 }
 
 func (this *SignerSuite) TestNoFetchParam() {
@@ -225,7 +227,7 @@ func (this *SignerSuite) TestNoFetchParam() {
 	exchange, err := signedexchange.ReadExchange(resp.Body)
 	this.Require().NoError(err)
 	this.Assert().Equal(fakePath, this.lastRequest.URL.String())
-	this.Assert().Equal(this.httpsURL()+fakePath, exchange.RequestURI.String())
+	this.Assert().Equal(this.httpsURL()+fakePath, exchange.RequestURI)
 }
 
 func (this *SignerSuite) TestSignAsPathParam() {
@@ -238,7 +240,7 @@ func (this *SignerSuite) TestSignAsPathParam() {
 	exchange, err := signedexchange.ReadExchange(resp.Body)
 	this.Require().NoError(err)
 	this.Assert().Equal(fakePath, this.lastRequest.URL.String())
-	this.Assert().Equal(this.httpsURL()+fakePath, exchange.RequestURI.String())
+	this.Assert().Equal(this.httpsURL()+fakePath, exchange.RequestURI)
 }
 
 func (this *SignerSuite) TestPreservesContentType() {
@@ -453,7 +455,7 @@ func (this *SignerSuite) TestProxyUnsignedIfMissingAMPCacheTransformHeader() {
 		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
 	}}
 	resp := pkgt.GetH(this.T(), this.new(urlSets), "/priv/doc?sign="+url.QueryEscape(this.httpsURL()+fakePath), http.Header{
-		"Accept": {"application/signed-exchange;v=b2"}})
+		"Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}})
 	this.Assert().Equal(http.StatusOK, resp.StatusCode, "incorrect status: %#v", resp)
 	body, err := ioutil.ReadAll(resp.Body)
 	this.Require().NoError(err)
@@ -472,6 +474,38 @@ func (this *SignerSuite) TestProxyUnsignedIfMissingAcceptHeader() {
 	this.Assert().Equal(fakeBody, body, "incorrect body: %#v", resp)
 }
 
+func (this *SignerSuite) TestProxyUnsignedNonCachable() {
+	urlSets := []util.URLSet{{
+		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
+	}}
+	this.fakeHandler = func(resp http.ResponseWriter, req *http.Request) {
+		resp.Header().Set("Content-Type", "text/html")
+		resp.Header().Set("Cache-Control", "no-store")
+		resp.WriteHeader(200)
+	}
+
+	resp := this.get(this.T(), this.new(urlSets), "/priv/doc?sign="+url.QueryEscape(this.httpsURL()+fakePath))
+	this.Assert().Equal(200, resp.StatusCode)
+	this.Assert().Equal("no-store", resp.Header.Get("Cache-Control"))
+	this.Assert().Equal("text/html", resp.Header.Get("Content-Type"))
+}
+
+func (this *SignerSuite) TestProxyUnsignedBadContentEncoding() {
+	urlSets := []util.URLSet{{
+		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
+	}}
+	this.fakeHandler = func(resp http.ResponseWriter, req *http.Request) {
+		resp.Header().Set("Content-Type", "text/html")
+		resp.Header().Set("Content-Encoding", "br")
+		resp.WriteHeader(200)
+	}
+
+	resp := this.get(this.T(), this.new(urlSets), "/priv/doc?sign="+url.QueryEscape(this.httpsURL()+fakePath))
+	this.Assert().Equal(200, resp.StatusCode)
+	this.Assert().Equal("br", resp.Header.Get("Content-Encoding"))
+	this.Assert().Equal("text/html", resp.Header.Get("Content-Type"))
+}
+
 func (this *SignerSuite) TestProxyUnsignedErrOnStatefulHeader() {
 	urlSets := []util.URLSet{{
 		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), true, 2000, nil},
@@ -489,23 +523,24 @@ func (this *SignerSuite) TestProxyUnsignedErrOnStatefulHeader() {
 	this.Assert().Equal("text/html", resp.Header.Get("Content-Type"))
 }
 
-func (this *SignerSuite) TestProxyUnsignedNonCachable() {
+func (this *SignerSuite) TestProxyUnsignedOnVariants() {
 	urlSets := []util.URLSet{{
-		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil},
+		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), true, 2000, nil},
 	}}
 	this.fakeHandler = func(resp http.ResponseWriter, req *http.Request) {
 		resp.Header().Set("Content-Type", "text/html; charset=utf-8")
-		resp.Header().Set("Cache-Control", "no-store")
+		resp.Header().Set("Variants", "foo")
 		resp.Header().Set("Content-Type", "text/html")
 		resp.WriteHeader(200)
 	}
 
 	resp := this.get(this.T(), this.new(urlSets), "/priv/doc?sign="+url.QueryEscape(this.httpsURL()+fakePath))
 	this.Assert().Equal(200, resp.StatusCode)
-	this.Assert().Equal("no-store", resp.Header.Get("Cache-Control"))
+	this.Assert().Equal("foo", resp.Header.Get("Variants"))
 	this.Assert().Equal("text/html", resp.Header.Get("Content-Type"))
 }
 
+
 func (this *SignerSuite) TestProxyUnsignedIfNotAMP() {
 	urlSets := []util.URLSet{{
 		Sign: &util.URLPattern{[]string{"https"}, "", this.httpsHost(), stringPtr("/amp/.*"), []string{}, stringPtr(""), false, 2000, nil}}}

packager/signer/validation.go

@@ -200,6 +200,10 @@ func validateFetch(req *http.Request, resp *http.Response) error {
 	// Validate response is publicly-cacheable, per
 	// https://tools.ietf.org/html/draft-yasskin-http-origin-signed-responses-03#section-6.1, as referenced by
 	// https://tools.ietf.org/html/draft-yasskin-httpbis-origin-signed-exchanges-impl-00#section-6.
+	//
+	// Note: If the cachecontrol library ever adds support for no-cache
+	// with field name arguments, then instruct the signer to remove these
+	// headers, per https://github.com/WICG/webpackage/pull/339.
 	nonCachableReasons, _, err := cachecontrol.CachableResponse(req, resp, cachecontrol.Options{PrivateCache: false})
 	if err != nil {
 		return errors.Wrap(err, "Parsing cache headers")
@@ -208,6 +212,12 @@ func validateFetch(req *http.Request, resp *http.Response) error {
 		return errors.Errorf("Non-cacheable response: %s", nonCachableReasons)
 	}
 
+	// Validate that no Content-Encoding is specified. Otherwise, it was
+	// encoded as something that http.Client was unable to decode (e.g. br).
+	if encoding := resp.Header.Get("Content-Encoding"); encoding != "" {
+		return errors.Errorf("Invalid Content-Encoding: %s", encoding)
+	}
+
 	// Validate that Content-Type seems right. This doesn't validate its
 	// params (such as charset); we just want to verify we're not
 	// misinterpreting the server's intent. We override the Content-Type