WFE2: allow POST-as-GET for directory & newNonce endpoints. (#4595)

RFC 8555 §6.3 says the server's directory and newNonce endpoints should
support POST-as-GET as well as GET.

wfe2/wfe.go

@@ -341,10 +341,6 @@ func (wfe *WebFrontEndImpl) Handler() http.Handler {
 	wfe.HandleFunc(m, issuerPath, wfe.Issuer, "GET")
 	wfe.HandleFunc(m, buildIDPath, wfe.BuildID, "GET")
 
-	// GETable ACME endpoints
-	wfe.HandleFunc(m, directoryPath, wfe.Directory, "GET")
-	wfe.HandleFunc(m, newNoncePath, wfe.Nonce, "GET")
-
 	// POSTable ACME endpoints
 	wfe.HandleFunc(m, newAcctPath, wfe.NewAccount, "POST")
 	wfe.HandleFunc(m, acctPath, wfe.Account, "POST")
@@ -353,6 +349,9 @@ func (wfe *WebFrontEndImpl) Handler() http.Handler {
 	wfe.HandleFunc(m, newOrderPath, wfe.NewOrder, "POST")
 	wfe.HandleFunc(m, finalizeOrderPath, wfe.FinalizeOrder, "POST")
 
+	// GETable and POST-as-GETable ACME endpoints
+	wfe.HandleFunc(m, directoryPath, wfe.Directory, "GET", "POST")
+	wfe.HandleFunc(m, newNoncePath, wfe.Nonce, "GET", "POST")
 	// POST-as-GETable ACME endpoints
 	// TODO(@cpu): After November 1st, 2019 support for "GET" to the following
 	// endpoints will be removed, leaving only POST-as-GET support.
@@ -426,6 +425,15 @@ func (wfe *WebFrontEndImpl) Directory(
 		"keyChange":  rolloverPath,
 	}
 
+	if request.Method == http.MethodPost {
+		acct, prob := wfe.validPOSTAsGETForAccount(request, ctx, logEvent)
+		if prob != nil {
+			wfe.sendError(response, logEvent, prob, nil)
+			return
+		}
+		logEvent.Requester = acct.ID
+	}
+
 	// Add a random key to the directory in order to make sure that clients don't hardcode an
 	// expected set of keys. This ensures that we can properly extend the directory when we
 	// need to add a new endpoint or meta element.
@@ -473,12 +481,21 @@ func (wfe *WebFrontEndImpl) Nonce(
 	logEvent *web.RequestEvent,
 	response http.ResponseWriter,
 	request *http.Request) {
+	if request.Method == http.MethodPost {
+		acct, prob := wfe.validPOSTAsGETForAccount(request, ctx, logEvent)
+		if prob != nil {
+			wfe.sendError(response, logEvent, prob, nil)
+			return
+		}
+		logEvent.Requester = acct.ID
+	}
+
 	statusCode := http.StatusNoContent
 	// The ACME specification says GET requets should receive http.StatusNoContent
-	// and HEAD requests should receive http.StatusOK. We gate this with the
+	// and HEAD/POST-as-GET requests should receive http.StatusOK. We gate this
-	// HeadNonceStatusOK feature flag because it may break clients that are
+	// with the HeadNonceStatusOK feature flag because it may break clients that
-	// programmed to expect StatusOK.
+	// are programmed to expect StatusOK.
-	if features.Enabled(features.HeadNonceStatusOK) && request.Method == "HEAD" {
+	if features.Enabled(features.HeadNonceStatusOK) && request.Method != "GET" {
 		statusCode = http.StatusOK
 	}
 	response.WriteHeader(statusCode)

wfe2/wfe_test.go

@@ -717,8 +717,28 @@ func TestDirectory(t *testing.T) {
 	core.RandReader = fakeRand{}
 	defer func() { core.RandReader = rand.Reader }()
 
-	// Directory with a key change endpoint and a meta entry
+	dirURL, _ := url.Parse("/directory")
-	metaJSON := `{
+
+	getReq := &http.Request{
+		Method: http.MethodGet,
+		URL:    dirURL,
+		Host:   "localhost:4300",
+	}
+
+	_, _, jwsBody := signRequestKeyID(t, 1, nil, "http://localhost/directory", "", wfe.nonceService)
+	postAsGetReq := makePostRequestWithPath("/directory", jwsBody)
+
+	testCases := []struct {
+		name         string
+		caaIdent     string
+		website      string
+		expectedJSON string
+		request      *http.Request
+	}{
+		{
+			name:    "standard GET, no CAA ident/website meta",
+			request: getReq,
+			expectedJSON: `{
   "keyChange": "http://localhost:4300/acme/key-change",
   "meta": {
     "termsOfService": "http://example.invalid/terms"
@@ -728,36 +748,14 @@ func TestDirectory(t *testing.T) {
   "newOrder": "http://localhost:4300/acme/new-order",
   "revokeCert": "http://localhost:4300/acme/revoke-cert",
   "AAAAAAAAAAA": "https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417"
-}`
+}`,
-
+		},
-	// NOTE: the req.URL will be modified and must be constructed per
+		{
-	// testcase or things will break and you will be confused and sad.
+			name:     "standard GET, CAA ident/website meta",
-	url, _ := url.Parse("/directory")
+			caaIdent: "Radiant Lock",
-	req := &http.Request{
+			website:  "zombo.com",
-		Method: "GET",
+			request:  getReq,
-		URL:    url,
+			expectedJSON: `{
-		Host:   "localhost:4300",
-	}
-	// Serve the /directory response for this request into a recorder
-	responseWriter := httptest.NewRecorder()
-	mux.ServeHTTP(responseWriter, req)
-	// We expect all directory requests to return a json object with a good HTTP status
-	test.AssertEquals(t, responseWriter.Header().Get("Content-Type"), "application/json")
-	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
-	test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), metaJSON)
-	// Check if there is a random directory key present and if so, that it is
-	// expected to be present
-	test.AssertEquals(t,
-		randomDirectoryKeyPresent(t, responseWriter.Body.Bytes()),
-		true)
-
-	// Configure a caaIdentity and website for the /directory meta
-	wfe.DirectoryCAAIdentity = "Radiant Lock"
-	wfe.DirectoryWebsite = "zombo.com"
-
-	// Expect directory with a key change endpoint and a meta entry that has both
-	// a website and a caaIdentity
-	metaJSON = `{
   "AAAAAAAAAAA": "https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417",
   "keyChange": "http://localhost:4300/acme/key-change",
   "meta": {
@@ -771,19 +769,51 @@ func TestDirectory(t *testing.T) {
   "newNonce": "http://localhost:4300/acme/new-nonce",
   "newOrder": "http://localhost:4300/acme/new-order",
   "revokeCert": "http://localhost:4300/acme/revoke-cert"
-}`
+}`,
-	// Serve the /directory response for this request into a recorder
+		},
-	responseWriter = httptest.NewRecorder()
+		{
-	mux.ServeHTTP(responseWriter, req)
+			name:     "POST-as-GET, CAA ident/website meta",
-	// We expect all directory requests to return a json object with a good HTTP status
+			caaIdent: "Radiant Lock",
-	test.AssertEquals(t, responseWriter.Header().Get("Content-Type"), "application/json")
+			website:  "zombo.com",
-	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
+			request:  postAsGetReq,
-	test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), metaJSON)
+			expectedJSON: `{
-	// Check if there is a random directory key present and if so, that it is
+  "AAAAAAAAAAA": "https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417",
-	// expected to be present
+  "keyChange": "http://localhost/acme/key-change",
-	test.AssertEquals(t,
+  "meta": {
-		randomDirectoryKeyPresent(t, responseWriter.Body.Bytes()),
+    "caaIdentities": [
-		true)
+      "Radiant Lock"
+    ],
+    "termsOfService": "http://example.invalid/terms",
+    "website": "zombo.com"
+  },
+  "newAccount": "http://localhost/acme/new-acct",
+  "newNonce": "http://localhost/acme/new-nonce",
+  "newOrder": "http://localhost/acme/new-order",
+  "revokeCert": "http://localhost/acme/revoke-cert"
+}`,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Configure a caaIdentity and website for the /directory meta based on the tc
+			wfe.DirectoryCAAIdentity = tc.caaIdent // "Radiant Lock"
+			wfe.DirectoryWebsite = tc.website      //"zombo.com"
+			responseWriter := httptest.NewRecorder()
+			// Serve the /directory response for this request into a recorder
+			mux.ServeHTTP(responseWriter, tc.request)
+			// We expect all directory requests to return a json object with a good HTTP status
+			test.AssertEquals(t, responseWriter.Header().Get("Content-Type"), "application/json")
+			// We expect all requests to return status OK
+			test.AssertEquals(t, responseWriter.Code, http.StatusOK)
+			// The response should match expected
+			test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), tc.expectedJSON)
+			// Check that the random directory key is present
+			test.AssertEquals(t,
+				randomDirectoryKeyPresent(t, responseWriter.Body.Bytes()),
+				true)
+		})
+	}
 }
 
 func TestRelativeDirectory(t *testing.T) {
@@ -847,37 +877,64 @@ func TestRelativeDirectory(t *testing.T) {
 }
 
 // TestNonceEndpoint tests requests to the WFE2's new-nonce endpoint
-func TestNonceEndpointGET(t *testing.T) {
+func TestNonceEndpoint(t *testing.T) {
 	wfe, _ := setupWFE(t)
 	mux := wfe.Handler()
 
+	getReq := &http.Request{
+		Method: http.MethodGet,
+		URL:    mustParseURL(newNoncePath),
+	}
+	headReq := &http.Request{
+		Method: http.MethodHead,
+		URL:    mustParseURL(newNoncePath),
+	}
+
+	// Make two PAG requests. We can't use the same req twice because the nonce in
+	// the signature will be stale the 2nd time.
+	_, _, jwsBodyA := signRequestKeyID(t, 1, nil, fmt.Sprintf("http://localhost%s", newNoncePath), "", wfe.nonceService)
+	_, _, jwsBodyB := signRequestKeyID(t, 1, nil, fmt.Sprintf("http://localhost%s", newNoncePath), "", wfe.nonceService)
+	postAsGetReqA := makePostRequestWithPath(newNoncePath, jwsBodyA)
+	postAsGetReqB := makePostRequestWithPath(newNoncePath, jwsBodyB)
+
 	testCases := []struct {
-		Name              string
+		name              string
-		Method            string
+		request           *http.Request
-		ExpectedStatus    int
+		expectedStatus    int
-		HeadNonceStatusOK bool
+		headNonceStatusOK bool
 	}{
 		{
-			Name:           "GET new-nonce request",
+			name:           "GET new-nonce request",
-			Method:         "GET",
+			request:        getReq,
-			ExpectedStatus: http.StatusNoContent,
+			expectedStatus: http.StatusNoContent,
 		},
 		{
-			Name:           "HEAD new-nonce request (legacy)",
+			name:           "HEAD new-nonce request (legacy status code)",
-			Method:         "HEAD",
+			request:        headReq,
-			ExpectedStatus: http.StatusNoContent,
+			expectedStatus: http.StatusNoContent,
 		},
 		{
-			Name:              "HEAD new-nonce request (feature flag)",
+			name:              "HEAD new-nonce request (ok status code)",
-			Method:            "HEAD",
+			request:           headReq,
-			ExpectedStatus:    http.StatusOK,
+			expectedStatus:    http.StatusOK,
-			HeadNonceStatusOK: true,
+			headNonceStatusOK: true,
+		},
+		{
+			name:           "POST-as-GET new-nonce request (legacy status code)",
+			request:        postAsGetReqA,
+			expectedStatus: http.StatusNoContent,
+		},
+		{
+			name:              "POST-as-GET new-nonce request (ok status code)",
+			request:           postAsGetReqB,
+			expectedStatus:    http.StatusOK,
+			headNonceStatusOK: true,
 		},
 	}
 
 	for _, tc := range testCases {
-		t.Run(tc.Name, func(t *testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
-			if tc.HeadNonceStatusOK {
+			if tc.headNonceStatusOK {
 				if err := features.Set(map[string]bool{"HeadNonceStatusOK": true}); err != nil {
 					t.Fatalf("Failed to enable HeadNonceStatusOK feature: %v", err)
 				}
@@ -885,12 +942,9 @@ func TestNonceEndpointGET(t *testing.T) {
 			}
 
 			responseWriter := httptest.NewRecorder()
-			mux.ServeHTTP(responseWriter, &http.Request{
+			mux.ServeHTTP(responseWriter, tc.request)
-				Method: tc.Method,
-				URL:    mustParseURL(newNoncePath),
-			})
 			// The response should have the expected HTTP status code
-			test.AssertEquals(t, responseWriter.Code, tc.ExpectedStatus)
+			test.AssertEquals(t, responseWriter.Code, tc.expectedStatus)
 			// And the response should contain a valid nonce in the Replay-Nonce header
 			nonce := responseWriter.Header().Get("Replay-Nonce")
 			test.AssertEquals(t, wfe.nonceService.Valid(nonce), true)
@@ -919,9 +973,9 @@ func TestHTTPMethods(t *testing.T) {
 			Allowed: getOnly,
 		},
 		{
-			Name:    "Directory path should be GET only",
+			Name:    "Directory path should be GET or POST only",
 			Path:    directoryPath,
-			Allowed: getOnly,
+			Allowed: getOrPost,
 		},
 		{
 			Name:    "NewAcct path should be POST only",
@@ -983,9 +1037,9 @@ func TestHTTPMethods(t *testing.T) {
 			Allowed: getOrPost,
 		},
 		{
-			Name:    "Nonce path should be GET only",
+			Name:    "Nonce path should be GET or POST only",
 			Path:    newNoncePath,
-			Allowed: getOnly,
+			Allowed: getOrPost,
 		},
 	}