WFE2: Implement badRevocationReason problem type. (#4252)

Previously we were returning a Malformed problem type where RFC 8555
mandates the use of badRevocationReason and encourages including the
allowed reasons in the problem detail.

probs/probs.go

@@ -26,6 +26,7 @@ const (
 	OrderNotReadyProblem         = ProblemType("orderNotReady")
 	BadSignatureAlgorithmProblem = ProblemType("badSignatureAlgorithm")
 	BadPublicKeyProblem          = ProblemType("badPublicKey")
+	BadRevocationReasonProblem   = ProblemType("badRevocationReason")
 
 	V1ErrorNS = "urn:acme:error:"
 	V2ErrorNS = "urn:ietf:params:acme:error:"
@@ -92,7 +93,8 @@ func ProblemDetailsToStatusCode(prob *ProblemDetails) int {
 		BadNonceProblem,
 		InvalidEmailProblem,
 		RejectedIdentifierProblem,
-		AccountDoesNotExistProblem:
+		AccountDoesNotExistProblem,
+		BadRevocationReasonProblem:
 		return http.StatusBadRequest
 	case ServerInternalProblem:
 		return http.StatusInternalServerError
@@ -320,3 +322,13 @@ func OrderNotReady(detail string, a ...interface{}) *ProblemDetails {
 		HTTPStatus: http.StatusForbidden,
 	}
 }
+
+// BadRevocationReason returns a ProblemDetails representing
+// a BadRevocationReasonProblem
+func BadRevocationReason(detail string, a ...interface{}) *ProblemDetails {
+	return &ProblemDetails{
+		Type:       BadRevocationReasonProblem,
+		Detail:     fmt.Sprintf(detail, a...),
+		HTTPStatus: http.StatusBadRequest,
+	}
+}

probs/probs_test.go

@@ -36,6 +36,7 @@ func TestProblemDetailsToStatusCode(t *testing.T) {
 		{&ProblemDetails{Type: "foo", HTTPStatus: 200}, 200},
 		{&ProblemDetails{Type: ConnectionProblem, HTTPStatus: 200}, 200},
 		{&ProblemDetails{Type: AccountDoesNotExistProblem}, http.StatusBadRequest},
+		{&ProblemDetails{Type: BadRevocationReasonProblem}, http.StatusBadRequest},
 	}
 
 	for _, c := range testCases {
@@ -64,6 +65,7 @@ func TestProblemDetailsConvenience(t *testing.T) {
 		{TLSError("TLS error detail"), TLSProblem, http.StatusBadRequest, "TLS error detail"},
 		{RejectedIdentifier("rejected identifier detail"), RejectedIdentifierProblem, http.StatusBadRequest, "rejected identifier detail"},
 		{AccountDoesNotExist("no account detail"), AccountDoesNotExistProblem, http.StatusBadRequest, "no account detail"},
+		{BadRevocationReason("only reason xxx is supported"), BadRevocationReasonProblem, http.StatusBadRequest, "only reason xxx is supported"},
 	}
 
 	for _, c := range testCases {

revocation/reasons.go

@@ -1,5 +1,11 @@
 package revocation
 
+import (
+	"fmt"
+	"sort"
+	"strings"
+)
+
 // Reason is used to specify a certificate revocation reason
 type Reason int
 
@@ -44,3 +50,26 @@ var UserAllowedReasons = map[Reason]struct{}{
 	Superseded:           {}, // superseded
 	CessationOfOperation: {}, // cessationOfOperation
 }
+
+// UserAllowedReasonsMessage creates a string describing a list of user allowed
+// revocation reasons. This is useful when a revocation is rejected because it
+// is not a valid user supplied reason and the allowed values must be
+// communicated.
+func UserAllowedReasonsMessage() string {
+	// Build a slice of ints from the allowed reason codes.
+	// We want a slice because iterating `UserAllowedReasons` will change order
+	// and make the message unpredictable and cumbersome for unit testing.
+	// We use []ints instead of []Reason to use `sort.Ints` without fuss.
+	var allowed []int
+	for reason, _ := range UserAllowedReasons {
+		allowed = append(allowed, int(reason))
+	}
+	sort.Ints(allowed)
+
+	var reasonStrings []string
+	for _, reason := range allowed {
+		reasonStrings = append(reasonStrings, fmt.Sprintf("%s (%d)",
+			ReasonToString[Reason(reason)], reason))
+	}
+	return strings.Join(reasonStrings, ", ")
+}

wfe2/wfe.go

@@ -749,7 +749,15 @@ func (wfe *WebFrontEndImpl) processRevocation(
 	reason := revocation.Reason(0)
 	if revokeRequest.Reason != nil && wfe.AcceptRevocationReason {
 		if _, present := revocation.UserAllowedReasons[*revokeRequest.Reason]; !present {
-			return probs.Malformed("unsupported revocation reason code provided")
+			reasonStr, ok := revocation.ReasonToString[revocation.Reason(*revokeRequest.Reason)]
+			if !ok {
+				reasonStr = "unknown"
+			}
+			return probs.BadRevocationReason(
+				"unsupported revocation reason code provided: %s (%d). Supported reasons: %s",
+				reasonStr,
+				*revokeRequest.Reason,
+				revocation.UserAllowedReasonsMessage())
 		}
 		reason = *revokeRequest.Reason
 	}

wfe2/wfe_test.go

@@ -2631,13 +2631,13 @@ func TestRevokeCertificateReasons(t *testing.T) {
 			Name:             "Unsupported reason",
 			Reason:           &reason2,
 			ExpectedHTTPCode: http.StatusBadRequest,
-			ExpectedBody:     `{"type":"` + probs.V2ErrorNS + `malformed","detail":"unsupported revocation reason code provided","status":400}`,
+			ExpectedBody:     `{"type":"` + probs.V2ErrorNS + `badRevocationReason","detail":"unsupported revocation reason code provided: cACompromise (2). Supported reasons: unspecified (0), keyCompromise (1), affiliationChanged (3), superseded (4), cessationOfOperation (5)","status":400}`,
 		},
 		{
 			Name:             "Non-existent reason",
 			Reason:           &reason100,
 			ExpectedHTTPCode: http.StatusBadRequest,
-			ExpectedBody:     `{"type":"` + probs.V2ErrorNS + `malformed","detail":"unsupported revocation reason code provided","status":400}`,
+			ExpectedBody:     `{"type":"` + probs.V2ErrorNS + `badRevocationReason","detail":"unsupported revocation reason code provided: unknown (100). Supported reasons: unspecified (0), keyCompromise (1), affiliationChanged (3), superseded (4), cessationOfOperation (5)","status":400}`,
 		},
 	}