Merge pull request #511 from ampproject/master

Snapshot release version 6.

.github/workflows/prerequisites.yml

@@ -0,0 +1,54 @@
+name: Check prerequisites
+on: [push, pull_request]
+jobs:
+  check-format:
+    runs-on: ubuntu-latest
+    steps:
+    - name: Setup Go 1.16
+      uses: actions/setup-go@v1
+      with:
+        go-version: '1.16'
+
+    - name: Install goimports
+      run: go get golang.org/x/tools/cmd/goimports
+
+    - name: Checkout the repository
+      uses: actions/checkout@v2
+
+# Comment this out for now until amppackager passes goimports.
+# https://github.com/ampproject/amppackager/issues/506
+#    - name: Check formatting with goimports
+#      run: |
+#      if $(go env GOPATH)/bin/goimports -d . | grep . ; then
+#          exit 1
+#      fi
+
+  go-vet:
+    runs-on: ubuntu-latest
+    steps:
+    - name: Setup Go 1.16
+      uses: actions/setup-go@v1
+      with:
+        go-version: '1.16'
+
+    - name: Checkout the repository
+      uses: actions/checkout@v2
+
+    - name: Diagnose the code with go vet
+      # TODO(banaag): Turn on composite checking when
+      # https://github.com/ampproject/amppackager/issues/507 is fixed.
+      run: go vet -composites=false ./...
+
+  go-test:
+    runs-on: ubuntu-latest
+    steps:
+    - name: Setup Go 1.16
+      uses: actions/setup-go@v1
+      with:
+        go-version: '1.16'
+
+    - name: Checkout the repository
+      uses: actions/checkout@v2
+
+    - name: Run the tests
+      run: go test ./...

README.md

@@ -37,13 +37,21 @@ own and can obtain certificates for.
   1. Get amppackager.
 
      Check your Go version by running `go version`.
-
-     For Go 1.14 and higher versions run:
+
+     For Go 1.16 and higher run:
+
+       ```
+       git clone https://github.com/ampproject/amppackager.git my-amp-directory
+       cd my-amp-directory
+       go install github.com/ampproject/amppackager/cmd/amppkg
+       ```
+
+     For Go 1.14 and Go 1.15 run:
 
        ```
        go get -u github.com/ampproject/amppackager/cmd/amppkg
        ```
-     
+
      For Go 1.13 and earlier versions run:
 
        ```

amppkg.example.toml

@@ -191,10 +191,6 @@ ForwardedRequestHeaders = []
   # PathRE = "/world/.*"
   # QueryRE = ""
 
-# IMPORTANT NOTE: the support of the ACME protocol and automatic renewal of certificates is currently in the
-# EXPERIMENTAL stage.  Once we have more experience with people using it out in the wild, we will gradually
-# move it to PRODUCTION mode.
-#
 # ACME is a protocol that allows for automatic renewal of certificates. AMP Packager uses an ACME library
 # https://github.com/go-acme/lego to handle certificate renewal. Automatic certificate renewal is enabled
 # in AMP Packager via the 'autorenewcert' flag. Turning the flag on will enable AMP Packager to automatically
@@ -227,6 +223,15 @@ ForwardedRequestHeaders = []
     # request signed exchange certificates.
     # EmailAddress = "user@company.com"
 
+    # The EABKid and EABHmac need to have synchronized values.  They can both be empty (in which case EAB is not used)
+    # or both have valid values. If one is empty and the other is not, the AMP Packager will generate an error.
+    # This is the Key Identifier from ACME CA. Used for External Account Binding.
+    # EABKid = "eab.kid"
+
+    # This is the MAC Key from ACME CA. Used for External Account Binding. Should be in
+    # Base64 URL Encoding without padding format.
+    # EABHmac = "eab.hmac"
+
     # For the remaining configuration items, it's important to understand the different challenges employed as
     # part of the ACME protocol.  See:
     #   https://ietf-wg-acme.github.io/acme/draft-ietf-acme-acme.html#identifier-validation-challenges
@@ -265,6 +270,8 @@ ForwardedRequestHeaders = []
   # [ACMEConfig.Development]
     # DiscoURL = "https://development-acme.discovery.url/"
     # EmailAddress = "user@company.com"
+    # EABKid = "eab.kid"
+    # EABHmac = "eab.hmac"
     # HttpChallengePort = 5002
     # HttpWebRootDir = '/path/to/www_root_dir'
     # TlsChallengePort = 5003

deploy/gcloud/README.md

@@ -118,6 +118,61 @@ The following information can be customized in setup.sh, but the default also wo
 
   kubectl apply -f amppackager_service.yaml
 
+## Production Reverse Proxy Setup
+
+For now, productionizing is a bit manual. The minimum steps are:
+
+  1. Don't expose amppkg to the outside world. If possible, keep it on your internal network.
+     In addition, use the loadBalancerSourceRanges above so that it only responds to the reverse
+     proxy you set up. More information could be found here:
+     1. [Kubernetes Firewall Configuration](https://kubernetes.io/docs/tasks/access-application-cluster/configure-cloud-provider-firewall/)
+     2. [Subnet Calculator](https://mxtoolbox.com/subnetcalculator.aspx)
+     3. [Understanding CIDR notation](https://www.digitalocean.com/community/tutorials/understanding-ip-addresses-subnets-and-cidr-notation-for-networking)
+  2. Configure your TLS-serving frontend server to conditionally proxy to
+     `amppkg`:
+     1. If the URL starts with `/amppkg/`, forward the request unmodified.
+     2. If the URL points to an AMP page and the `AMP-Cache-Transform` request
+        header is present, rewrite the URL by prepending `/priv/doc` and forward
+        the request.
+
+        NOTE: If using nginx, prefer using `proxy_pass` with `$request_uri`,
+        rather than using `rewrite`, as in [this PR](https://github.com/Warashi/try-amppackager/pull/3),
+        to avoid percent-encoding issues.
+     3. If at all possible, don't send URLs of non-AMP pages to `amppkg`; its
+        [transforms](transformer/) may break non-AMP HTML.
+     4. DO NOT forward `/priv/doc` requests; these URLs are meant to be
+        generated by the frontend server only.
+  3. For HTTP compliance, ensure the `Vary` header set to `AMP-Cache-Transform,
+     Accept` for all URLs that point to an AMP page, irrespective of whether the
+     response is HTML or SXG. (SXG responses that come from `amppkg` will have
+     the appropriate `Vary` header set, so it may only be necessary to
+     explicitly set the `Vary` header for HTML responses.)
+  4. Get an SXG cert from your CA. It must use an EC key with the prime256v1
+     algorithm, and it must have a [CanSignHttpExchanges
+     extension](https://wicg.github.io/webpackage/draft-yasskin-httpbis-origin-signed-exchanges-impl.html#cross-origin-cert-req).
+     One provider of SXG certs is [DigiCert](https://www.digicert.com/account/ietf/http-signed-exchange.php).
+     You can fill in your ACME Directory URL using these [steps](https://docs.digicert.com/certificate-tools/Certificate-lifecycle-automation-index/acme-user-guide/acme-directory-urls-signed-http-exchange-certificates/).
+  5. Every 90 days or sooner, renew your SXG cert (per
+     [WICG/webpackage#383](https://github.com/WICG/webpackage/pull/383)) and
+     restart amppkg (per
+     [#93](https://github.com/ampproject/amppackager/issues/93)). This is only necessary if ACME
+     cert auto-renewal is not enabled.
+  6. Keep amppkg updated from either `releases` (the default branch, so `go get` works)
+     about every ~2 months. The [wg-caching](https://github.com/ampproject/wg-caching)
+     team will release a new version approximately this often. Soon after each
+     release, Googlebot will increment the version it requests with
+     `AMP-Cache-Transform`. Googlebot will only allow the latest 2-3 versions
+     (details are still TBD), so an update is necessary but not immediately. If
+     amppkg doesn't support the requested version range, it will fall back to
+     serving unsigned AMP.
+
+     The version of amppkg on the [Google Cloud Marketplace](https://cloud.google.com/marketplace/)
+     will also be updated once a new release is published.
+
+     To keep subscribed to releases, you can select "Releases only" from the
+     "Watch" dropdown in GitHub, or use [various tools](https://stackoverflow.com/questions/9845655/how-do-i-get-notifications-for-commits-to-a-repository)
+     to subscribe to the `releases` branch.
+
 ## Optionally, run the teardown script (gcloud_down.sh)
 
   1. Go to the directory where you installed amppackager.

docs/cache_requirements.md

@@ -15,6 +15,7 @@ The Google AMP cache sets some requirements in addition to the ones set by the
 These include:
 
  * The signed `fallback URL` must equal the URL at which the SXG was delivered.
+ * The signed `cert-url` must be `https`.
  * The signature header must contain only:
    * One parameterised identifier.
    * Parameter values of type string, binary, or identifier.
@@ -34,7 +35,7 @@ These include:
  * 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/releases/packager/signer/signer.go#L272)
+     must equal those from the [AMP cache CSP](https://github.com/ampproject/amppackager/blob/c54d36556d56d5a604eea079ef7dc8067f67e1ea/packager/signer/signer.go#L244-L255)
    * `base-uri`, `block-all-mixed-content`, `font-src`, `form-action`,
      `manifest-src`, `referrer`, and `upgrade-insecure-requests` may be omitted
      or have any value

go.sum

@@ -251,15 +251,12 @@ github.com/prometheus/client_golang v0.9.3-0.20190127221311-3c4408c8b829/go.mod
 github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5FsnadC4Ky3P0J6CfImo=
 github.com/prometheus/client_golang v1.1.0 h1:BQ53HtBmfOitExawJ6LokA4x8ov/z0SYYb0+HxJfRI8=
 github.com/prometheus/client_golang v1.1.0/go.mod h1:I1FGZT9+L76gKKOs5djB6ezCbFQP1xR9D75/vuwEF3g=
-github.com/prometheus/client_golang v1.6.0 h1:YVPodQOcK15POxhgARIvnDRVpLcuK8mglnMrWfyrw6A=
-github.com/prometheus/client_golang v1.7.1 h1:NTGy1Ja9pByO+xAeH/qiWnLrKtr3hJPNjaVUwnjpdpA=
 github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
 github.com/prometheus/client_model v0.0.0-20190115171406-56726106282f/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
 github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90 h1:S/YWwWx/RA8rT8tKFRuGUZhuA90OyIBpPCXkcbwU8DE=
 github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
 github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4 h1:gQz4mCbXsO+nc9n1hCxHcGA3Zx3Eo+UHZoInFGUIXNM=
 github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
-github.com/prometheus/client_model v0.2.0 h1:uq5h0d+GuxiXLJLNABMgp2qUWDPiLvgCzz2dUR+/W/M=
 github.com/prometheus/common v0.2.0/go.mod h1:TNfzLD0ON7rHzMJeJkieUDPYmFC7Snx/y86RQel1bk4=
 github.com/prometheus/common v0.4.1/go.mod h1:TNfzLD0ON7rHzMJeJkieUDPYmFC7Snx/y86RQel1bk4=
 github.com/prometheus/common v0.6.0 h1:kRhiuYSXR3+uv2IbVbZhUxK5zVD/2pp3Gd2PpvPkpEo=

packager/certcache/certcache.go

@@ -717,6 +717,7 @@ func (this *CertCache) setCerts(certs []*x509.Certificate) {
 	this.certs = certs
 	this.certName = util.CertName(certs[0])
 
+	log.Printf("Writing cert %s to file %v", this.certName, this.CertFile)
 	err := certloader.WriteCertsToFile(this.certs, this.CertFile)
 	if err != nil {
 		log.Printf("Unable to write certs to file: %s", this.CertFile)

packager/certfetcher/certfetcher.go

@@ -60,8 +60,8 @@ func (u *AcmeUser) GetPrivateKey() crypto.PrivateKey {
 // fetcher := CertFetcher()
 // fetcher.setUser(email, privateKey)
 // fetcher.bindToPort(port)
-func New(email string, certSignRequest *x509.CertificateRequest, privateKey crypto.PrivateKey,
-	acmeDiscoURL string, httpChallengePort int, httpChallengeWebRoot string,
+func New(email string, eabKid string, eabHmac string, certSignRequest *x509.CertificateRequest,
+	privateKey crypto.PrivateKey, acmeDiscoURL string, httpChallengePort int, httpChallengeWebRoot string,
 	tlsChallengePort int, dnsProvider string, shouldRegister bool) (*CertFetcher, error) {
 
 	acmeUser := AcmeUser{
@@ -119,17 +119,32 @@ func New(email string, certSignRequest *x509.CertificateRequest, privateKey cryp
 		}
 	}
 
-	// Theoretically, this should always be set to false as users should have pre-registered for access
-	// to the ACME CA and agreed to the TOS.
-	// TODO(banaag): revisit this when trying the class out with Digicert CA.
+	var reg *registration.Resource
 	if !shouldRegister {
 		acmeUser.Registration = new(registration.Resource)
+	} else if reg, err = client.Registration.ResolveAccountByKey(); err == nil {
+		// Check if we already have an account.
+		acmeUser.Registration = reg
 	} else {
+		// We need to reset the LEGO client after calling Registration.ResolveAccountByKey().
+		client, err = lego.NewClient(config)
+		if err != nil {
+			return nil, errors.Wrap(err, "Obtaining LEGO client.")
+		}
 		// TODO(banaag) make sure we present the TOS URL to the user and prompt for confirmation.
 		// The plan is to move this to some separate setup command outside the server which would be
 		// executed one time. Alternatively, we can have a field in the toml file that is documented
 		// to indicate agreement with TOS.
-		reg, err := client.Registration.Register(registration.RegisterOptions{TermsOfServiceAgreed: true})
+		if eabKid == "" && eabHmac == "" {
+			reg, err = client.Registration.Register(registration.RegisterOptions{
+				TermsOfServiceAgreed: true})
+		} else {
+			reg, err = client.Registration.RegisterWithExternalAccountBinding(registration.RegisterEABOptions{
+				TermsOfServiceAgreed: true,
+				Kid:                  eabKid,
+				HmacEncoded:          eabHmac})
+		}
+
 		if err != nil {
 			return nil, errors.Wrap(err, "ACME CA client registration")
 		}

packager/certfetcher/certfetcher_test.go

@@ -89,8 +89,8 @@ func TestNewFetcher(t *testing.T) {
 		DNSNames: []string{"test.example.com"},
 	}
 
-	fetcher, err := New("test@test.com", &csr, privateKey, apiURL+"/dir",
-		5002, "", 0, "", false)
+	fetcher, err := New("test@test.com", "eab.kid", "eab.hmac", &csr,
+		privateKey, apiURL+"/dir", 5002, "", 0, "", false)
 	assert.Nil(t, err)
 	assert.NotNil(t, fetcher.legoClient)
 	assert.Equal(t, "test@test.com", fetcher.AcmeUser.Email)
@@ -120,8 +120,8 @@ func TestFetchCertSuccess(t *testing.T) {
 		DNSNames: []string{"test.example.com"},
 	}
 
-	fetcher, err := New("test@test.com", &csr, privateKey, apiURL+"/dir",
-		5002, "", 0, "", false)
+	fetcher, err := New("test@test.com", "eab.kid", "eab.hmac", &csr,
+		privateKey, apiURL+"/dir", 5002, "", 0, "", false)
 	assert.Nil(t, err)
 	assert.NotNil(t, fetcher)
 
@@ -151,8 +151,8 @@ func TestFetchCertFail(t *testing.T) {
 		DNSNames: []string{"test.example.com"},
 	}
 
-	fetcher, err := New("test@test.com", &csr, privateKey, apiURL+"/dir",
-		5002, "", 0, "", false)
+	fetcher, err := New("test@test.com", "eab.kid", "eab.hmac", &csr,
+		privateKey, apiURL+"/dir", 5002, "", 0, "", false)
 	assert.Nil(t, err)
 	assert.NotNil(t, fetcher)
 

packager/certloader/certloader.go

@@ -64,11 +64,22 @@ func CreateCertFetcher(config *util.Config, key crypto.PrivateKey, domain string
 		return nil, errors.New("missing acme disco url")
 	}
 	acmeDiscoveryURL := acmeConfig.DiscoURL
+	// Fields for External Account Binding. Some CAs require them, others like
+	// DigiCert, do not. Either both eabKid and eabHmac has to be specified or
+	// none of them are specified.
+	if acmeConfig.EABKid == "" && acmeConfig.EABHmac != "" {
+		return nil, errors.New("EABKid is empty, but EABHmac is not empty, both values need to be set or empty")
+	}
+	if acmeConfig.EABKid != "" && acmeConfig.EABHmac == "" {
+		return nil, errors.New("EABKid is not empty, but EABHmac is empty, both values need to be set or empty")
+	}
+	eabKid := acmeConfig.EABKid
+	eabHmac := acmeConfig.EABHmac
 	if acmeConfig.HttpChallengePort == 0 &&
 		acmeConfig.HttpWebRootDir == "" &&
 		acmeConfig.TlsChallengePort == 0 &&
 		acmeConfig.DnsProvider == "" {
-		return nil, errors.New("One of HttpChallengePort, HttpWebRootDir, TlsChallengePort and DnsProvider must be present.")
+		return nil, errors.New("one of HttpChallengePort, HttpWebRootDir, TlsChallengePort and DnsProvider must be present")
 	}
 	httpChallengePort := acmeConfig.HttpChallengePort
 	httpWebRootDir := acmeConfig.HttpWebRootDir
@@ -83,8 +94,9 @@ func CreateCertFetcher(config *util.Config, key crypto.PrivateKey, domain string
 	}
 
 	// Create the cert fetcher that will auto-renew the cert.
-	certFetcher, err := certfetcher.New(emailAddress, csr, key, acmeDiscoveryURL,
-		httpChallengePort, httpWebRootDir, tlsChallengePort, dnsProvider, true)
+	certFetcher, err := certfetcher.New(emailAddress, eabKid, eabHmac, csr, key,
+		acmeDiscoveryURL, httpChallengePort, httpWebRootDir, tlsChallengePort,
+		dnsProvider, true)
 	if err != nil {
 		return nil, errors.Wrap(err, "creating certfetcher")
 	}

packager/signer/signer.go

@@ -240,12 +240,15 @@ func MutateFetchedContentSecurityPolicy(fetched string) string {
 		}
 	}
 	// Add missing directives or replace the ones that were removed in some cases
+	// NOTE: After changing this string, please update the permalink in
+	// docs/cache_requirements.md.
 	newCsp.WriteString(
 		"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/;" +
 			"style-src 'unsafe-inline' https://cdn.materialdesignicons.com " +
 			"https://cloud.typography.com https://fast.fonts.net " +

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/viewer/;"+
+			"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/;"+
 			"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/util/config.go

@@ -66,10 +66,17 @@ type ACMEConfig struct {
 }
 
 type ACMEServerConfig struct {
-	DiscoURL   string // ACME Directory Resource URL
-	AccountURL string // ACME Account URL. If non-empty, we
-	// will auto-renew cert via ACME.
-	EmailAddress string // Email address registered with ACME CA.
+	// ACME Directory Resource URL
+	AccountURL string
+	// ACME Account URL. If non-empty, we will auto-renew cert via ACME.
+	DiscoURL string
+	// Email address registered with ACME CA.
+	EmailAddress string
+	// Key Identifier from ACME CA. Used for External Account Binding.
+	EABKid string
+	// MAC Key from ACME CA. Used for External Account Binding. Should be in
+	// Base64 URL Encoding without padding format.
+	EABHmac string
 
 	// See: https://letsencrypt.org/docs/challenge-types/
 	// For non-wildcard domains, only one of HttpChallengePort, HttpWebRootDir or