diff --git a/builtin/logical/pki/acme_challenge_engine.go b/builtin/logical/pki/acme_challenge_engine.go index 96dcf7158f..e0f14c08c9 100644 --- a/builtin/logical/pki/acme_challenge_engine.go +++ b/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) { diff --git a/builtin/logical/pki/acme_errors.go b/builtin/logical/pki/acme_errors.go index a9b195ed2d..5b73d9d215 100644 --- a/builtin/logical/pki/acme_errors.go +++ b/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 } diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index c6ae706036..dd09a8e2ab 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/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 diff --git a/builtin/logical/pki/path_acme_test.go b/builtin/logical/pki/path_acme_test.go index cb26bfa5c8..3ec854d5b3 100644 --- a/builtin/logical/pki/path_acme_test.go +++ b/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{