diff --git a/errors/errors.go b/errors/errors.go index 37fc84e24..cc1790362 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -1,13 +1,23 @@ -// Package errors provides internal-facing error types for use in Boulder. Many -// of these are transformed directly into Problem Details documents by the WFE. -// Some, like NotFound, may be handled internally. We avoid using Problem -// Details documents as part of our internal error system to avoid layering -// confusions. +// Package errors provide a special error type for use in Boulder. This error +// type carries additional type information with it, and has two special powers: // -// These errors are specifically for use in errors that cross RPC boundaries. -// An error type that does not need to be passed through an RPC can use a plain -// Go type locally. Our gRPC code is aware of these error types and will -// serialize and deserialize them automatically. +// 1. It is recognized by our gRPC code, and the type metadata and detail string +// will cross gRPC boundaries intact. +// +// 2. It is recognized by our frontend API "rendering" code, and will be +// automatically converted to the corresponding urn:ietf:params:acme:error:... +// ACME Problem Document. +// +// This means that a deeply-nested service (such as the SA) that wants to ensure +// that the ACME client sees a particular problem document (such as NotFound) +// can return a BoulderError and be sure that it will be propagated all the way +// to the client. +// +// Note, however, that any additional context wrapped *around* the BoulderError +// (such as by fmt.Errorf("oops: %w")) will be lost when the error is converted +// into a problem document. Similarly, any type information wrapped *by* a +// BoulderError (such as a sql.ErrNoRows) is lost at the gRPC serialization +// boundary. package errors import ( @@ -85,10 +95,15 @@ type SubBoulderError struct { Identifier identifier.ACMEIdentifier } +// Error implements the error interface, returning a string representation of +// this error. func (be *BoulderError) Error() string { return be.Detail } +// Unwrap implements the optional error-unwrapping interface. It returns the +// underlying type, all of when themselves implement the error interface, so +// that `if errors.Is(someError, berrors.Malformed)` works. func (be *BoulderError) Unwrap() error { return be.Type } @@ -164,30 +179,30 @@ func New(errType ErrorType, msg string) error { // newf is a convenience function for creating a new BoulderError with a // formatted message. -func newf(errType ErrorType, msg string, args ...interface{}) error { +func newf(errType ErrorType, msg string, args ...any) error { return &BoulderError{ Type: errType, Detail: fmt.Sprintf(msg, args...), } } -func InternalServerError(msg string, args ...interface{}) error { +func InternalServerError(msg string, args ...any) error { return newf(InternalServer, msg, args...) } -func MalformedError(msg string, args ...interface{}) error { +func MalformedError(msg string, args ...any) error { return newf(Malformed, msg, args...) } -func UnauthorizedError(msg string, args ...interface{}) error { +func UnauthorizedError(msg string, args ...any) error { return newf(Unauthorized, msg, args...) } -func NotFoundError(msg string, args ...interface{}) error { +func NotFoundError(msg string, args ...any) error { return newf(NotFound, msg, args...) } -func RateLimitError(retryAfter time.Duration, msg string, args ...interface{}) error { +func RateLimitError(retryAfter time.Duration, msg string, args ...any) error { return &BoulderError{ Type: RateLimit, Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/", args...), @@ -195,7 +210,7 @@ func RateLimitError(retryAfter time.Duration, msg string, args ...interface{}) e } } -func RegistrationsPerIPAddressError(retryAfter time.Duration, msg string, args ...interface{}) error { +func RegistrationsPerIPAddressError(retryAfter time.Duration, msg string, args ...any) error { return &BoulderError{ Type: RateLimit, Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/#new-registrations-per-ip-address", args...), @@ -203,7 +218,7 @@ func RegistrationsPerIPAddressError(retryAfter time.Duration, msg string, args . } } -func RegistrationsPerIPv6RangeError(retryAfter time.Duration, msg string, args ...interface{}) error { +func RegistrationsPerIPv6RangeError(retryAfter time.Duration, msg string, args ...any) error { return &BoulderError{ Type: RateLimit, Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/#new-registrations-per-ipv6-range", args...), @@ -211,7 +226,7 @@ func RegistrationsPerIPv6RangeError(retryAfter time.Duration, msg string, args . } } -func NewOrdersPerAccountError(retryAfter time.Duration, msg string, args ...interface{}) error { +func NewOrdersPerAccountError(retryAfter time.Duration, msg string, args ...any) error { return &BoulderError{ Type: RateLimit, Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/#new-orders-per-account", args...), @@ -219,7 +234,7 @@ func NewOrdersPerAccountError(retryAfter time.Duration, msg string, args ...inte } } -func CertificatesPerDomainError(retryAfter time.Duration, msg string, args ...interface{}) error { +func CertificatesPerDomainError(retryAfter time.Duration, msg string, args ...any) error { return &BoulderError{ Type: RateLimit, Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/#new-certificates-per-registered-domain", args...), @@ -227,7 +242,7 @@ func CertificatesPerDomainError(retryAfter time.Duration, msg string, args ...in } } -func CertificatesPerFQDNSetError(retryAfter time.Duration, msg string, args ...interface{}) error { +func CertificatesPerFQDNSetError(retryAfter time.Duration, msg string, args ...any) error { return &BoulderError{ Type: RateLimit, Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/#new-certificates-per-exact-set-of-hostnames", args...), @@ -235,7 +250,7 @@ func CertificatesPerFQDNSetError(retryAfter time.Duration, msg string, args ...i } } -func FailedAuthorizationsPerDomainPerAccountError(retryAfter time.Duration, msg string, args ...interface{}) error { +func FailedAuthorizationsPerDomainPerAccountError(retryAfter time.Duration, msg string, args ...any) error { return &BoulderError{ Type: RateLimit, Detail: fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account", args...), @@ -243,55 +258,55 @@ func FailedAuthorizationsPerDomainPerAccountError(retryAfter time.Duration, msg } } -func RejectedIdentifierError(msg string, args ...interface{}) error { +func RejectedIdentifierError(msg string, args ...any) error { return newf(RejectedIdentifier, msg, args...) } -func InvalidEmailError(msg string, args ...interface{}) error { +func InvalidEmailError(msg string, args ...any) error { return newf(InvalidEmail, msg, args...) } -func UnsupportedContactError(msg string, args ...interface{}) error { +func UnsupportedContactError(msg string, args ...any) error { return newf(UnsupportedContact, msg, args...) } -func ConnectionFailureError(msg string, args ...interface{}) error { +func ConnectionFailureError(msg string, args ...any) error { return newf(ConnectionFailure, msg, args...) } -func CAAError(msg string, args ...interface{}) error { +func CAAError(msg string, args ...any) error { return newf(CAA, msg, args...) } -func MissingSCTsError(msg string, args ...interface{}) error { +func MissingSCTsError(msg string, args ...any) error { return newf(MissingSCTs, msg, args...) } -func DuplicateError(msg string, args ...interface{}) error { +func DuplicateError(msg string, args ...any) error { return newf(Duplicate, msg, args...) } -func OrderNotReadyError(msg string, args ...interface{}) error { +func OrderNotReadyError(msg string, args ...any) error { return newf(OrderNotReady, msg, args...) } -func DNSError(msg string, args ...interface{}) error { +func DNSError(msg string, args ...any) error { return newf(DNS, msg, args...) } -func BadPublicKeyError(msg string, args ...interface{}) error { +func BadPublicKeyError(msg string, args ...any) error { return newf(BadPublicKey, msg, args...) } -func BadCSRError(msg string, args ...interface{}) error { +func BadCSRError(msg string, args ...any) error { return newf(BadCSR, msg, args...) } -func AlreadyReplacedError(msg string, args ...interface{}) error { +func AlreadyReplacedError(msg string, args ...any) error { return newf(AlreadyReplaced, msg, args...) } -func AlreadyRevokedError(msg string, args ...interface{}) error { +func AlreadyRevokedError(msg string, args ...any) error { return newf(AlreadyRevoked, msg, args...) } @@ -303,18 +318,18 @@ func UnknownSerialError() error { return newf(UnknownSerial, "unknown serial") } -func InvalidProfileError(msg string, args ...interface{}) error { +func InvalidProfileError(msg string, args ...any) error { return newf(InvalidProfile, msg, args...) } -func BadSignatureAlgorithmError(msg string, args ...interface{}) error { +func BadSignatureAlgorithmError(msg string, args ...any) error { return newf(BadSignatureAlgorithm, msg, args...) } -func AccountDoesNotExistError(msg string, args ...interface{}) error { +func AccountDoesNotExistError(msg string, args ...any) error { return newf(AccountDoesNotExist, msg, args...) } -func BadNonceError(msg string, args ...interface{}) error { +func BadNonceError(msg string, args ...any) error { return newf(BadNonce, msg, args...) } diff --git a/probs/probs.go b/probs/probs.go index 2d1970c05..ae225e1e2 100644 --- a/probs/probs.go +++ b/probs/probs.go @@ -70,7 +70,7 @@ type SubProblemDetails struct { Identifier identifier.ACMEIdentifier `json:"identifier"` } -func (pd *ProblemDetails) Error() string { +func (pd *ProblemDetails) String() string { return fmt.Sprintf("%s :: %s", pd.Type, pd.Detail) } @@ -329,26 +329,6 @@ func Conflict(detail string) *ProblemDetails { } } -// ContentLengthRequired returns a ProblemDetails representing a missing -// Content-Length header error -func ContentLengthRequired() *ProblemDetails { - return &ProblemDetails{ - Type: MalformedProblem, - Detail: "missing Content-Length header", - HTTPStatus: http.StatusLengthRequired, - } -} - -// InvalidContentType returns a ProblemDetails suitable for a missing -// ContentType header, or an incorrect ContentType header -func InvalidContentType(detail string) *ProblemDetails { - return &ProblemDetails{ - Type: MalformedProblem, - Detail: detail, - HTTPStatus: http.StatusUnsupportedMediaType, - } -} - // MethodNotAllowed returns a ProblemDetails representing a disallowed HTTP // method error. func MethodNotAllowed() *ProblemDetails { diff --git a/probs/probs_test.go b/probs/probs_test.go index deadb6714..ceefdfc64 100644 --- a/probs/probs_test.go +++ b/probs/probs_test.go @@ -15,7 +15,7 @@ func TestProblemDetails(t *testing.T) { Detail: "Wat? o.O", HTTPStatus: 403, } - test.AssertEquals(t, pd.Error(), "malformed :: Wat? o.O") + test.AssertEquals(t, pd.String(), "malformed :: Wat? o.O") } func TestProblemDetailsConvenience(t *testing.T) { diff --git a/ra/ra_test.go b/ra/ra_test.go index 8101275eb..b63662313 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -851,7 +851,7 @@ func TestPerformValidationVAError(t *testing.T) { challenge, err := bgrpc.PBToChallenge(dbAuthzPB.Challenges[challIdx]) test.AssertNotError(t, err, "Failed to marshall corepb.Challenge to core.Challenge.") test.Assert(t, challenge.Status == core.StatusInvalid, "challenge was not marked as invalid") - test.AssertContains(t, challenge.Error.Error(), "Could not communicate with VA") + test.AssertContains(t, challenge.Error.String(), "Could not communicate with VA") test.Assert(t, challenge.ValidationRecord == nil, "challenge had a ValidationRecord") // Check that validated timestamp was recorded, stored, and retrieved diff --git a/va/caa.go b/va/caa.go index 893268225..49db4fa66 100644 --- a/va/caa.go +++ b/va/caa.go @@ -2,6 +2,7 @@ package va import ( "context" + "errors" "fmt" "net/url" "regexp" @@ -70,7 +71,7 @@ func (va *ValidationAuthorityImpl) DoCAA(ctx context.Context, req *vapb.IsCAAVal if prob != nil { // CAA check failed. probType = string(prob.Type) - logEvent.Error = prob.Error() + logEvent.Error = prob.String() } else { // CAA check passed. outcome = pass @@ -145,7 +146,7 @@ func (va *ValidationAuthorityImpl) checkCAA( identifier identifier.ACMEIdentifier, params *caaParams) error { if core.IsAnyNilOrZero(params, params.validationMethod, params.accountURIID) { - return probs.ServerInternal("expected validationMethod or accountURIID not provided to checkCAA") + return errors.New("expected validationMethod or accountURIID not provided to checkCAA") } foundAt, valid, response, err := va.checkCAARecords(ctx, identifier, params) diff --git a/va/caa_test.go b/va/caa_test.go index a0e79c6ba..7a0ab08b5 100644 --- a/va/caa_test.go +++ b/va/caa_test.go @@ -1139,7 +1139,7 @@ func TestCAAFailure(t *testing.T) { } prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.DNSProblem) - test.AssertContains(t, prob.Error(), "NXDOMAIN") + test.AssertContains(t, prob.String(), "NXDOMAIN") } func TestFilterCAA(t *testing.T) { diff --git a/va/dns_test.go b/va/dns_test.go index 804dc1b66..b12e2eb7b 100644 --- a/va/dns_test.go +++ b/va/dns_test.go @@ -24,7 +24,7 @@ func TestDNSValidationWrong(t *testing.T) { t.Fatalf("Successful DNS validation with wrong TXT record") } prob := detailedError(err) - test.AssertEquals(t, prob.Error(), "unauthorized :: Incorrect TXT record \"a\" found at _acme-challenge.wrong-dns01.com") + test.AssertEquals(t, prob.String(), "unauthorized :: Incorrect TXT record \"a\" found at _acme-challenge.wrong-dns01.com") } func TestDNSValidationWrongMany(t *testing.T) { @@ -35,7 +35,7 @@ func TestDNSValidationWrongMany(t *testing.T) { t.Fatalf("Successful DNS validation with wrong TXT record") } prob := detailedError(err) - test.AssertEquals(t, prob.Error(), "unauthorized :: Incorrect TXT record \"a\" (and 4 more) found at _acme-challenge.wrong-many-dns01.com") + test.AssertEquals(t, prob.String(), "unauthorized :: Incorrect TXT record \"a\" (and 4 more) found at _acme-challenge.wrong-many-dns01.com") } func TestDNSValidationWrongLong(t *testing.T) { @@ -46,7 +46,7 @@ func TestDNSValidationWrongLong(t *testing.T) { t.Fatalf("Successful DNS validation with wrong TXT record") } prob := detailedError(err) - test.AssertEquals(t, prob.Error(), "unauthorized :: Incorrect TXT record \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...\" found at _acme-challenge.long-dns01.com") + test.AssertEquals(t, prob.String(), "unauthorized :: Incorrect TXT record \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...\" found at _acme-challenge.long-dns01.com") } func TestDNSValidationFailure(t *testing.T) { diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 34081394f..5466c55f2 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -254,7 +254,7 @@ func TestTLSALPN01TalkingToHTTP(t *testing.T) { test.AssertError(t, err, "TLS-SNI-01 validation passed when talking to a HTTP-only server") prob := detailedError(err) expected := "Server only speaks HTTP, not TLS" - if !strings.HasSuffix(prob.Error(), expected) { + if !strings.HasSuffix(prob.String(), expected) { t.Errorf("Got wrong error detail. Expected %q, got %q", expected, prob) } } @@ -780,7 +780,7 @@ func TestTLSALPN01ExtraSANs(t *testing.T) { // In go >= 1.19, the TLS client library detects that the certificate has // a duplicate extension and terminates the connection itself. prob := detailedError(err) - test.AssertContains(t, prob.Error(), "Error getting validation data") + test.AssertContains(t, prob.String(), "Error getting validation data") } func TestTLSALPN01ExtraAcmeExtensions(t *testing.T) { @@ -795,7 +795,7 @@ func TestTLSALPN01ExtraAcmeExtensions(t *testing.T) { // In go >= 1.19, the TLS client library detects that the certificate has // a duplicate extension and terminates the connection itself. prob := detailedError(err) - test.AssertContains(t, prob.Error(), "Error getting validation data") + test.AssertContains(t, prob.String(), "Error getting validation data") } func TestAcceptableExtensions(t *testing.T) { @@ -857,7 +857,7 @@ func TestTLSALPN01BadIdentifier(t *testing.T) { _, err := va.validateTLSALPN01(ctx, identifier.ACMEIdentifier{Type: "smime", Value: "dobber@bad.horse"}, expectedKeyAuthorization) test.AssertError(t, err, "Server accepted a hypothetical S/MIME identifier") prob := detailedError(err) - test.AssertContains(t, prob.Error(), "Identifier type for TLS-ALPN-01 challenge was not DNS or IP") + test.AssertContains(t, prob.String(), "Identifier type for TLS-ALPN-01 challenge was not DNS or IP") } // TestTLSALPN01ServerName tests compliance with RFC 8737, Sec. 3 (step 3) & RFC diff --git a/va/va.go b/va/va.go index 0a131b3c9..3aaf6ac16 100644 --- a/va/va.go +++ b/va/va.go @@ -703,7 +703,7 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV outcome := fail if prob != nil { probType = string(prob.Type) - logEvent.Error = prob.Error() + logEvent.Error = prob.String() logEvent.Challenge.Error = prob logEvent.Challenge.Status = core.StatusInvalid } else { diff --git a/web/probs.go b/web/probs.go index 4aa9f721d..1f1c9c8a9 100644 --- a/web/probs.go +++ b/web/probs.go @@ -75,17 +75,13 @@ func problemDetailsForBoulderError(err *berrors.BoulderError, msg string) *probs return outProb } -// ProblemDetailsForError turns an error into a ProblemDetails with the special -// case of returning the same error back if its already a ProblemDetails. If the -// error is of an type unknown to ProblemDetailsForError, it will return a -// ServerInternal ProblemDetails. +// ProblemDetailsForError turns an error into a ProblemDetails. If the error is +// of an type unknown to ProblemDetailsForError, it will return a ServerInternal +// ProblemDetails. func ProblemDetailsForError(err error, msg string) *probs.ProblemDetails { - var probsProblemDetails *probs.ProblemDetails - var berrorsBoulderError *berrors.BoulderError - if errors.As(err, &probsProblemDetails) { - return probsProblemDetails - } else if errors.As(err, &berrorsBoulderError) { - return problemDetailsForBoulderError(berrorsBoulderError, msg) + var bErr *berrors.BoulderError + if errors.As(err, &bErr) { + return problemDetailsForBoulderError(bErr, msg) } else { // Internal server error messages may include sensitive data, so we do // not include it. diff --git a/web/probs_test.go b/web/probs_test.go index 2475861e9..cd69093d9 100644 --- a/web/probs_test.go +++ b/web/probs_test.go @@ -11,7 +11,7 @@ import ( "github.com/letsencrypt/boulder/test" ) -func TestProblemDetailsFromError(t *testing.T) { +func TestProblemDetailsForError(t *testing.T) { // errMsg is used as the msg argument for `ProblemDetailsForError` and is // always returned in the problem detail. const errMsg = "testError" @@ -50,14 +50,6 @@ func TestProblemDetailsFromError(t *testing.T) { t.Errorf("Expected detailed message %q, got %q", c.detail, p.Detail) } } - - expected := &probs.ProblemDetails{ - Type: probs.MalformedProblem, - HTTPStatus: 200, - Detail: "gotcha", - } - p := ProblemDetailsForError(expected, "k") - test.AssertDeepEquals(t, expected, p) } func TestSubProblems(t *testing.T) { diff --git a/wfe2/wfe.go b/wfe2/wfe.go index c1d65bf5f..71b388f4a 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -592,7 +592,21 @@ func (wfe *WebFrontEndImpl) Nonce( } // sendError wraps web.SendError -func (wfe *WebFrontEndImpl) sendError(response http.ResponseWriter, logEvent *web.RequestEvent, prob *probs.ProblemDetails, ierr error) { +func (wfe *WebFrontEndImpl) sendError(response http.ResponseWriter, logEvent *web.RequestEvent, eerr any, ierr error) { + // TODO(#4980): Simplify this function to only take a single error argument, + // and use web.ProblemDetailsForError to extract the corresponding prob from + // that. For now, though, the third argument has to be `any` so that it can + // be either an error or a problem, and this function can handle either one. + var prob *probs.ProblemDetails + switch v := eerr.(type) { + case *probs.ProblemDetails: + prob = v + case error: + prob = web.ProblemDetailsForError(v, "") + default: + panic(fmt.Sprintf("wfe.sendError got %#v (type %T), but expected ProblemDetails or error", eerr, eerr)) + } + var bErr *berrors.BoulderError if errors.As(ierr, &bErr) { retryAfterSeconds := int(bErr.RetryAfter.Round(time.Second).Seconds()) @@ -998,7 +1012,7 @@ func (wfe *WebFrontEndImpl) revokeCertByCertKey( // certificate by checking that to-be-revoked certificate has the same public // key as the JWK that was used to authenticate the request if !core.KeyDigestEquals(jwk, cert.PublicKey) { - return probs.Unauthorized( + return berrors.UnauthorizedError( "JWK embedded in revocation request must be the same public key as the cert to be revoked") } diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index e1f5ca06e..8b12dc571 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -3364,7 +3364,7 @@ func TestRevokeCertificateWrongCertificateKey(t *testing.T) { makePostRequestWithPath("revoke-cert", jwsBody)) test.AssertEquals(t, responseWriter.Code, 403) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), - `{"type":"`+probs.ErrorNS+`unauthorized","detail":"JWK embedded in revocation request must be the same public key as the cert to be revoked","status":403}`) + `{"type":"`+probs.ErrorNS+`unauthorized","detail":"Unable to revoke :: JWK embedded in revocation request must be the same public key as the cert to be revoked","status":403}`) } type mockSAGetRegByKeyFails struct {