Switch to hand-rolled http mux. (#313)

This fixes the error where `/priv/doc/https://` URLs were being
improperly unescaped before fetching/signing.

Also, verify no special URL chars get signed. This complies with the recommendation at
https://wicg.github.io/webpackage/draft-yasskin-http-origin-signed-responses.html#seccons-content-sniffing.

Author: twifkak

Gopkg.lock

@@ -24,18 +24,17 @@ import (
 	"log"
 	"net/http"
 	"net/url"
-	"path"
 	"time"
 
 	"github.com/WICG/webpackage/go/signedexchange"
-	"github.com/julienschmidt/httprouter"
 	"github.com/pkg/errors"
 
 	"github.com/ampproject/amppackager/packager/certcache"
+	"github.com/ampproject/amppackager/packager/mux"
+	"github.com/ampproject/amppackager/packager/rtv"
 	"github.com/ampproject/amppackager/packager/signer"
 	"github.com/ampproject/amppackager/packager/util"
 	"github.com/ampproject/amppackager/packager/validitymap"
-	"github.com/ampproject/amppackager/packager/rtv"
 )
 
 var flagConfig = flag.String("config", "amppkg.toml", "Path to the config toml file.")
@@ -126,32 +125,13 @@ func main() {
 		}
 	}
 
-	packager, err := signer.New(certs[0], key, config.URLSet, rtvCache, certCache.IsHealthy,
+	signer, err := signer.New(certs[0], key, config.URLSet, rtvCache, certCache.IsHealthy,
 		overrideBaseURL, /*requireHeaders=*/!*flagDevelopment, config.ForwardedRequestHeaders)
 	if err != nil {
-		die(errors.Wrap(err, "building packager"))
+		die(errors.Wrap(err, "building signer"))
 	}
 
 	// TODO(twifkak): Make log output configurable.
-	mux := httprouter.New()
-	mux.RedirectTrailingSlash = false
-	mux.RedirectFixedPath = false
-
-	var handlerConfigs = []struct {
-		path string
-		handler httprouter.Handle
-	} {
-		{util.ValidityMapPath, validityMap.ServeHTTP},
-		{"/priv/doc", packager.ServeHTTP},
-		{"/priv/doc/*signURL", packager.ServeHTTP},
-		{path.Join(util.CertURLPrefix, ":certName"), certCache.ServeHTTP},
-	}
-	// GET and HEAD requests are handled identically. http.Server empties
-	// the body before responding to HEAD requests.
-	for _, handlerConfig := range handlerConfigs {
-		mux.GET(handlerConfig.path, handlerConfig.handler)
-		mux.HEAD(handlerConfig.path, handlerConfig.handler)
-	}
 
 	addr := ""
 	if config.LocalOnly {
@@ -162,7 +142,7 @@ func main() {
 		Addr: addr,
 		// Don't use DefaultServeMux, per
 		// https://blog.cloudflare.com/exposing-go-on-the-internet/.
-		Handler:           logIntercept{mux},
+		Handler:           logIntercept{mux.New(certCache, signer, validityMap)},
 		ReadTimeout:       10 * time.Second,
 		ReadHeaderTimeout: 5 * time.Second,
 		// If needing to stream the response, disable WriteTimeout and

cmd/amppkg/main.go

@@ -23,7 +23,6 @@ import (
 	"github.com/ampproject/amppackager/packager/rtv"
 	"github.com/ampproject/amppackager/packager/signer"
 	"github.com/ampproject/amppackager/packager/util"
-	"github.com/julienschmidt/httprouter"
 	"golang.org/x/crypto/ocsp"
 	"google.golang.org/grpc"
 )
@@ -133,7 +132,7 @@ func (s *gatewayServer) GenerateSXG(ctx context.Context, request *pb.SXGRequest)
 
 	httpreq, err := http.NewRequest("GET", baseUrl.String(), nil)
 	httpresp := httptest.NewRecorder()
-	packager.ServeHTTP(httpresp, httpreq, httprouter.Params{})
+	packager.ServeHTTP(httpresp, httpreq)
 
 	// TODO(amaltas): Capture error when signer returns unsigned document.
 	if httpresp.Code != 200 {

cmd/gateway_server/server.go

@@ -29,8 +29,8 @@ import (
 	"time"
 
 	"github.com/WICG/webpackage/go/signedexchange/certurl"
+	"github.com/ampproject/amppackager/packager/mux"
 	"github.com/ampproject/amppackager/packager/util"
-	"github.com/julienschmidt/httprouter"
 	"github.com/pkg/errors"
 	"github.com/pquerna/cachecontrol"
 	"golang.org/x/crypto/ocsp"
@@ -144,8 +144,9 @@ func (this *CertCache) ocspMidpoint(bytes []byte, issuer *x509.Certificate) (tim
 	return resp.ThisUpdate.Add(resp.NextUpdate.Sub(resp.ThisUpdate) / 2), nil
 }
 
-func (this *CertCache) ServeHTTP(resp http.ResponseWriter, req *http.Request, params httprouter.Params) {
-	if params.ByName("certName") == this.certName {
+func (this *CertCache) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
+	params := mux.Params(req)
+	if params["certName"] == this.certName {
 		// https://tools.ietf.org/html/draft-yasskin-httpbis-origin-signed-exchanges-impl-00#section-3.3
 		// This content-type is not standard, but included to reduce
 		// the chance that faulty user agents employ content sniffing.

packager/certcache/certcache.go

@@ -29,9 +29,9 @@ import (
 
 	"github.com/WICG/webpackage/go/signedexchange"
 	"github.com/WICG/webpackage/go/signedexchange/cbor"
+	"github.com/ampproject/amppackager/packager/mux"
 	pkgt "github.com/ampproject/amppackager/packager/testing"
 	"github.com/ampproject/amppackager/packager/util"
-	"github.com/julienschmidt/httprouter"
 	"github.com/stretchr/testify/suite"
 	"golang.org/x/crypto/ocsp"
 )
@@ -133,6 +133,10 @@ func (this *CertCacheSuite) TearDownTest() {
 	}
 }
 
+func (this *CertCacheSuite) mux() http.Handler {
+	return mux.New(this.handler, nil, nil)
+}
+
 func (this *CertCacheSuite) ocspServerCalled(f func()) bool {
 	this.ocspServerWasCalled = false
 	f()
@@ -168,7 +172,7 @@ func (this *CertCacheSuite) DecodeCBOR(r io.Reader) map[string][]byte {
 }
 
 func (this *CertCacheSuite) TestServesCertificate() {
-	resp := pkgt.GetP(this.T(), this.handler, "/amppkg/cert/"+pkgt.CertName, httprouter.Params{httprouter.Param{"certName", pkgt.CertName}})
+	resp := pkgt.Get(this.T(), this.mux(), "/amppkg/cert/"+pkgt.CertName)
 	this.Assert().Equal(http.StatusOK, resp.StatusCode, "incorrect status: %#v", resp)
 	this.Assert().Equal("nosniff", resp.Header.Get("X-Content-Type-Options"))
 	cbor := this.DecodeCBOR(resp.Body)
@@ -178,7 +182,7 @@ func (this *CertCacheSuite) TestServesCertificate() {
 }
 
 func (this *CertCacheSuite) TestServes404OnMissingCertificate() {
-	resp := pkgt.GetP(this.T(), this.handler, "/amppkg/cert/lalala", httprouter.Params{httprouter.Param{"certName", "lalala"}})
+	resp := pkgt.Get(this.T(), this.mux(), "/amppkg/cert/lalala")
 	this.Assert().Equal(http.StatusNotFound, resp.StatusCode, "incorrect status: %#v", resp)
 	body, _ := ioutil.ReadAll(resp.Body)
 	// Small enough not to fit a cert or key:
@@ -187,7 +191,7 @@ func (this *CertCacheSuite) TestServes404OnMissingCertificate() {
 
 func (this *CertCacheSuite) TestOCSP() {
 	// Verify it gets included in the cert-chain+cbor payload.
-	resp := pkgt.GetP(this.T(), this.handler, "/amppkg/cert/"+pkgt.CertName, httprouter.Params{httprouter.Param{"certName", pkgt.CertName}})
+	resp := pkgt.Get(this.T(), this.mux(), "/amppkg/cert/"+pkgt.CertName)
 	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.
 	// TODO(twifkak): Make this less flaky, by injecting a fake clock.
@@ -222,7 +226,7 @@ func (this *CertCacheSuite) TestOCSPExpiry() {
 	}))
 
 	// Verify HTTP response expires immediately:
-	resp := pkgt.GetP(this.T(), this.handler, "/amppkg/cert/"+pkgt.CertName, httprouter.Params{httprouter.Param{"certName", pkgt.CertName}})
+	resp := pkgt.Get(this.T(), this.mux(), "/amppkg/cert/"+pkgt.CertName)
 	this.Assert().Equal("public, max-age=0", resp.Header.Get("Cache-Control"))
 
 	// On update, verify network is called: