Skip to content

Commit

Permalink
va: return error instead of ProblemDetails (#7313)
Browse files Browse the repository at this point in the history 
This allows us to defer creating the user-friendly ProblemDetails to the
highest level (va.PerformValidation), which in turn makes it possible to
log the original error alongside the user-friendly error. It also
reduces the likelihood of "boxed nil" bugs.

Many of the unittests check for a specific ProblemDetails.Type and
specific Details contents. These test against the output of
`detailedError`, which transforms `error` into `ProblemDetails`. So the
updates to the tests include insertion of `detailedError(err)` in many
places.

Several places that were returning a specific ProblemDetails.Type
instead return the corresponding `berrors` type. This follows a pattern
that `berrors` was designed to enable: use the `berrors` types
internally and transform into `ProblemDetails` at the edge where we are
rendering something to present to the user: WFE, and now VA.
  • Loading branch information
@jsha
jsha authored Feb 12, 2024
1 parent 8ede0e9 commit 3865b46
Show file tree
Hide file tree
Showing 10 changed files with 239 additions and 204 deletions.
16 changes: 11 additions & 5 deletions va/caa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,11 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC
}

checkResult := "success"
prob := va.checkCAA(ctx, acmeID, params)
err := va.checkCAA(ctx, acmeID, params)
localCheckLatency := time.Since(checkStartTime)
if prob != nil {
var prob *probs.ProblemDetails
if err != nil {
prob = detailedError(err)
logEvent.Error = prob.Error()
prob.Detail = fmt.Sprintf("While processing CAA for %s: %s", req.Domain, prob.Detail)
checkResult = "failure"
Expand Down Expand Up @@ -116,6 +118,10 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC
va.log.AuditObject("CAA check result", logEvent)

if prob != nil {
// The ProblemDetails will be serialized through gRPC, which requires UTF-8.
// It will also later be serialized in JSON, which defaults to UTF-8. Make
// sure it is UTF-8 clean now.
prob = filterProblemDetails(prob)
return &vapb.IsCAAValidResponse{Problem: &corepb.ProblemDetails{
ProblemType: string(prob.Type),
Detail: replaceInvalidUTF8([]byte(prob.Detail)),
Expand Down Expand Up @@ -273,20 +279,20 @@ func (va *ValidationAuthorityImpl) performRemoteCAACheck(
func (va *ValidationAuthorityImpl) checkCAA(
ctx context.Context,
identifier identifier.ACMEIdentifier,
params *caaParams) *probs.ProblemDetails {
params *caaParams) error {
if core.IsAnyNilOrZero(params, params.validationMethod, params.accountURIID) {
return probs.ServerInternal("expected validationMethod or accountURIID not provided to checkCAA")
}

foundAt, valid, response, err := va.checkCAARecords(ctx, identifier, params)
if err != nil {
return probs.DNS(err.Error())
return berrors.DNSError("%s", err)
}

va.log.AuditInfof("Checked CAA records for %s, [Present: %t, Account ID: %d, Challenge: %s, Valid for issuance: %t, Found at: %q] Response=%q",
identifier.Value, foundAt != "", params.accountURIID, params.validationMethod, valid, foundAt, response)
if !valid {
return probs.CAA(fmt.Sprintf("CAA record for %s prevents issuance", foundAt))
return berrors.CAAError("CAA record for %s prevents issuance", foundAt)
}
return nil
}
Expand Down
22 changes: 9 additions & 13 deletions va/caa_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/letsencrypt/boulder/bdns"
"github.com/letsencrypt/boulder/core"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/features"
"github.com/letsencrypt/boulder/identifier"
"github.com/letsencrypt/boulder/probs"
Expand Down Expand Up @@ -197,14 +198,8 @@ func TestCAATimeout(t *testing.T) {
}

err := va.checkCAA(ctx, identifier.DNSIdentifier("caa-timeout.com"), params)
if err.Type != probs.DNSProblem {
t.Errorf("Expected timeout error type %s, got %s", probs.DNSProblem, err.Type)
}

expected := "error"
if err.Detail != expected {
t.Errorf("checkCAA: got %#v, expected %#v", err.Detail, expected)
}
test.AssertErrorIs(t, err, berrors.DNS)
test.AssertContains(t, err.Error(), "error")
}

func TestCAAChecking(t *testing.T) {
Expand Down Expand Up @@ -970,16 +965,17 @@ func TestCAAFailure(t *testing.T) {

va, _ := setup(hs, 0, "", nil, caaMockDNS{})

_, prob := va.validate(ctx, dnsi("reserved.com"), 1, chall)
if prob == nil {
_, err := va.validate(ctx, dnsi("reserved.com"), 1, chall)
if err == nil {
t.Fatalf("Expected CAA rejection for reserved.com, got success")
}
test.AssertEquals(t, prob.Type, probs.CAAProblem)
test.AssertErrorIs(t, err, berrors.CAA)

_, prob = va.validate(ctx, dnsi("example.gonetld"), 1, chall)
if prob == nil {
_, err = va.validate(ctx, dnsi("example.gonetld"), 1, chall)
if err == nil {
t.Fatalf("Expected CAA rejection for gonetld, got success")
}
prob := detailedError(err)
test.AssertEquals(t, prob.Type, probs.DNSProblem)
test.AssertContains(t, prob.Error(), "NXDOMAIN")
}
Expand Down
13 changes: 6 additions & 7 deletions va/dns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/letsencrypt/boulder/core"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/identifier"
"github.com/letsencrypt/boulder/probs"
)

// getAddr will query for all A/AAAA records associated with hostname and return
Expand Down Expand Up @@ -49,10 +48,10 @@ func availableAddresses(allAddrs []net.IP) (v4 []net.IP, v6 []net.IP) {
return
}

func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) {
func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, error) {
if ident.Type != identifier.DNS {
va.log.Infof("Identifier type for DNS challenge was not DNS: %s", ident)
return nil, probs.Malformed("Identifier type for DNS was not itself DNS")
return nil, berrors.MalformedError("Identifier type for DNS was not itself DNS")
}

// Compute the digest of the key authorization file
Expand All @@ -64,14 +63,14 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident iden
challengeSubdomain := fmt.Sprintf("%s.%s", core.DNSPrefix, ident.Value)
txts, resolvers, err := va.dnsClient.LookupTXT(ctx, challengeSubdomain)
if err != nil {
return nil, probs.DNS(err.Error())
return nil, berrors.DNSError("%s", err)
}

// If there weren't any TXT records return a distinct error message to allow
// troubleshooters to differentiate between no TXT records and
// invalid/incorrect TXT records.
if len(txts) == 0 {
return nil, probs.Unauthorized(fmt.Sprintf("No TXT record found at %s", challengeSubdomain))
return nil, berrors.UnauthorizedError("No TXT record found at %s", challengeSubdomain)
}

for _, element := range txts {
Expand All @@ -89,6 +88,6 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident iden
if len(txts) > 1 {
andMore = fmt.Sprintf(" (and %d more)", len(txts)-1)
}
return nil, probs.Unauthorized(fmt.Sprintf("Incorrect TXT record %q%s found at %s",
invalidRecord, andMore, challengeSubdomain))
return nil, berrors.UnauthorizedError("Incorrect TXT record %q%s found at %s",
invalidRecord, andMore, challengeSubdomain)
}
37 changes: 23 additions & 14 deletions va/dns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,37 +41,41 @@ func TestDNSValidationEmpty(t *testing.T) {

func TestDNSValidationWrong(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
_, prob := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), dnsChallenge())
if prob == nil {
_, err := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), dnsChallenge())
if err == nil {
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")
}

func TestDNSValidationWrongMany(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), dnsChallenge())
if prob == nil {
_, err := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), dnsChallenge())
if err == nil {
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")
}

func TestDNSValidationWrongLong(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), dnsChallenge())
if prob == nil {
_, err := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), dnsChallenge())
if err == nil {
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")
}

func TestDNSValidationFailure(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateDNS01(ctx, dnsi("localhost"), dnsChallenge())
_, err := va.validateDNS01(ctx, dnsi("localhost"), dnsChallenge())
prob := detailedError(err)

test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem)
}
Expand All @@ -84,7 +88,8 @@ func TestDNSValidationInvalid(t *testing.T) {

va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateDNS01(ctx, notDNS, dnsChallenge())
_, err := va.validateDNS01(ctx, notDNS, dnsChallenge())
prob := detailedError(err)

test.AssertEquals(t, prob.Type, probs.MalformedProblem)
}
Expand All @@ -94,7 +99,8 @@ func TestDNSValidationNotSane(t *testing.T) {

chall := dnsChallenge()
chall.Token = ""
_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall)
_, err := va.validateChallenge(ctx, dnsi("localhost"), chall)
prob := detailedError(err)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -104,7 +110,8 @@ func TestDNSValidationNotSane(t *testing.T) {
}

chall.Token = "yfCBb-bRTLz8Wd1C0lTUQK3qlKj3-t2tYGwx5Hj7r_"
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall)
_, err = va.validateChallenge(ctx, dnsi("localhost"), chall)
prob = detailedError(err)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -114,7 +121,8 @@ func TestDNSValidationNotSane(t *testing.T) {
}

chall.ProvidedKeyAuthorization = "a"
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall)
_, err = va.validateChallenge(ctx, dnsi("localhost"), chall)
prob = detailedError(err)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -128,8 +136,9 @@ func TestDNSValidationNotSane(t *testing.T) {
func TestDNSValidationServFail(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("servfail.com"), dnsChallenge())
_, err := va.validateChallenge(ctx, dnsi("servfail.com"), dnsChallenge())

prob := detailedError(err)
test.AssertEquals(t, prob.Type, probs.DNSProblem)
}

Expand All @@ -147,8 +156,8 @@ func TestDNSValidationNoServer(t *testing.T) {
log,
nil)

_, prob := va.validateChallenge(ctx, dnsi("localhost"), dnsChallenge())

_, err = va.validateChallenge(ctx, dnsi("localhost"), dnsChallenge())
prob := detailedError(err)
test.AssertEquals(t, prob.Type, probs.DNSProblem)
}

Expand Down
22 changes: 10 additions & 12 deletions va/http.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/iana"
"github.com/letsencrypt/boulder/identifier"
"github.com/letsencrypt/boulder/probs"
)

const (
Expand Down Expand Up @@ -395,11 +394,10 @@ func (va *ValidationAuthorityImpl) setupHTTPValidation(
func (va *ValidationAuthorityImpl) fetchHTTP(
ctx context.Context,
host string,
path string) ([]byte, []core.ValidationRecord, *probs.ProblemDetails) {
path string) ([]byte, []core.ValidationRecord, error) {
body, records, err := va.processHTTPValidation(ctx, host, path)
if err != nil {
// Use detailedError to convert the error into a problem
return body, records, detailedError(err)
return body, records, err
}
return body, records, nil
}
Expand Down Expand Up @@ -653,24 +651,24 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
return body, records, nil
}

func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) {
func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, error) {
if ident.Type != identifier.DNS {
va.log.Infof("Got non-DNS identifier for HTTP validation: %s", ident)
return nil, probs.Malformed("Identifier type for HTTP validation was not DNS")
return nil, berrors.MalformedError("Identifier type for HTTP validation was not DNS")
}

// Perform the fetch
path := fmt.Sprintf(".well-known/acme-challenge/%s", challenge.Token)
body, validationRecords, prob := va.fetchHTTP(ctx, ident.Value, "/"+path)
if prob != nil {
return validationRecords, prob
body, validationRecords, err := va.fetchHTTP(ctx, ident.Value, "/"+path)
if err != nil {
return validationRecords, err
}
payload := strings.TrimRightFunc(string(body), unicode.IsSpace)

if payload != challenge.ProvidedKeyAuthorization {
problem := probs.Unauthorized(fmt.Sprintf("The key authorization file from the server did not match this challenge. Expected %q (got %q)",
challenge.ProvidedKeyAuthorization, payload))
va.log.Infof("%s for %s", problem.Detail, ident)
problem := berrors.UnauthorizedError("The key authorization file from the server did not match this challenge. Expected %q (got %q)",
challenge.ProvidedKeyAuthorization, payload)
va.log.Infof("%s for %s", problem, ident)
return validationRecords, problem
}

Expand Down
Loading

0 comments on commit 3865b46

Please sign in to comment.