letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

ProblemDetails no longer implements Error (#8078) 

Remove the .Error() method from probs.ProblemDetails, so that it can no
longer be returned from functions which return an error. Update various
call sites to use the .String() method to get a textual representation
of the problem instead. Simplify ProblemDetailsForError to not
special-case and pass-through ProblemDetails, since they are no longer a
valid input to that function.

This reduces instances of "boxed nil" bugs, and paves the way for all of
the WFE methods to be refactored to simply return errors instead of
writing them directly into the response object.

Part of https://github.com/letsencrypt/boulder/issues/4980
This commit is contained in:
Aaron Gable 2025-03-28 13:36:26 -05:00 committed by GitHub
parent 082142867d
commit 2c28c4799c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 92 additions and 94 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

91
errors/errors.go
View File

@ -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...)
}

22
probs/probs.go
View File

@ -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 {

2
probs/probs_test.go
View File

@ -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) {

2
ra/ra_test.go
View File

@ -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

5
va/caa.go
View File

@ -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)

2
va/caa_test.go
View File

@ -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) {

6
va/dns_test.go
View File

@ -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) {

8
va/tlsalpn_test.go
View File

@ -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

2
va/va.go
View File

@ -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 {

16
web/probs.go
View File

@ -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.

10
web/probs_test.go
View File

@ -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) {

18
wfe2/wfe.go
View File

@ -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")
}

2
wfe2/wfe_test.go
View File

@ -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 {