Fix OCSP-Responder double slash collapsing (letsencrypt#2748)

Roland Bracewell Shoemaker · cpu · commit bd045b9325ca · 2017-05-10T09:51:10.000-04:00
Uses a special mux for the OCSP Responder so that we stop collapsing double slashes in GET requests which cause a small number of requests to be considered malformed.
diff --git a/cmd/ocsp-responder/main.go b/cmd/ocsp-responder/main.go
@@ -234,6 +234,19 @@ as generated by Boulder's single-ocsp command.
 	<-forever
 }
 
+// ocspMux partially implements the interface defined for http.ServeMux but doesn't implement
+// the path cleaning its Handler method does. Notably http.ServeMux will collapse repeated
+// slashes into a single slash which breaks the base64 encoding that is used in OCSP GET
+// requests. CFSSL explicitly recommends against using http.ServeMux for this reason:
+// https://github.com/cloudflare/cfssl/blob/6388e1ec18d2933c35f0f8dfbbe383713eb04b1e/ocsp/responder.go#L170
+type ocspMux struct {
+	handler http.Handler
+}
+
+func (om *ocspMux) Handler(_ *http.Request) (http.Handler, string) {
+	return om.handler, "/"
+}
+
 func mux(scope metrics.Scope, responderPath string, source cfocsp.Source) http.Handler {
 	stripPrefix := http.StripPrefix(responderPath, cfocsp.NewResponder(source))
 	h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -244,7 +257,5 @@ func mux(scope metrics.Scope, responderPath string, source cfocsp.Source) http.H
 		}
 		stripPrefix.ServeHTTP(w, r)
 	})
-	m := http.NewServeMux()
-	m.Handle("/", h)
-	return measured_http.New(m, clock.Default())
+	return measured_http.New(&ocspMux{h}, clock.Default())
 }
diff --git a/cmd/ocsp-responder/main_test.go b/cmd/ocsp-responder/main_test.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"bytes"
+	"encoding/base64"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -29,16 +30,29 @@ func TestMux(t *testing.T) {
 	if err != nil {
 		t.Fatalf("ocsp.ParseRequest: %s", err)
 	}
+	doubleSlashBytes, err := base64.StdEncoding.DecodeString("MFMwUTBPME0wSzAJBgUrDgMCGgUABBR+5mrncpqz/PiiIGRsFqEtYHEIXQQUqEpqYwR93brm0Tm3pkVl7/Oo7KECEgO/AC2R1FW8hePAj4xp//8Jhw==")
+	if err != nil {
+		t.Fatalf("failed to decode double slash OCSP request")
+	}
+	doubleSlashReq, err := ocsp.ParseRequest(doubleSlashBytes)
+	if err != nil {
+		t.Fatalf("failed to parse double slash OCSP request")
+	}
 	src := make(cfocsp.InMemorySource)
 	src[ocspReq.SerialNumber.String()] = resp.OCSPResponse
+	src[doubleSlashReq.SerialNumber.String()] = resp.OCSPResponse
 	h := mux(stats, "/foobar/", src)
 	type muxTest struct {
 		method   string
 		path     string
 		reqBody  []byte
 		respBody []byte
 	}
-	mts := []muxTest{{"POST", "/foobar/", req, resp.OCSPResponse}, {"GET", "/", nil, nil}}
+	mts := []muxTest{
+		{"POST", "/foobar/", req, resp.OCSPResponse},
+		{"GET", "/", nil, nil},
+		{"GET", "/foobar/MFMwUTBPME0wSzAJBgUrDgMCGgUABBR+5mrncpqz/PiiIGRsFqEtYHEIXQQUqEpqYwR93brm0Tm3pkVl7/Oo7KECEgO/AC2R1FW8hePAj4xp//8Jhw==", nil, resp.OCSPResponse},
+	}
 	for i, mt := range mts {
 		w := httptest.NewRecorder()
 		r, err := http.NewRequest(mt.method, mt.path, bytes.NewReader(mt.reqBody))
diff --git a/metrics/measured_http/http.go b/metrics/measured_http/http.go
@@ -34,17 +34,25 @@ func (r *responseWriterWithStatus) WriteHeader(code int) {
 	r.ResponseWriter.WriteHeader(code)
 }
 
+// serveMux is a partial interface wrapper for the method http.ServeMux
+// exposes that we use. This is needed so that we can replace the default
+// http.ServeMux in ocsp-responder where we don't want to use its path
+// canonicalization.
+type serveMux interface {
+	Handler(*http.Request) (http.Handler, string)
+}
+
 // MeasuredHandler wraps an http.Handler and records prometheus stats
 type MeasuredHandler struct {
-	*http.ServeMux
+	serveMux
 	clk clock.Clock
 	// Normally this is always responseTime, but we override it for testing.
 	stat *prometheus.HistogramVec
 }
 
-func New(m *http.ServeMux, clk clock.Clock) *MeasuredHandler {
+func New(m serveMux, clk clock.Clock) *MeasuredHandler {
 	return &MeasuredHandler{
-		ServeMux: m,
+		serveMux: m,
 		clk:      clk,
 		stat:     responseTime,
 	}
diff --git a/metrics/measured_http/http_test.go b/metrics/measured_http/http_test.go
@@ -36,7 +36,7 @@ func TestMeasuring(t *testing.T) {
 	mux := http.NewServeMux()
 	mux.Handle("/foo", sleepyHandler{clk})
 	mh := MeasuredHandler{
-		ServeMux: mux,
+		serveMux: mux,
 		clk:      clk,
 		stat:     stat,
 	}

Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ func TestMeasuring(t *testing.T) {`
`36`	`36`	`mux := http.NewServeMux()`
`37`	`37`	`mux.Handle("/foo", sleepyHandler{clk})`
`38`	`38`	`mh := MeasuredHandler{`
`39`		`- ServeMux: mux,`
	`39`	`+ serveMux: mux,`
`40`	`40`	`clk: clk,`
`41`	`41`	`stat: stat,`
`42`	`42`	`}`