Fix order, authorization, challenge status when failing to validate a challenge (#20914)

* Fix ACME computed order status

* Return validation errors and status updates for authorizations

 - We now populate the error field within challenges with the error results from the challenge
 - Update the status of the challenge and authorizations to invalid when we give up on the challenge
 - Verify that only a single challenge within a given authorization can be accepted to avoid race conditions.

builtin/logical/pki/acme_challenge_engine.go

@@ -234,11 +234,21 @@ func (ace *ACMEChallengeEngine) AcceptChallenge(sc *storageContext, account stri
 	}
 
 	if authz.Status != ACMEAuthorizationPending {
-		return fmt.Errorf("cannot accept already validated authorization %v (%v)", authz.Id, authz.Status)
+		return fmt.Errorf("%w: cannot accept already validated authorization %v (%v)", ErrMalformed, authz.Id, authz.Status)
 	}
 
-	if challenge.Status != ACMEChallengePending && challenge.Status != ACMEChallengeProcessing {
-		return fmt.Errorf("challenge is in invalid state (%v) in authorization %v", challenge.Status, authz.Id)
+	for _, otherChallenge := range authz.Challenges {
+		// We assume within an authorization we won't have multiple challenges of the same challenge type
+		// and we want to limit a single challenge being in a processing state to avoid race conditions
+		// failing one challenge and passing another.
+		if otherChallenge.Type != challenge.Type && otherChallenge.Status != ACMEChallengePending {
+			return fmt.Errorf("%w: only a single challenge within an authorization can be accepted (%v) in status %v", ErrMalformed, otherChallenge.Type, otherChallenge.Status)
+		}
+
+		// The requested challenge can ping us to wake us up, so allow pending and currently processing statuses
+		if otherChallenge.Status != ACMEChallengePending && otherChallenge.Status != ACMEChallengeProcessing {
+			return fmt.Errorf("%w: challenge is in invalid state (%v) in authorization %v", ErrMalformed, challenge.Status, authz.Id)
+		}
 	}
 
 	token := challenge.ChallengeFields["token"].(string)
@@ -391,8 +401,8 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 
 		valid, err = ValidateHTTP01Challenge(authz.Identifier.Value, cv.Token, cv.Thumbprint, config)
 		if err != nil {
-			err = fmt.Errorf("error validating http-01 challenge %v: %w", id, err)
-			return ace._verifyChallengeRetry(sc, cv, authz, err, id)
+			err = fmt.Errorf("%w: error validating http-01 challenge %v: %s", ErrIncorrectResponse, id, err.Error())
+			return ace._verifyChallengeRetry(sc, cv, authzPath, authz, challenge, err, id)
 		}
 	case ACMEDNSChallenge:
 		if authz.Identifier.Type != ACMEDNSIdentifier {
@@ -402,8 +412,8 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 
 		valid, err = ValidateDNS01Challenge(authz.Identifier.Value, cv.Token, cv.Thumbprint, config)
 		if err != nil {
-			err = fmt.Errorf("error validating dns-01 challenge %v: %w", id, err)
-			return ace._verifyChallengeRetry(sc, cv, authz, err, id)
+			err = fmt.Errorf("%w: error validating dns-01 challenge %v: %s", ErrIncorrectResponse, id, err.Error())
+			return ace._verifyChallengeRetry(sc, cv, authzPath, authz, challenge, err, id)
 		}
 	default:
 		err = fmt.Errorf("unsupported ACME challenge type %v for challenge %v", cv.ChallengeType, id)
@@ -411,8 +421,8 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 	}
 
 	if !valid {
-		err = fmt.Errorf("challenge failed with no additional information")
-		return ace._verifyChallengeRetry(sc, cv, authz, err, id)
+		err = fmt.Errorf("%w: challenge failed with no additional information", ErrIncorrectResponse)
+		return ace._verifyChallengeRetry(sc, cv, authzPath, authz, challenge, err, id)
 	}
 
 	// If we got here, the challenge verification was successful. Update
@@ -420,23 +430,28 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 	expires := now.Add(15 * 24 * time.Hour)
 	challenge.Status = ACMEChallengeValid
 	challenge.Validated = now.Format(time.RFC3339)
+	challenge.Error = nil
 	authz.Status = ACMEAuthorizationValid
 	authz.Expires = expires.Format(time.RFC3339)
 
 	if err := saveAuthorizationAtPath(sc, authzPath, authz); err != nil {
 		err = fmt.Errorf("error saving updated (validated) authorization %v/%v for challenge %v: %w", cv.Account, cv.Authorization, id, err)
-		return ace._verifyChallengeRetry(sc, cv, authz, err, id)
+		return ace._verifyChallengeRetry(sc, cv, authzPath, authz, challenge, err, id)
 	}
 
 	return ace._verifyChallengeCleanup(sc, nil, id)
 }
 
-func (ace *ACMEChallengeEngine) _verifyChallengeRetry(sc *storageContext, cv *ChallengeValidation, authz *ACMEAuthorization, err error, id string) (bool, time.Time, error) {
+func (ace *ACMEChallengeEngine) _verifyChallengeRetry(sc *storageContext, cv *ChallengeValidation, authzPath string, auth *ACMEAuthorization, challenge *ACMEChallenge, verificationErr error, id string) (bool, time.Time, error) {
 	now := time.Now()
 	path := acmeValidationPrefix + id
 
+	if err := updateChallengeStatus(sc, cv, authzPath, auth, challenge, verificationErr); err != nil {
+		return true, now, err
+	}
+
 	if cv.RetryCount > MaxRetryAttempts {
-		err = fmt.Errorf("reached max error attempts for challenge %v: %w", id, err)
+		err := fmt.Errorf("reached max error attempts for challenge %v: %w", id, verificationErr)
 		return ace._verifyChallengeCleanup(sc, err, id)
 	}
 
@@ -449,18 +464,35 @@ func (ace *ACMEChallengeEngine) _verifyChallengeRetry(sc *storageContext, cv *Ch
 
 	json, jsonErr := logical.StorageEntryJSON(path, cv)
 	if jsonErr != nil {
-		return true, now, fmt.Errorf("error persisting updated challenge validation queue entry (error prior to retry, if any: %v): %w", err, jsonErr)
+		return true, now, fmt.Errorf("error persisting updated challenge validation queue entry (error prior to retry, if any: %v): %w", verificationErr, jsonErr)
 	}
 
 	if putErr := sc.Storage.Put(sc.Context, json); putErr != nil {
-		return true, now, fmt.Errorf("error writing updated challenge validation entry (error prior to retry, if any: %v): %w", err, putErr)
+		return true, now, fmt.Errorf("error writing updated challenge validation entry (error prior to retry, if any: %v): %w", verificationErr, putErr)
 	}
 
-	if err != nil {
-		err = fmt.Errorf("retrying validation: %w", err)
+	if verificationErr != nil {
+		verificationErr = fmt.Errorf("retrying validation: %w", verificationErr)
 	}
 
-	return true, cv.RetryAfter, err
+	return true, cv.RetryAfter, verificationErr
+}
+
+func updateChallengeStatus(sc *storageContext, cv *ChallengeValidation, authzPath string, auth *ACMEAuthorization, challenge *ACMEChallenge, verificationErr error) error {
+	if verificationErr != nil {
+		challengeError := TranslateErrorToErrorResponse(verificationErr)
+		challenge.Error = challengeError.MarshalForStorage()
+	}
+
+	if cv.RetryCount > MaxRetryAttempts {
+		challenge.Status = ACMEChallengeInvalid
+		auth.Status = ACMEAuthorizationInvalid
+	}
+
+	if err := saveAuthorizationAtPath(sc, authzPath, auth); err != nil {
+		return fmt.Errorf("error persisting authorization/challenge update: %w", err)
+	}
+	return nil
 }
 
 func (ace *ACMEChallengeEngine) _verifyChallengeCleanup(sc *storageContext, err error, id string) (bool, time.Time, error) {

builtin/logical/pki/acme_errors.go

@@ -111,6 +111,19 @@ type ErrorResponse struct {
 	Subproblems []*ErrorResponse `json:"subproblems"`
 }
 
+func (e *ErrorResponse) MarshalForStorage() map[string]interface{} {
+	subProblems := []map[string]interface{}{}
+	for _, subProblem := range e.Subproblems {
+		subProblems = append(subProblems, subProblem.MarshalForStorage())
+	}
+	return map[string]interface{}{
+		"status":      e.StatusCode,
+		"type":        e.Type,
+		"detail":      e.Detail,
+		"subproblems": subProblems,
+	}
+}
+
 func (e *ErrorResponse) Marshal() (*logical.Response, error) {
 	body, err := json.Marshal(e)
 	if err != nil {
@@ -157,6 +170,12 @@ func TranslateError(given error) (*logical.Response, error) {
 		return logical.RespondWithStatusCode(nil, nil, http.StatusNotFound)
 	}
 
+	body := TranslateErrorToErrorResponse(given)
+
+	return body.Marshal()
+}
+
+func TranslateErrorToErrorResponse(given error) ErrorResponse {
 	// We're multierror aware here: if we're given a list of errors, assume
 	// they're structured so the first error is the outer error and the inner
 	// subproblems are subsequent in the multierror.
@@ -187,6 +206,5 @@ func TranslateError(given error) (*logical.Response, error) {
 
 		body.Subproblems = append(body.Subproblems, &sub)
 	}
-
-	return body.Marshal()
+	return body
 }

builtin/logical/pki/path_acme_order.go

@@ -234,11 +234,9 @@ func (b *backend) acmeFinalizeOrderHandler(ac *acmeContext, _ *logical.Request,
 		return nil, err
 	}
 
-	if order.Status == ACMEOrderPending {
-		// Lets see if we can update our order status to ready if all the authorizations have been completed.
-		if requiredAuthorizationsCompleted(b, ac, uc, order) {
-			order.Status = ACMEOrderReady
-		}
+	order.Status, err = computeOrderStatus(ac, uc, order)
+	if err != nil {
+		return nil, err
 	}
 
 	if order.Status != ACMEOrderReady {
@@ -293,23 +291,62 @@ func (b *backend) acmeFinalizeOrderHandler(ac *acmeContext, _ *logical.Request,
 	return formatOrderResponse(ac, order), nil
 }
 
-func requiredAuthorizationsCompleted(b *backend, ac *acmeContext, uc *jwsCtx, order *acmeOrder) bool {
-	if len(order.AuthorizationIds) == 0 {
-		return false
+func computeOrderStatus(ac *acmeContext, uc *jwsCtx, order *acmeOrder) (ACMEOrderStatusType, error) {
+	// If we reached a final stage, no use computing anything else
+	if order.Status == ACMEOrderInvalid || order.Status == ACMEOrderValid {
+		return order.Status, nil
 	}
 
+	// We aren't in a final state yet, check for expiry
+	if time.Now().After(order.Expires) {
+		return ACMEOrderInvalid, nil
+	}
+
+	// Intermediary steps passed authorizations should short circuit us as well
+	if order.Status == ACMEOrderReady || order.Status == ACMEOrderProcessing {
+		return order.Status, nil
+	}
+
+	// If we have no authorizations attached to the order, nothing to compute either
+	if len(order.AuthorizationIds) == 0 {
+		return ACMEOrderPending, nil
+	}
+
+	anyFailed := false
+	allPassed := true
 	for _, authId := range order.AuthorizationIds {
-		authorization, err := b.acmeState.LoadAuthorization(ac, uc, authId)
+		authorization, err := ac.getAcmeState().LoadAuthorization(ac, uc, authId)
 		if err != nil {
-			return false
+			return order.Status, fmt.Errorf("failed loading authorization: %s: %w", authId, err)
+		}
+
+		if authorization.Status == ACMEAuthorizationPending {
+			allPassed = false
+			continue
 		}
 
 		if authorization.Status != ACMEAuthorizationValid {
-			return false
+			// Per RFC 8555 - 7.1.6. Status Changes
+			// The order also moves to the "invalid" state if it expires or
+			// one of its authorizations enters a final state other than
+			// "valid" ("expired", "revoked", or "deactivated").
+			allPassed = false
+			anyFailed = true
+			break
 		}
 	}
 
-	return true
+	if anyFailed {
+		return ACMEOrderInvalid, nil
+	}
+
+	if allPassed {
+		return ACMEOrderReady, nil
+	}
+
+	// The order has not expired, no authorizations have yet to be marked as failed
+	// nor have we passed them all.
+	return ACMEOrderPending, nil
 }
 
 func validateCsrNotUsingAccountKey(csr *x509.CertificateRequest, uc *jwsCtx) error {
@@ -551,11 +588,9 @@ func (b *backend) acmeGetOrderHandler(ac *acmeContext, _ *logical.Request, field
 		return nil, err
 	}
 
-	if order.Status == ACMEOrderPending {
-		// Lets see if we can update our order status to ready if all the authorizations have been completed.
-		if requiredAuthorizationsCompleted(b, ac, uc, order) {
-			order.Status = ACMEOrderReady
-		}
+	order.Status, err = computeOrderStatus(ac, uc, order)
+	if err != nil {
+		return nil, err
 	}
 
 	// Per RFC 8555 -> 7.1.3.  Order Objects

builtin/logical/pki/path_acme_test.go

@@ -17,11 +17,14 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"os"
 	"path"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/hashicorp/vault/helper/testhelpers"
+
 	"github.com/go-test/deep"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/acme"
@@ -1039,6 +1042,109 @@ func testAcmeCertSignedByCa(t *testing.T, client *api.Client, derCerts [][]byte,
 	}
 }
 
+// TestAcmeValidationError make sure that we properly return errors on validation errors.
+func TestAcmeValidationError(t *testing.T) {
+	cluster, _, _ := setupAcmeBackend(t)
+	defer cluster.Cleanup()
+
+	testCtx := context.Background()
+	baseAcmeURL := "/v1/pki/acme/"
+	accountKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	require.NoError(t, err, "failed creating rsa key")
+
+	acmeClient := getAcmeClientForCluster(t, cluster, baseAcmeURL, accountKey)
+
+	// Create new account
+	t.Logf("Testing register on %s", baseAcmeURL)
+	_, err = acmeClient.Register(testCtx, &acme.Account{}, func(tosURL string) bool { return true })
+	require.NoError(t, err, "failed registering account")
+
+	// Create an order
+	t.Logf("Testing Authorize Order on %s", baseAcmeURL)
+	identifiers := []string{"www.dadgarcorp.com"}
+	order, err := acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{
+		{Type: "dns", Value: identifiers[0]},
+	})
+	require.NoError(t, err, "failed creating order")
+
+	// Load authorizations
+	var authorizations []*acme.Authorization
+	for _, authUrl := range order.AuthzURLs {
+		auth, err := acmeClient.GetAuthorization(testCtx, authUrl)
+		require.NoError(t, err, "failed fetching authorization: %s", authUrl)
+
+		authorizations = append(authorizations, auth)
+	}
+	require.Len(t, authorizations, 1, "expected a certain number of authorizations")
+	require.Len(t, authorizations[0].Challenges, 2, "expected a certain number of challenges associated with authorization")
+
+	acceptedAuth, err := acmeClient.Accept(testCtx, authorizations[0].Challenges[0])
+	require.NoError(t, err, "Should have been allowed to accept challenge 1")
+	require.Equal(t, string(ACMEChallengeProcessing), acceptedAuth.Status)
+
+	_, err = acmeClient.Accept(testCtx, authorizations[0].Challenges[1])
+	require.Error(t, err, "Should have been prevented to accept challenge 2")
+
+	// Make sure our challenge returns errors
+	testhelpers.RetryUntil(t, 30*time.Second, func() error {
+		challenge, err := acmeClient.GetChallenge(testCtx, authorizations[0].Challenges[0].URI)
+		if err != nil {
+			return err
+		}
+
+		if challenge.Error == nil {
+			return fmt.Errorf("no error set in challenge yet")
+		}
+
+		acmeError, ok := challenge.Error.(*acme.Error)
+		if !ok {
+			return fmt.Errorf("unexpected error back: %v", err)
+		}
+
+		if acmeError.ProblemType != "urn:ietf:params:acme:error:incorrectResponse" {
+			return fmt.Errorf("unexpected ACME error back: %v", acmeError)
+		}
+
+		return nil
+	})
+
+	// Make sure our challenge,auth and order status change.
+	// This takes a little too long to run in CI properly, we need the ability to influence
+	// how long the validations take before CI can go wild on this.
+	if os.Getenv("CI") == "" {
+		testhelpers.RetryUntil(t, 10*time.Minute, func() error {
+			challenge, err := acmeClient.GetChallenge(testCtx, authorizations[0].Challenges[0].URI)
+			if err != nil {
+				return fmt.Errorf("failed to load challenge: %w", err)
+			}
+
+			if challenge.Status != string(ACMEChallengeInvalid) {
+				return fmt.Errorf("challenge state was not changed to invalid: %v", challenge)
+			}
+
+			authz, err := acmeClient.GetAuthorization(testCtx, authorizations[0].URI)
+			if err != nil {
+				return fmt.Errorf("failed to load authorization: %w", err)
+			}
+
+			if authz.Status != string(ACMEAuthorizationInvalid) {
+				return fmt.Errorf("authz state was not changed to invalid: %v", authz)
+			}
+
+			myOrder, err := acmeClient.GetOrder(testCtx, order.URI)
+			if err != nil {
+				return fmt.Errorf("failed to load order: %w", err)
+			}
+
+			if myOrder.Status != string(ACMEOrderInvalid) {
+				return fmt.Errorf("order state was not changed to invalid: %v", order)
+			}
+
+			return nil
+		})
+	}
+}
+
 func setupTestPkiCluster(t *testing.T) (*vault.TestCluster, *api.Client) {
 	coreConfig := &vault.CoreConfig{
 		LogicalBackends: map[string]logical.Factory{