WFE2: Implement badRevocationReason problem type. (letsencrypt#4252)

Daniel McCarney · web-flow · commit 584702bdce34 · 2019-06-06T17:08:41.000-04:00
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.
diff --git a/probs/probs.go b/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,
+	}
+}
diff --git a/probs/probs_test.go b/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 {
diff --git a/revocation/reasons.go b/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, ", ")
+}
diff --git a/wfe2/wfe.go b/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
 	}
diff --git a/wfe2/wfe_test.go b/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}`,
 		},
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -36,6 +36,7 @@ func TestProblemDetailsToStatusCode(t *testing.T) {`
`36`	`36`	`{&ProblemDetails{Type: "foo", HTTPStatus: 200}, 200},`
`37`	`37`	`{&ProblemDetails{Type: ConnectionProblem, HTTPStatus: 200}, 200},`
`38`	`38`	`{&ProblemDetails{Type: AccountDoesNotExistProblem}, http.StatusBadRequest},`
	`39`	`+ {&ProblemDetails{Type: BadRevocationReasonProblem}, http.StatusBadRequest},`
`39`	`40`	`}`
`40`	`41`
`41`	`42`	`for _, c := range testCases {`
`@@ -64,6 +65,7 @@ func TestProblemDetailsConvenience(t *testing.T) {`
`64`	`65`	`{TLSError("TLS error detail"), TLSProblem, http.StatusBadRequest, "TLS error detail"},`
`65`	`66`	`{RejectedIdentifier("rejected identifier detail"), RejectedIdentifierProblem, http.StatusBadRequest, "rejected identifier detail"},`
`66`	`67`	`{AccountDoesNotExist("no account detail"), AccountDoesNotExistProblem, http.StatusBadRequest, "no account detail"},`
	`68`	`+ {BadRevocationReason("only reason xxx is supported"), BadRevocationReasonProblem, http.StatusBadRequest, "only reason xxx is supported"},`
`67`	`69`	`}`
`68`	`70`
`69`	`71`	`for _, c := range testCases {`