fix: rewrite status management (#2428)

acme/api/order.go

@@ -3,6 +3,7 @@ package api
 import (
 	"encoding/base64"
 	"errors"
+	"fmt"
 	"net"
 	"time"
 
@@ -105,7 +106,7 @@ func (o *OrderService) UpdateForCSR(orderURL string, csr []byte) (acme.ExtendedO
 	}
 
 	if order.Status == acme.StatusInvalid {
-		return acme.ExtendedOrder{}, order.Error
+		return acme.ExtendedOrder{}, fmt.Errorf("invalid order: %w", order.Err())
 	}
 
 	return acme.ExtendedOrder{Order: order}, nil

acme/commons.go

@@ -200,6 +200,14 @@ type Order struct {
 	Replaces string `json:"replaces,omitempty"`
 }
 
+func (r *Order) Err() error {
+	if r.Error != nil {
+		return r.Error
+	}
+
+	return nil
+}
+
 // Authorization the ACME authorization object.
 // - https://www.rfc-editor.org/rfc/rfc8555.html#section-7.1.4
 type Authorization struct {
@@ -285,6 +293,14 @@ type Challenge struct {
 	KeyAuthorization string `json:"keyAuthorization"`
 }
 
+func (c *Challenge) Err() error {
+	if c.Error != nil {
+		return c.Error
+	}
+
+	return nil
+}
+
 // Identifier the ACME identifier object.
 // - https://www.rfc-editor.org/rfc/rfc8555.html#section-9.7.7
 type Identifier struct {

acme/errors.go

@@ -25,15 +25,7 @@ type ProblemDetails struct {
 	URL    string `json:"url,omitempty"`
 }
 
-// SubProblem a "subproblems".
-// - https://www.rfc-editor.org/rfc/rfc8555.html#section-6.7.1
-type SubProblem struct {
-	Type       string     `json:"type,omitempty"`
-	Detail     string     `json:"detail,omitempty"`
-	Identifier Identifier `json:"identifier,omitempty"`
-}
-
-func (p ProblemDetails) Error() string {
+func (p *ProblemDetails) Error() string {
 	msg := fmt.Sprintf("acme: error: %d", p.HTTPStatus)
 	if p.Method != "" || p.URL != "" {
 		msg += fmt.Sprintf(" :: %s :: %s", p.Method, p.URL)
@@ -51,6 +43,14 @@ func (p ProblemDetails) Error() string {
 	return msg
 }
 
+// SubProblem a "subproblems".
+// - https://www.rfc-editor.org/rfc/rfc8555.html#section-6.7.1
+type SubProblem struct {
+	Type       string     `json:"type,omitempty"`
+	Detail     string     `json:"detail,omitempty"`
+	Identifier Identifier `json:"identifier,omitempty"`
+}
+
 // NonceError represents the error which is returned
 // if the nonce sent by the client was not accepted by the server.
 type NonceError struct {

certificate/certificates.go

@@ -700,7 +700,7 @@ func checkOrderStatus(order acme.ExtendedOrder) (bool, error) {
 	case acme.StatusValid:
 		return true, nil
 	case acme.StatusInvalid:
-		return false, order.Error
+		return false, fmt.Errorf("invalid order: %w", order.Err())
 	default:
 		return false, nil
 	}

certificate/certificates_test.go

@@ -389,6 +389,51 @@ func Test_Get(t *testing.T) {
 	assert.Equal(t, issuerMock, string(certRes.IssuerCertificate), "IssuerCertificate")
 }
 
+func Test_checkOrderStatus(t *testing.T) {
+	testCases := []struct {
+		desc       string
+		order      acme.Order
+		requireErr require.ErrorAssertionFunc
+		expected   bool
+	}{
+		{
+			desc:       "status valid",
+			order:      acme.Order{Status: acme.StatusValid},
+			requireErr: require.NoError,
+			expected:   true,
+		},
+		{
+			desc:       "status invalid",
+			order:      acme.Order{Status: acme.StatusInvalid},
+			requireErr: require.Error,
+			expected:   false,
+		},
+		{
+			desc:       "status invalid with error",
+			order:      acme.Order{Status: acme.StatusInvalid, Error: &acme.ProblemDetails{}},
+			requireErr: require.Error,
+			expected:   false,
+		},
+		{
+			desc:       "unknown status",
+			order:      acme.Order{Status: "foo"},
+			requireErr: require.NoError,
+			expected:   false,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			t.Parallel()
+
+			status, err := checkOrderStatus(acme.ExtendedOrder{Order: test.order})
+			test.requireErr(t, err)
+
+			assert.Equal(t, test.expected, status)
+		})
+	}
+}
+
 type resolverMock struct {
 	error error
 }

challenge/resolver/solver_manager.go

@@ -124,7 +124,7 @@ func validate(core *api.Core, domain string, chlg acme.Challenge) error {
 			return nil
 		}
 
-		return errors.New("the server didn't respond to our request")
+		return fmt.Errorf("the server didn't respond to our request (status=%s)", authz.Status)
 	}
 
 	return backoff.Retry(operation, bo)
@@ -137,9 +137,9 @@ func checkChallengeStatus(chlng acme.ExtendedChallenge) (bool, error) {
 	case acme.StatusPending, acme.StatusProcessing:
 		return false, nil
 	case acme.StatusInvalid:
-		return false, chlng.Error
+		return false, fmt.Errorf("invalid challenge: %w", chlng.Err())
 	default:
-		return false, errors.New("the server returned an unexpected state")
+		return false, fmt.Errorf("the server returned an unexpected challenge status: %s", chlng.Status)
 	}
 }
 
@@ -154,11 +154,11 @@ func checkAuthorizationStatus(authz acme.Authorization) (bool, error) {
 	case acme.StatusInvalid:
 		for _, chlg := range authz.Challenges {
 			if chlg.Status == acme.StatusInvalid && chlg.Error != nil {
-				return false, chlg.Error
+				return false, fmt.Errorf("invalid authorization: %w", chlg.Err())
 			}
 		}
-		return false, fmt.Errorf("the authorization state %s", authz.Status)
+		return false, errors.New("invalid authorization")
 	default:
-		return false, errors.New("the server returned an unexpected state")
+		return false, fmt.Errorf("the server returned an unexpected authorization status: %s", authz.Status)
 	}
 }

challenge/resolver/solver_manager_test.go

@@ -55,9 +55,6 @@ func TestValidate(t *testing.T) {
 		statuses = statuses[1:]
 
 		chlg := &acme.Challenge{Type: "http-01", Status: st, URL: "http://example.com/", Token: "token"}
-		if st == acme.StatusInvalid {
-			chlg.Error = &acme.ProblemDetails{}
-		}
 
 		err := tester.WriteJSONResponse(w, chlg)
 		if err != nil {
@@ -83,7 +80,6 @@ func TestValidate(t *testing.T) {
 		if st == acme.StatusInvalid {
 			chlg := acme.Challenge{
 				Status: acme.StatusInvalid,
-				Error:  &acme.ProblemDetails{},
 			}
 			authorization.Challenges = append(authorization.Challenges, chlg)
 		}
@@ -106,7 +102,7 @@ func TestValidate(t *testing.T) {
 		{
 			name:     "POST-unexpected",
 			statuses: []string{"weird"},
-			want:     "unexpected",
+			want:     "the server returned an unexpected challenge status: weird",
 		},
 		{
 			name:     "POST-valid",
@@ -115,12 +111,12 @@ func TestValidate(t *testing.T) {
 		{
 			name:     "POST-invalid",
 			statuses: []string{acme.StatusInvalid},
-			want:     "error",
+			want:     "invalid challenge:",
 		},
 		{
 			name:     "POST-pending-unexpected",
 			statuses: []string{acme.StatusPending, "weird"},
-			want:     "unexpected",
+			want:     "the server returned an unexpected authorization status: weird",
 		},
 		{
 			name:     "POST-pending-valid",
@@ -129,7 +125,7 @@ func TestValidate(t *testing.T) {
 		{
 			name:     "POST-pending-invalid",
 			statuses: []string{acme.StatusPending, acme.StatusInvalid},
-			want:     "error",
+			want:     "invalid authorization",
 		},
 	}
 
@@ -148,6 +144,126 @@ func TestValidate(t *testing.T) {
 	}
 }
 
+func Test_checkChallengeStatus(t *testing.T) {
+	testCases := []struct {
+		desc       string
+		challenge  acme.Challenge
+		requireErr require.ErrorAssertionFunc
+		expected   bool
+	}{
+		{
+			desc:       "status valid",
+			challenge:  acme.Challenge{Status: acme.StatusValid},
+			requireErr: require.NoError,
+			expected:   true,
+		},
+		{
+			desc:       "status invalid",
+			challenge:  acme.Challenge{Status: acme.StatusInvalid},
+			requireErr: require.Error,
+			expected:   false,
+		},
+		{
+			desc:       "status invalid with error",
+			challenge:  acme.Challenge{Status: acme.StatusInvalid, Error: &acme.ProblemDetails{}},
+			requireErr: require.Error,
+			expected:   false,
+		},
+		{
+			desc:       "status pending",
+			challenge:  acme.Challenge{Status: acme.StatusPending},
+			requireErr: require.NoError,
+			expected:   false,
+		},
+		{
+			desc:       "status processing",
+			challenge:  acme.Challenge{Status: acme.StatusProcessing},
+			requireErr: require.NoError,
+			expected:   false,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			t.Parallel()
+
+			status, err := checkChallengeStatus(acme.ExtendedChallenge{Challenge: test.challenge})
+			test.requireErr(t, err)
+
+			assert.Equal(t, test.expected, status)
+		})
+	}
+}
+
+func Test_checkAuthorizationStatus(t *testing.T) {
+	testCases := []struct {
+		desc          string
+		authorization acme.Authorization
+		requireErr    require.ErrorAssertionFunc
+		expected      bool
+	}{
+		{
+			desc:          "status valid",
+			authorization: acme.Authorization{Status: acme.StatusValid},
+			requireErr:    require.NoError,
+			expected:      true,
+		},
+		{
+			desc:          "status invalid",
+			authorization: acme.Authorization{Status: acme.StatusInvalid},
+			requireErr:    require.Error,
+			expected:      false,
+		},
+		{
+			desc:          "status invalid with error",
+			authorization: acme.Authorization{Status: acme.StatusInvalid, Challenges: []acme.Challenge{{Error: &acme.ProblemDetails{}}}},
+			requireErr:    require.Error,
+			expected:      false,
+		},
+		{
+			desc:          "status pending",
+			authorization: acme.Authorization{Status: acme.StatusPending},
+			requireErr:    require.NoError,
+			expected:      false,
+		},
+		{
+			desc:          "status processing",
+			authorization: acme.Authorization{Status: acme.StatusProcessing},
+			requireErr:    require.NoError,
+			expected:      false,
+		},
+		{
+			desc:          "status deactivated",
+			authorization: acme.Authorization{Status: acme.StatusDeactivated},
+			requireErr:    require.Error,
+			expected:      false,
+		},
+		{
+			desc:          "status expired",
+			authorization: acme.Authorization{Status: acme.StatusExpired},
+			requireErr:    require.Error,
+			expected:      false,
+		},
+		{
+			desc:          "status revoked",
+			authorization: acme.Authorization{Status: acme.StatusRevoked},
+			requireErr:    require.Error,
+			expected:      false,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			t.Parallel()
+
+			status, err := checkAuthorizationStatus(test.authorization)
+			test.requireErr(t, err)
+
+			assert.Equal(t, test.expected, status)
+		})
+	}
+}
+
 // validateNoBody reads the http.Request POST body, parses the JWS and validates it to read the body.
 // If there is an error doing this,
 // or if the JWS body is not the empty JSON payload "{}" or a POST-as-GET payload "" an error is returned.

registration/registar.go

@@ -60,7 +60,7 @@ func (r *Registrar) Register(options RegisterOptions) (*Resource, error) {
 	account, err := r.core.Accounts.New(accMsg)
 	if err != nil {
 		// seems impossible
-		var errorDetails acme.ProblemDetails
+		errorDetails := &acme.ProblemDetails{}
 		if !errors.As(err, &errorDetails) || errorDetails.HTTPStatus != http.StatusConflict {
 			return nil, err
 		}
@@ -84,7 +84,7 @@ func (r *Registrar) RegisterWithExternalAccountBinding(options RegisterEABOption
 	account, err := r.core.Accounts.NewEAB(accMsg, options.Kid, options.HmacEncoded)
 	if err != nil {
 		// seems impossible
-		var errorDetails acme.ProblemDetails
+		errorDetails := &acme.ProblemDetails{}
 		if !errors.As(err, &errorDetails) || errorDetails.HTTPStatus != http.StatusConflict {
 			return nil, err
 		}