Merge pull request #531 from ampproject/master

Release version 7 of AMP cache transforms

Author: banaag

docs/cache_requirements.md

@@ -34,18 +34,17 @@ These include:
    it cannot have a value (i.e. `no-cache=some-header` is disallowed).
  * The signed `content-security-policy` header must be present and comply with
    these rules:
-   * `default-src`, `script-src`, `object-src`, `style-src`, and `report-uri`
-     must equal those from the [AMP cache CSP](https://github.com/ampproject/amppackager/blob/c54d36556d56d5a604eea079ef7dc8067f67e1ea/packager/signer/signer.go#L244-L255)
+   * `default-src`, `script-src`, `object-src`, and `style-src` must equal
+     those from the [AMP cache CSP](https://github.com/ampproject/amppackager/blob/e581627de0f60e41cd073de8f097bd3e7259ffdf/packager/signer/signer.go#L246-L259).
    * `base-uri`, `block-all-mixed-content`, `font-src`, `form-action`,
-     `manifest-src`, `referrer`, and `upgrade-insecure-requests` may be omitted
-     or have any value
-   * all other directives are disallowed
+     `manifest-src`, `referrer`, `report-uri` and `upgrade-insecure-requests`
+     may be omitted or have any value.
+   * All other directives are disallowed.
  * The signed `content-type` header must be present. Its media type must be
    `text/html`. Its `charset` parameter, if present, must case-insensitively
    equal `utf-8`.
- * The signed `link` header, if present, must look like [this](https://github.com/ampproject/amppackager/blob/e4bf0430ba152cfe82ccf063df92021dfc0f26a5/packager/signer/signer.go#L426)
-   (the validation logic is currently very picky about its serialization); and
-   have limits like [this](https://github.com/ampproject/amppackager/blob/e4bf0430ba152cfe82ccf063df92021dfc0f26a5/transformer/transformer.go#L177)
+ * The signed `link` header, if present, must
+   have limits like [this](https://github.com/ampproject/amppackager/blob/a5bb0248b0fd0bf2ad6e4d47ca444261c6409e3f/transformer/transformer.go#L201)
    (e.g. max 20 urls, rel=preload only, as=script|style only). URLs must be
    limited to `cdn.ampproject.org` and the allowlisted [font provider URLs](https://github.com/ampproject/amphtml/blob/b0ff92429923c86f3973009a84ff02f4f1868b4d/validator/validator-main.protoascii#L310).
  * There must not be a signed `variant-key-04` or `variants-04` header.

go.mod

@@ -15,8 +15,9 @@ require (
 	github.com/pquerna/cachecontrol v0.0.0-20180306154005-525d0eb5f91d
 	github.com/prometheus/client_golang v1.1.0
 	github.com/stretchr/testify v1.4.0
+	github.com/twifkak/crypto v0.0.0-20210326012946-1fce8924335d
 	golang.org/x/crypto v0.0.0-20190701094942-4def268fd1a4
-	golang.org/x/net v0.0.0-20190930134127-c5a3c61f89f3
+	golang.org/x/net v0.0.0-20210226172049-e18ecbb05110
 	google.golang.org/grpc v1.27.0
 	google.golang.org/protobuf v1.25.0
 	gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect

go.sum

@@ -291,6 +291,9 @@ github.com/timewasted/linode v0.0.0-20160829202747-37e84520dcf7 h1:CpHxIaZzVy26G
 github.com/timewasted/linode v0.0.0-20160829202747-37e84520dcf7/go.mod h1:imsgLplxEC/etjIhdr3dNzV3JeT27LbVu5pYWm0JCBY=
 github.com/transip/gotransip v0.0.0-20190812104329-6d8d9179b66f h1:clyOmELPZd2LuFEyuo1mP6RXpbAW75PwD+RfDj4kBm0=
 github.com/transip/gotransip v0.0.0-20190812104329-6d8d9179b66f/go.mod h1:i0f4R4o2HM0m3DZYQWsj6/MEowD57VzoH0v3d7igeFY=
+github.com/twifkak/crypto v0.0.0-20210326012350-5f9a47aaade5 h1:Oota0BCNCpjP5Q7jeGhL9YUntFZ3DsIS9gYGZicdG5w=
+github.com/twifkak/crypto v0.0.0-20210326012946-1fce8924335d h1:BBr+XPoeoGMHJIe/QTrEStIeqtPR6vd6VQkPxZpmvtg=
+github.com/twifkak/crypto v0.0.0-20210326012946-1fce8924335d/go.mod h1:rNU8gVCuI2stMMtBCigkYDjse6rFX/5ygOfUGQpGsbY=
 github.com/uber-go/atomic v1.3.2 h1:Azu9lPBWRNKzYXSIwRfgRuDuS0YKsK4NFhiQv98gkxo=
 github.com/uber-go/atomic v1.3.2/go.mod h1:/Ct5t2lcmbJ4OSe/waGBoaVvVqtO0bmtfVNex1PFV8g=
 github.com/ugorji/go/codec v0.0.0-20181209151446-772ced7fd4c2 h1:EICbibRW4JNKMcY+LsWmuwob+CRS1BmdRdjphAm9mH4=
@@ -343,6 +346,9 @@ golang.org/x/sys v0.0.0-20190507160741-ecd444e8653b h1:ag/x1USPSsqHud38I9BAC88qd
 golang.org/x/sys v0.0.0-20190507160741-ecd444e8653b/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20190801041406-cbf593c0f2f3 h1:4y9KwBHBgBNwDbtu44R5o1fdOCQUEXhbk/P4A9WmJq0=
 golang.org/x/sys v0.0.0-20190801041406-cbf593c0f2f3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20201119102817-f84b799fce68 h1:nxC68pudNYkKU6jWhgrqdreuFiOQWj1Fs7T3VrH4Pjw=
+golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=

packager/certcache/certcache.go

@@ -429,9 +429,9 @@ func (this *CertCache) readOCSP(allowRetries bool) ([]byte, time.Time, error) {
 
 // Print # of retries, wait for specified time and returned updated wait time.
 func waitForSpecifiedTime(waitTimeInMinutes int, numRetries int) int {
-	log.Printf("Retrying OCSP server: retry #%d\n", numRetries)
+	log.Printf("Retrying OCSP server: retry #%d", numRetries)
 	// Wait using exponential backoff.
-	log.Printf("Waiting for %d minute(s)\n", waitTimeInMinutes)
+	log.Printf("Waiting for %d minute(s)", waitTimeInMinutes)
 	waitTimeDuration := time.Duration(waitTimeInMinutes) * time.Minute
 	// For exponential backoff.
 	newWaitTimeInMinutes := 2 * waitTimeInMinutes
@@ -650,11 +650,28 @@ func (this *CertCache) fetchOCSP(orig []byte, certs []*x509.Certificate, ocspUpd
 		log.Println("OCSP nextUpdate in the past:", resp.NextUpdate)
 		return orig
 	}
+	for _, test := range []struct {
+		name  string
+		value time.Time
+	}{
+		{"thisUpdate", resp.ThisUpdate},
+		{"nextUpdate", resp.NextUpdate},
+		{"producedAt", resp.ProducedAt},
+	} {
+		if test.value.Before(certs[0].NotBefore) {
+			log.Printf("OCSP %s %+v before certificate notBefore %+v", test.name, test.value, certs[0].NotBefore)
+			return orig
+		}
+		if test.value.After(certs[0].NotAfter) {
+			log.Printf("OCSP %s %+v after certificate notAfter %+v", test.name, test.value, certs[0].NotAfter)
+			return orig
+		}
+	}
 	// OCSP duration must be <=7 days, per
 	// https://wicg.github.io/webpackage/draft-yasskin-httpbis-origin-signed-exchanges-impl.html#cross-origin-trust.
 	// Serving these responses may cause UAs to reject the SXG.
 	if resp.NextUpdate.Sub(resp.ThisUpdate) > time.Hour*24*7 {
-		log.Printf("OCSP nextUpdate %+v too far ahead of thisUpdate %+v\n", resp.NextUpdate, resp.ThisUpdate)
+		log.Printf("OCSP nextUpdate %+v too far ahead of thisUpdate %+v", resp.NextUpdate, resp.ThisUpdate)
 		return orig
 	}
 	return respBytes

packager/certcache/certcache_test.go

@@ -33,6 +33,7 @@ import (
 	pkgt "github.com/ampproject/amppackager/packager/testing"
 	"github.com/ampproject/amppackager/packager/util"
 	"github.com/stretchr/testify/suite"
+	ocsptest "github.com/twifkak/crypto/ocsp"
 	"golang.org/x/crypto/ocsp"
 )
 
@@ -48,16 +49,20 @@ var caKey = func() *rsa.PrivateKey {
 	return key.(*rsa.PrivateKey)
 }()
 
-func FakeOCSPResponse(thisUpdate time.Time) ([]byte, error) {
-	template := ocsp.Response{
+// FakeOCSPResponse returns a DER-encoded fake OCSP response. producedAt is
+// rounded up to the nearest minute, rather than the default ocsp lib behavior
+// of rounding down, so that calls to this function with producedAt ==
+// thisUpdate return a valid response.
+func FakeOCSPResponse(thisUpdate, producedAt time.Time) ([]byte, error) {
+	template := ocsptest.Response{
 		Status:           ocsp.Good,
 		SerialNumber:     pkgt.B3Certs[0].SerialNumber,
 		ThisUpdate:       thisUpdate,
 		NextUpdate:       thisUpdate.Add(7 * 24 * time.Hour),
 		RevokedAt:        thisUpdate.AddDate( /*years=*/ 0 /*months=*/, 0 /*days=*/, 365),
 		RevocationReason: ocsp.Unspecified,
 	}
-	return ocsp.CreateResponse(caCert, caCert, template, caKey)
+	return ocsptest.CreateResponse(caCert, caCert, template, caKey, producedAt.Add(1*time.Minute))
 }
 
 type CertCacheSuite struct {
@@ -110,9 +115,14 @@ func (this *CertCacheSuite) TearDownSuite() {
 }
 
 func (this *CertCacheSuite) SetupTest() {
+	// Set fake clock to a time 8 days past the NotBefore of the cert, so
+	// our tests can backdate OCSPs to test expiry logic, without
+	// accidentally hitting the requirement that OCSPs must postdate certs.
 	this.fakeClock = pkgt.NewFakeClock()
+	this.fakeClock.SecondsSince0 = pkgt.B3Certs[0].NotBefore.Add(8 * 24 * time.Hour).Sub(time.Unix(0, 0))
+	now := this.fakeClock.Now()
 	var err error
-	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now())
+	this.fakeOCSP, err = FakeOCSPResponse(now, now)
 	this.Require().NoError(err, "creating fake OCSP response")
 
 	this.ocspHandler = func(resp http.ResponseWriter, req *http.Request) {
@@ -193,11 +203,30 @@ func (this *CertCacheSuite) TestCertCacheIsHealthy() {
 	this.Assert().NoError(this.handler.IsHealthy())
 }
 
+func (this *CertCacheSuite) TestOCSPInvalidThisUpdate() {
+	// Set fake clock equal to cert NotBefore, so we can produce an OCSP
+	// where "now" is within its ThisUpdate/NextUpdate window, but the OCSP
+	// is itself outside of the cert's NotBefore/NotAfter window.
+	this.fakeClock.SecondsSince0 = pkgt.B3Certs[0].NotBefore.Sub(time.Unix(0, 0))
+
+	err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
+	this.Require().NoError(err, "deleting OCSP tempfile")
+
+	// Build an OCSP response that's not expired, but invalid because it predates the cert:
+	invalidOCSP, err := FakeOCSPResponse(this.fakeClock.Now().Add(-1*24*time.Hour), this.fakeClock.Now())
+	this.Require().NoError(err, "creating invalid OCSP response")
+	this.fakeOCSP = invalidOCSP
+	this.Require().True(this.ocspServerCalled(func() {
+		this.handler, err = this.New()
+		this.Require().EqualError(err, "initializing CertCache: Missing OCSP response.")
+	}))
+}
+
 func (this *CertCacheSuite) TestCertCacheIsNotHealthy() {
 	// Prime memory cache with a past-midpoint OCSP:
 	err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
 	this.Require().NoError(err, "deleting OCSP tempfile")
-	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-4 * 24 * time.Hour))
+	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-4*24*time.Hour), this.fakeClock.Now())
 	this.Require().NoError(err, "creating stale OCSP response")
 	this.Require().True(this.ocspServerCalled(func() {
 		this.handler, err = this.New()
@@ -230,7 +259,7 @@ func (this *CertCacheSuite) TestOCSP() {
 	resp := pkgt.NewRequest(this.T(), this.mux(), "/amppkg/cert/"+pkgt.CertName).Do()
 	this.Assert().Equal(http.StatusOK, resp.StatusCode, "incorrect status: %#v", resp)
 	// 302400 is 3.5 days. max-age is slightly less because of the time between fake OCSP generation and cert-chain response.
-	this.Assert().Equal("public, max-age=302387", resp.Header.Get("Cache-Control"))
+	this.Assert().Equal("public, max-age=302388", resp.Header.Get("Cache-Control"))
 	cbor := this.DecodeCBOR(resp.Body)
 	this.Assert().Equal(this.fakeOCSP, cbor["ocsp"])
 }
@@ -253,7 +282,7 @@ func (this *CertCacheSuite) TestOCSPExpiry() {
 	// Prime memory and disk cache with a past-midpoint OCSP:
 	err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
 	this.Require().NoError(err, "deleting OCSP tempfile")
-	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-4 * 24 * time.Hour))
+	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-4*24*time.Hour), this.fakeClock.Now())
 	this.Require().NoError(err, "creating expired OCSP response")
 	this.Require().True(this.ocspServerCalled(func() {
 		this.handler, err = this.New()
@@ -275,15 +304,16 @@ func (this *CertCacheSuite) TestOCSPUpdateFromDisk() {
 	// Prime memory cache with a past-midpoint OCSP:
 	err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
 	this.Require().NoError(err, "deleting OCSP tempfile")
-	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-4 * 24 * time.Hour))
+	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-4*24*time.Hour), this.fakeClock.Now())
 	this.Require().NoError(err, "creating stale OCSP response")
 	this.Require().True(this.ocspServerCalled(func() {
 		this.handler, err = this.New()
 		this.Require().NoError(err, "reinstantiating CertCache")
 	}))
 
 	// Prime disk cache with a fresh OCSP.
-	freshOCSP, err := FakeOCSPResponse(this.fakeClock.Now())
+	now := this.fakeClock.Now()
+	freshOCSP, err := FakeOCSPResponse(now, now)
 	this.Require().NoError(err, "creating fresh OCSP response")
 	err = ioutil.WriteFile(filepath.Join(this.tempDir, "ocsp"), freshOCSP, 0644)
 	this.Require().NoError(err, "writing fresh OCSP response to disk")
@@ -314,11 +344,11 @@ func (this *CertCacheSuite) TestOCSPExpiredViaHTTPHeaders() {
 	}))
 }
 
-func (this *CertCacheSuite) TestOCSPIgnoreInvalidUpdate() {
+func (this *CertCacheSuite) TestOCSPIgnoreExpiredNextUpdate() {
 	// Prime memory and disk cache with a past-midpoint OCSP:
 	err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
 	this.Require().NoError(err, "deleting OCSP tempfile")
-	staleOCSP, err := FakeOCSPResponse(this.fakeClock.Now().Add(-4 * 24 * time.Hour))
+	staleOCSP, err := FakeOCSPResponse(this.fakeClock.Now().Add(-4*24*time.Hour), this.fakeClock.Now())
 	this.Require().NoError(err, "creating stale OCSP response")
 	this.fakeOCSP = staleOCSP
 	this.Require().True(this.ocspServerCalled(func() {
@@ -327,7 +357,7 @@ func (this *CertCacheSuite) TestOCSPIgnoreInvalidUpdate() {
 	}))
 
 	// Try to update with an invalid OCSP:
-	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-8 * 24 * time.Hour))
+	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-8*24*time.Hour), this.fakeClock.Now())
 	this.Require().NoError(err, "creating expired OCSP response")
 	this.Assert().True(this.ocspServerCalled(func() {
 		_, _, err := this.handler.readOCSP(true)
@@ -340,6 +370,32 @@ func (this *CertCacheSuite) TestOCSPIgnoreInvalidUpdate() {
 	this.Assert().Equal(staleOCSP, ocsp)
 }
 
+func (this *CertCacheSuite) TestOCSPIgnoreInvalidProducedAt() {
+	// Prime memory and disk cache with a past-midpoint OCSP:
+	err := os.Remove(filepath.Join(this.tempDir, "ocsp"))
+	this.Require().NoError(err, "deleting OCSP tempfile")
+	staleOCSP, err := FakeOCSPResponse(this.fakeClock.Now().Add(-4*24*time.Hour), this.fakeClock.Now())
+	this.Require().NoError(err, "creating stale OCSP response")
+	this.fakeOCSP = staleOCSP
+	this.Require().True(this.ocspServerCalled(func() {
+		this.handler, err = this.New()
+		this.Require().NoError(err, "reinstantiating CertCache")
+	}))
+
+	// Try to update with an OCSP with an invalid ProducedAt:
+	this.fakeOCSP, err = FakeOCSPResponse(this.fakeClock.Now().Add(-4*24*time.Hour), time.Unix(0, 0))
+	this.Require().NoError(err, "creating invalid OCSP response")
+	this.Assert().True(this.ocspServerCalled(func() {
+		_, _, err := this.handler.readOCSP(true)
+		this.Require().NoError(err, "updating OCSP")
+	}))
+
+	// Verify that the invalid update doesn't squash the valid cache entry.
+	ocsp, _, err := this.handler.readOCSP(true)
+	this.Require().NoError(err, "reading OCSP")
+	this.Assert().Equal(staleOCSP, ocsp)
+}
+
 func (this *CertCacheSuite) TestPopulateCertCache() {
 	certCache, err := PopulateCertCache(
 		&util.Config{

packager/signer/signer.go

@@ -247,6 +247,7 @@ func MutateFetchedContentSecurityPolicy(fetched string) string {
 			"report-uri https://csp.withgoogle.com/csp/amp;" +
 			"script-src blob: https://cdn.ampproject.org/rtv/ " +
 			"https://cdn.ampproject.org/v0.js " +
+			"https://cdn.ampproject.org/v0.mjs " +
 			"https://cdn.ampproject.org/v0/ " +
 			"https://cdn.ampproject.org/lts/ " +
 			"https://cdn.ampproject.org/viewer/;" +
@@ -454,7 +455,13 @@ func formatLinkHeader(preloads []*rpb.Metadata_Preload) (string, error) {
 		var value strings.Builder
 		value.WriteByte('<')
 		value.WriteString(u.String())
-		value.WriteString(">;rel=preload;as=")
+		value.WriteString(">;rel=")
+		if preload.Module {
+			value.WriteString("modulepreload")
+		} else {
+			value.WriteString("preload")
+		}
+		value.WriteString(";as=")
 		value.WriteString(preload.As)
 		for _, attr := range preload.GetAttributes() {
 			value.WriteByte(';')

packager/signer/signer_test.go

@@ -468,7 +468,7 @@ func (this *SignerSuite) TestMutatesCspHeaders() {
 			"block-all-mixed-content;"+
 			"default-src * blob: data:;"+
 			"report-uri https://csp.withgoogle.com/csp/amp;"+
-			"script-src blob: https://cdn.ampproject.org/rtv/ https://cdn.ampproject.org/v0.js https://cdn.ampproject.org/v0/ https://cdn.ampproject.org/lts/ https://cdn.ampproject.org/viewer/;"+
+			"script-src blob: https://cdn.ampproject.org/rtv/ https://cdn.ampproject.org/v0.js https://cdn.ampproject.org/v0.mjs https://cdn.ampproject.org/v0/ https://cdn.ampproject.org/lts/ https://cdn.ampproject.org/viewer/;"+
 			"style-src 'unsafe-inline' https://cdn.materialdesignicons.com https://cloud.typography.com https://fast.fonts.net https://fonts.googleapis.com https://maxcdn.bootstrapcdn.com https://p.typekit.net https://pro.fontawesome.com https://use.fontawesome.com https://use.typekit.net;"+
 			"object-src 'none'",
 		exchange.ResponseHeaders.Get("Content-Security-Policy"))

packager/testing/testing.go

@@ -146,7 +146,7 @@ func (r *Request) Do() *http.Response {
 }
 
 type FakeClock struct {
-	secondsSince0 time.Duration
+	SecondsSince0 time.Duration
 	Delta         time.Duration
 }
 
@@ -155,7 +155,7 @@ func NewFakeClock() *FakeClock {
 }
 
 func (this *FakeClock) Now() time.Time {
-	secondsSince0 := this.secondsSince0
-	this.secondsSince0 = secondsSince0 + this.Delta
+	secondsSince0 := this.SecondsSince0
+	this.SecondsSince0 = secondsSince0 + this.Delta
 	return time.Unix(0, 0).Add(secondsSince0)
 }

packager/util/util.go

@@ -36,6 +36,10 @@ const SignerURLPrefix = "/priv/doc"
 // packager's cert cache. Should be stable and unique (e.g.
 // content-addressing). Clients should url.PathEscape this, just in case its
 // format changes to need escaping in the future.
+//
+// Given a PEM-encoded certificate, this is equivalent to:
+// $ openssl x509 -in cert.pem -outform DER |
+//     openssl dgst -sha256 -binary | base64 | tr /+ _- | tr -d =
 func CertName(cert *x509.Certificate) string {
 	sum := sha256.Sum256(cert.Raw)
 	return base64.RawURLEncoding.EncodeToString(sum[:])