diff --git a/cmd/boulder-va/main.go b/cmd/boulder-va/main.go index b28ec1134ba..495acf82364 100644 --- a/cmd/boulder-va/main.go +++ b/cmd/boulder-va/main.go @@ -125,8 +125,11 @@ func main() { remotes = append( remotes, va.RemoteVA{ - VAClient: vapb.NewVAClient(vaConn), - Address: rva.ServerAddress, + RemoteClients: va.RemoteClients{ + VAClient: vapb.NewVAClient(vaConn), + CAAClient: vapb.NewCAAClient(vaConn), + }, + Address: rva.ServerAddress, }, ) } diff --git a/features/features.go b/features/features.go index 559ac56aee9..ac99e6f59ed 100644 --- a/features/features.go +++ b/features/features.go @@ -87,6 +87,18 @@ type Config struct { // DOH enables DNS-over-HTTPS queries for validation DOH bool + + // EnforceMultiCAA causes the VA to kick off remote CAA rechecks when true. + // When false, no remote CAA rechecks will be performed. The primary VA will + // make a valid/invalid decision with the results. The primary VA will + // return an early decision if MultiCAAFullResults is false. + EnforceMultiCAA bool + + // MultiCAAFullResults will cause the main VA to block and wait for all of + // the remote VA CAA recheck results instead of returning early if the + // number of failures is greater than the configured + // maxRemoteValidationFailures. Only used when EnforceMultiCAA is true. + MultiCAAFullResults bool } var fMu = new(sync.RWMutex) diff --git a/ra/ra.go b/ra/ra.go index 2d214a5501d..7dd80e0ec79 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -878,7 +878,7 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA( return nil } -// recheckCAA accepts a list of of names that need to have their CAA records +// recheckCAA accepts a list of names that need to have their CAA records // rechecked because their associated authorizations are sufficiently old and // performs the CAA checks required for each. If any of the rechecks fail an // error is returned. diff --git a/ra/ra_test.go b/ra/ra_test.go index d12420022cd..7e6b2fcf374 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -157,14 +157,14 @@ func numAuthorizations(o *corepb.Order) int { } type DummyValidationAuthority struct { - request chan *vapb.PerformValidationRequest - ResultError error - ResultReturn *vapb.ValidationResult + performValidationRequest chan *vapb.PerformValidationRequest + PerformValidationRequestResultError error + PerformValidationRequestResultReturn *vapb.ValidationResult } func (dva *DummyValidationAuthority) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest, _ ...grpc.CallOption) (*vapb.ValidationResult, error) { - dva.request <- req - return dva.ResultReturn, dva.ResultError + dva.performValidationRequest <- req + return dva.PerformValidationRequestResultReturn, dva.PerformValidationRequestResultError } var ( @@ -323,7 +323,9 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho saDBCleanUp := test.ResetBoulderTestDatabase(t) - va := &DummyValidationAuthority{request: make(chan *vapb.PerformValidationRequest, 1)} + va := &DummyValidationAuthority{ + performValidationRequest: make(chan *vapb.PerformValidationRequest, 1), + } pa, err := policy.New(map[core.AcmeChallenge]bool{ core.ChallengeTypeHTTP01: true, @@ -818,7 +820,7 @@ func TestPerformValidationAlreadyValid(t *testing.T) { authzPB, err := bgrpc.AuthzToPB(authz) test.AssertNotError(t, err, "bgrpc.AuthzToPB failed") - va.ResultReturn = &vapb.ValidationResult{ + va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), @@ -847,7 +849,7 @@ func TestPerformValidationSuccess(t *testing.T) { // We know this is OK because of TestNewAuthorization authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) - va.ResultReturn = &vapb.ValidationResult{ + va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), @@ -869,7 +871,7 @@ func TestPerformValidationSuccess(t *testing.T) { var vaRequest *vapb.PerformValidationRequest select { - case r := <-va.request: + case r := <-va.performValidationRequest: vaRequest = r case <-time.After(time.Second): t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") @@ -908,7 +910,7 @@ func TestPerformValidationVAError(t *testing.T) { authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) - va.ResultError = fmt.Errorf("Something went wrong") + va.PerformValidationRequestResultError = fmt.Errorf("Something went wrong") challIdx := dnsChallIdx(t, authzPB.Challenges) authzPB, err := ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ @@ -920,7 +922,7 @@ func TestPerformValidationVAError(t *testing.T) { var vaRequest *vapb.PerformValidationRequest select { - case r := <-va.request: + case r := <-va.performValidationRequest: vaRequest = r case <-time.After(time.Second): t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") diff --git a/test/config-next/va.json b/test/config-next/va.json index 1ee699693b6..a6ccfd8c271 100644 --- a/test/config-next/va.json +++ b/test/config-next/va.json @@ -41,6 +41,8 @@ "EnforceMultiVA": true, "MultiVAFullResults": true, "CAAAfterValidation": true, + "EnforceMultiCAA": true, + "MultiCAAFullResults": true, "DOH": true }, "remoteVAs": [ diff --git a/va/caa.go b/va/caa.go index a8fd315e019..a48611be597 100644 --- a/va/caa.go +++ b/va/caa.go @@ -3,18 +3,24 @@ package va import ( "context" "fmt" + "math/rand" "net/url" "regexp" "strings" "sync" + "time" + "github.com/letsencrypt/boulder/canceled" "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" + bgrpc "github.com/letsencrypt/boulder/grpc" "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/probs" vapb "github.com/letsencrypt/boulder/va/proto" "github.com/miekg/dns" + "github.com/prometheus/client_golang/prometheus" ) type caaParams struct { @@ -22,10 +28,19 @@ type caaParams struct { validationMethod core.AcmeChallenge } +// IsCAAValid checks requested CAA records from a VA, and recursively any RVAs +// configured in the VA. It returns a response or an error. func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsCAAValidRequest) (*vapb.IsCAAValidResponse, error) { - if req.Domain == "" || req.ValidationMethod == "" || req.AccountURIID == 0 { + if core.IsAnyNilOrZero(req.Domain, req.ValidationMethod, req.AccountURIID) { return nil, berrors.InternalServerError("incomplete IsCAAValid request") } + logEvent := verificationRequestEvent{ + // TODO(#7061) Plumb req.Authz.Id as "ID:" through from the RA to + // correlate which authz triggered this request. + Requester: req.AccountURIID, + Hostname: req.Domain, + } + checkStartTime := va.clk.Now() validationMethod := core.AcmeChallenge(req.ValidationMethod) if !validationMethod.IsValid() { @@ -40,16 +55,224 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC accountURIID: req.AccountURIID, validationMethod: validationMethod, } - if prob := va.checkCAA(ctx, acmeID, params); prob != nil { - detail := fmt.Sprintf("While processing CAA for %s: %s", req.Domain, prob.Detail) - return &vapb.IsCAAValidResponse{ - Problem: &corepb.ProblemDetails{ - ProblemType: string(prob.Type), - Detail: replaceInvalidUTF8([]byte(detail)), - }, - }, nil + + var remoteCAAResults chan *remoteVAResult + if features.Get().EnforceMultiCAA { + if remoteVACount := len(va.remoteVAs); remoteVACount > 0 { + remoteCAAResults = make(chan *remoteVAResult, remoteVACount) + go va.performRemoteCAACheck(ctx, req, remoteCAAResults) + } + } + + checkResult := "success" + prob := va.checkCAA(ctx, acmeID, params) + localCheckLatency := time.Since(checkStartTime) + if prob != nil { + logEvent.Error = prob.Error() + prob.Detail = fmt.Sprintf("While processing CAA for %s: %s", req.Domain, prob.Detail) + checkResult = "failure" + } else if remoteCAAResults != nil { + if !features.Get().EnforceMultiCAA && features.Get().MultiCAAFullResults { + // If we're not going to enforce multi CAA but we are logging the + // differentials then collect and log the remote results in a separate go + // routine to avoid blocking the primary VA. + go func() { + _ = va.processRemoteCAAResults( + req.Domain, + req.AccountURIID, + string(validationMethod), + prob, + remoteCAAResults) + }() + } else if features.Get().EnforceMultiCAA { + remoteProb := va.processRemoteCAAResults( + req.Domain, + req.AccountURIID, + string(validationMethod), + prob, + remoteCAAResults) + + // If the remote result was a non-nil problem then fail the CAA check + if remoteProb != nil { + prob = remoteProb + logEvent.Error = remoteProb.Error() + checkResult = "failure" + va.log.Infof("CAA check failed due to remote failures: identifier=%v err=%s", + req.Domain, remoteProb) + va.metrics.remoteCAACheckFailures.Inc() + } + } + } + checkLatency := time.Since(checkStartTime) + logEvent.ValidationLatency = checkLatency.Round(time.Millisecond).Seconds() + + va.metrics.localCAACheckTime.With(prometheus.Labels{ + "result": checkResult, + }).Observe(localCheckLatency.Seconds()) + va.metrics.caaCheckTime.With(prometheus.Labels{ + "result": checkResult, + }).Observe(checkLatency.Seconds()) + + va.log.AuditObject("CAA check result", logEvent) + + if prob != nil { + return &vapb.IsCAAValidResponse{Problem: &corepb.ProblemDetails{ + ProblemType: string(prob.Type), + Detail: replaceInvalidUTF8([]byte(prob.Detail)), + }}, nil + } else { + return &vapb.IsCAAValidResponse{}, nil + } +} + +// processRemoteCAAResults evaluates a primary VA result, and a channel of +// remote VA problems to produce a single overall validation result based on +// configured feature flags. The overall result is calculated based on the VA's +// configured `maxRemoteFailures` value. +// +// If the `MultiCAAFullResults` feature is enabled then +// `processRemoteCAAResults` will expect to read a result from the +// `remoteResultsChan` channel for each VA and will not produce an overall +// result until all remote VAs have responded. In this case +// `logRemoteDifferentials` will also be called to describe the differential +// between the primary and all of the remote VAs. +// +// If the `MultiCAAFullResults` feature flag is not enabled then +// `processRemoteCAAResults` will potentially return before all remote VAs have +// had a chance to respond. This happens if the success or failure threshold is +// met. This doesn't allow for logging the differential between the primary and +// remote VAs but is more performant. +func (va *ValidationAuthorityImpl) processRemoteCAAResults( + domain string, + acctID int64, + challengeType string, + primaryResult *probs.ProblemDetails, + remoteResultsChan <-chan *remoteVAResult) *probs.ProblemDetails { + + state := "failure" + start := va.clk.Now() + + defer func() { + va.metrics.remoteCAACheckTime.With(prometheus.Labels{ + "result": state, + }).Observe(va.clk.Since(start).Seconds()) + }() + + required := len(va.remoteVAs) - va.maxRemoteFailures + good := 0 + bad := 0 + + var remoteResults []*remoteVAResult + var firstProb *probs.ProblemDetails + // Due to channel behavior this could block indefinitely and we rely on gRPC + // honoring the context deadline used in client calls to prevent that from + // happening. + for result := range remoteResultsChan { + // Add the result to the slice + remoteResults = append(remoteResults, result) + if result.Problem == nil { + good++ + } else { + bad++ + // Store the first non-nil problem to return later (if `MultiCAAFullResults` + // is enabled). + if firstProb == nil { + firstProb = result.Problem + } + } + + // If MultiCAAFullResults isn't enabled then return early whenever the + // success or failure threshold is met. + if !features.Get().MultiCAAFullResults { + if good >= required { + state = "success" + return nil + } else if bad > va.maxRemoteFailures { + modifiedProblem := *result.Problem + modifiedProblem.Detail = "During secondary CAA checking: " + firstProb.Detail + return &modifiedProblem + } + } + + // If we haven't returned early because of MultiCAAFullResults being + // enabled we need to break the loop once all of the VAs have returned a + // result. + if len(remoteResults) == len(va.remoteVAs) { + break + } + } + // If we are using `features.MultiCAAFullResults` then we haven't returned + // early and can now log the differential between what the primary VA saw and + // what all of the remote VAs saw. + va.logRemoteDifferentials( + domain, + acctID, + challengeType, + primaryResult, + remoteResults) + + // Based on the threshold of good/bad return nil or a problem. + if good >= required { + state = "success" + return nil + } else if bad > va.maxRemoteFailures { + modifiedProblem := *firstProb + modifiedProblem.Detail = "During secondary CAA checking: " + firstProb.Detail + // If the primary result was OK and there were more failures than the allowed + // threshold increment a stat that indicates this overall validation will have + // failed. + if primaryResult == nil { + va.metrics.prospectiveRemoteCAACheckFailures.Inc() + } + return &modifiedProblem + } + + // This condition should not occur - it indicates the good/bad counts didn't + // meet either the required threshold or the maxRemoteFailures threshold. + return probs.ServerInternal("Too few remote IsCAAValid RPC results") +} + +// performRemoteCAACheck calls `isCAAValid` for each of the configured remoteVAs +// in a random order. The provided `results` chan should have an equal size to +// the number of remote VAs. The CAA checks will be performed in separate +// go-routines. If the result `error` from a remote `isCAAValid` RPC is nil or a +// nil `ProblemDetails` instance it is written directly to the `results` chan. +// If the err is a cancelled error it is treated as a nil error. Otherwise the +// error/problem is written to the results channel as-is. +func (va *ValidationAuthorityImpl) performRemoteCAACheck( + ctx context.Context, + req *vapb.IsCAAValidRequest, + results chan<- *remoteVAResult) { + for _, i := range rand.Perm(len(va.remoteVAs)) { + remoteVA := va.remoteVAs[i] + go func(rva RemoteVA) { + result := &remoteVAResult{ + VAHostname: rva.Address, + } + res, err := rva.IsCAAValid(ctx, req) + if err != nil { + if canceled.Is(err) { + // Handle the cancellation error. + result.Problem = probs.ServerInternal("Remote VA IsCAAValid RPC cancelled") + } else { + // Handle validation error. + va.log.Errf("Remote VA %q.IsCAAValid failed: %s", rva.Address, err) + result.Problem = probs.ServerInternal("Remote VA IsCAAValid RPC failed") + } + } else if res.Problem != nil { + prob, err := bgrpc.PBToProblemDetails(res.Problem) + if err != nil { + va.log.Infof("Remote VA %q.IsCAAValid returned malformed problem: %s", rva.Address, err) + result.Problem = probs.ServerInternal( + fmt.Sprintf("Remote VA IsCAAValid RPC returned malformed result: %s", err)) + } else { + va.log.Infof("Remote VA %q.IsCAAValid returned problem: %s", rva.Address, prob) + result.Problem = prob + } + } + results <- result + }(remoteVA) } - return &vapb.IsCAAValidResponse{}, nil } // checkCAA performs a CAA lookup & validation for the provided identifier. If @@ -58,7 +281,7 @@ func (va *ValidationAuthorityImpl) checkCAA( ctx context.Context, identifier identifier.ACMEIdentifier, params *caaParams) *probs.ProblemDetails { - if params == nil || params.validationMethod == "" || params.accountURIID == 0 { + if core.IsAnyNilOrZero(params, params.validationMethod, params.accountURIID) { return probs.ServerInternal("expected validationMethod or accountURIID not provided to checkCAA") } diff --git a/va/caa_test.go b/va/caa_test.go index 639a462e666..dc3c62e4805 100644 --- a/va/caa_test.go +++ b/va/caa_test.go @@ -10,7 +10,9 @@ import ( "github.com/miekg/dns" + "github.com/letsencrypt/boulder/bdns" "github.com/letsencrypt/boulder/core" + "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/probs" "github.com/letsencrypt/boulder/test" @@ -187,8 +189,7 @@ func (mock caaMockDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, } func TestCAATimeout(t *testing.T) { - va, _ := setup(nil, 0, "", nil) - va.dnsClient = caaMockDNS{} + va, _ := setup(nil, 0, "", nil, caaMockDNS{}) params := &caaParams{ accountURIID: 12345, @@ -411,13 +412,12 @@ func TestCAAChecking(t *testing.T) { method := core.ChallengeTypeHTTP01 params := &caaParams{accountURIID: accountURIID, validationMethod: method} - va, _ := setup(nil, 0, "", nil) - va.dnsClient = caaMockDNS{} + va, _ := setup(nil, 0, "", nil, caaMockDNS{}) va.accountURIPrefixes = []string{"https://letsencrypt.org/acct/reg/"} for _, caaTest := range testCases { mockLog := va.log.(*blog.Mock) - mockLog.Clear() + defer mockLog.Clear() t.Run(caaTest.Name, func(t *testing.T) { ident := identifier.DNSIdentifier(caaTest.Domain) foundAt, valid, _, err := va.checkCAARecords(ctx, ident, params) @@ -435,8 +435,7 @@ func TestCAAChecking(t *testing.T) { } func TestCAALogging(t *testing.T) { - va, _ := setup(nil, 0, "", nil) - va.dnsClient = caaMockDNS{} + va, _ := setup(nil, 0, "", nil, caaMockDNS{}) testCases := []struct { Name string @@ -504,7 +503,7 @@ func TestCAALogging(t *testing.T) { for _, tc := range testCases { t.Run(tc.Domain, func(t *testing.T) { mockLog := va.log.(*blog.Mock) - mockLog.Clear() + defer mockLog.Clear() params := &caaParams{ accountURIID: tc.AccountURIID, @@ -526,8 +525,7 @@ func TestCAALogging(t *testing.T) { // TestIsCAAValidErrMessage tests that an error result from `va.IsCAAValid` // includes the domain name that was being checked in the failure detail. func TestIsCAAValidErrMessage(t *testing.T) { - va, _ := setup(nil, 0, "", nil) - va.dnsClient = caaMockDNS{} + va, _ := setup(nil, 0, "", nil, caaMockDNS{}) // Call IsCAAValid with a domain we know fails with a generic error from the // caaMockDNS. @@ -552,8 +550,7 @@ func TestIsCAAValidErrMessage(t *testing.T) { // which do not have the necessary parameters to do CAA Account and Method // Binding checks. func TestIsCAAValidParams(t *testing.T) { - va, _ := setup(nil, 0, "", nil) - va.dnsClient = caaMockDNS{} + va, _ := setup(nil, 0, "", nil, caaMockDNS{}) // Calling IsCAAValid without a ValidationMethod should fail. _, err := va.IsCAAValid(ctx, &vapb.IsCAAValidRequest{ @@ -578,13 +575,400 @@ func TestIsCAAValidParams(t *testing.T) { test.AssertError(t, err, "calling IsCAAValid without an AccountURIID") } +var errCAABrokenDNSClient = errors.New("dnsClient is broken") + +// caaBrokenDNS implements the `dns.DNSClient` interface, but always returns +// errors. +type caaBrokenDNS struct{} + +func (b caaBrokenDNS) LookupTXT(_ context.Context, hostname string) ([]string, error) { + return nil, errCAABrokenDNSClient +} + +func (b caaBrokenDNS) LookupHost(_ context.Context, hostname string) ([]net.IP, error) { + return nil, errCAABrokenDNSClient +} + +func (b caaBrokenDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, string, error) { + return nil, "", errCAABrokenDNSClient +} + +func TestDisabledMultiCAARechecking(t *testing.T) { + brokenRVA := setupRemote(nil, "broken", caaBrokenDNS{}) + remoteVAs := []RemoteVA{{brokenRVA, "broken"}} + va, _ := setup(nil, 0, "local", remoteVAs, nil) + + features.Set(features.Config{ + EnforceMultiCAA: false, + MultiCAAFullResults: false, + }) + defer features.Reset() + + isValidRes, err := va.IsCAAValid(context.TODO(), &vapb.IsCAAValidRequest{ + Domain: "present.com", + ValidationMethod: string(core.ChallengeTypeDNS01), + AccountURIID: 1, + }) + test.AssertNotError(t, err, "Error during IsCAAValid") + // The primary VA can successfully recheck the CAA record and is allowed to + // issue for this domain. If `EnforceMultiCAA`` was enabled, the configured + // remote VA with broken dns.Client would fail the check and return a + // Problem, but that code path could never trigger. + test.AssertBoxedNil(t, isValidRes.Problem, "IsCAAValid returned a problem, but should not have") +} + +// caaHijackedDNS implements the `dns.DNSClient` interface with a set of useful +// test answers for CAA queries. It returns alternate CAA records than what +// caaMockDNS returns simulating either a BGP hijack or DNS records that have +// changed while queries were inflight. +type caaHijackedDNS struct{} + +func (h caaHijackedDNS) LookupTXT(_ context.Context, hostname string) ([]string, error) { + return nil, nil +} + +func (h caaHijackedDNS) LookupHost(_ context.Context, hostname string) ([]net.IP, error) { + ip := net.ParseIP("127.0.0.1") + return []net.IP{ip}, nil +} +func (h caaHijackedDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, string, error) { + // These records are altered from their caaMockDNS counterparts. Use this to + // tickle remoteValidationFailures. + var results []*dns.CAA + var record dns.CAA + switch strings.TrimRight(domain, ".") { + case "present.com", "present.servfail.com": + record.Tag = "issue" + record.Value = "other-ca.com" + results = append(results, &record) + case "present-dns-only.com": + return results, "", fmt.Errorf("SERVFAIL") + case "satisfiable-wildcard.com": + record.Tag = "issuewild" + record.Value = ";" + results = append(results, &record) + secondRecord := record + secondRecord.Tag = "issue" + secondRecord.Value = ";" + results = append(results, &secondRecord) + } + var response string + if len(results) > 0 { + response = "foo" + } + return results, response, nil +} + +func TestMultiCAARechecking(t *testing.T) { + // The remote differential log order is non-deterministic, so let's use + // the same UA for all applicable RVAs. + const ( + localUA = "local" + remoteUA = "remote" + brokenUA = "broken" + hijackedUA = "hijacked" + ) + remoteVA := setupRemote(nil, remoteUA, nil) + brokenVA := setupRemote(nil, brokenUA, caaBrokenDNS{}) + // Returns incorrect results + hijackedVA := setupRemote(nil, hijackedUA, caaHijackedDNS{}) + + testCases := []struct { + name string + maxLookupFailures int + domains string + remoteVAs []RemoteVA + expectedProbSubstring string + expectedProbType probs.ProblemType + expectedDiffLogSubstring string + localDNSClient bdns.Client + }{ + { + name: "all VAs functional, no CAA records", + domains: "present-dns-only.com", + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "broken localVA, RVAs functional, no CAA records", + domains: "present-dns-only.com", + localDNSClient: caaBrokenDNS{}, + expectedProbSubstring: "While processing CAA for present-dns-only.com: dnsClient is broken", + expectedProbType: probs.DNSProblem, + remoteVAs: []RemoteVA{ + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "functional localVA, 1 broken RVA, no CAA records", + domains: "present-dns-only.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.DNSProblem, + expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {brokenVA, brokenUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "functional localVA, all broken RVAs, no CAA records", + domains: "present-dns-only.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.DNSProblem, + expectedDiffLogSubstring: `RemoteSuccesses":0,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {brokenVA, brokenUA}, + {brokenVA, brokenUA}, + {brokenVA, brokenUA}, + }, + }, + { + name: "all VAs functional, CAA issue type present", + domains: "present.com", + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "functional localVA, 1 broken RVA, CAA issue type present", + domains: "present.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.DNSProblem, + expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {brokenVA, brokenUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "functional localVA, all broken RVAs, CAA issue type present", + domains: "present.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.DNSProblem, + expectedDiffLogSubstring: `RemoteSuccesses":0,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {brokenVA, brokenUA}, + {brokenVA, brokenUA}, + {brokenVA, brokenUA}, + }, + }, + { + // The localVA kicks off the background goroutines before doing its + // own check. But if its own check fails, it doesn't wait for their + // results. + name: "all VAs functional, CAA issue type forbids issuance", + domains: "unsatisfiable.com", + expectedProbSubstring: "CAA record for unsatisfiable.com prevents issuance", + expectedProbType: probs.CAAProblem, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "1 hijacked RVA, CAA issue type present", + domains: "present.com", + expectedProbSubstring: "CAA record for present.com prevents issuance", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "2 hijacked RVAs, CAA issue type present", + domains: "present.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "3 hijacked RVAs, CAA issue type present", + domains: "present.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":0,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + }, + }, + { + name: "1 hijacked RVA, CAA issuewild type present", + domains: "satisfiable-wildcard.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "2 hijacked RVAs, CAA issuewild type present", + domains: "satisfiable-wildcard.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "3 hijacked RVAs, CAA issuewild type present", + domains: "satisfiable-wildcard.com", + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":0,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + }, + }, + { + name: "1 hijacked RVA, CAA issuewild type present, 1 failure allowed", + domains: "satisfiable-wildcard.com", + maxLookupFailures: 1, + expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {remoteVA, remoteUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "2 hijacked RVAs, CAA issuewild type present, 1 failure allowed", + domains: "satisfiable-wildcard.com", + maxLookupFailures: 1, + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + {remoteVA, remoteUA}, + }, + }, + { + name: "3 hijacked RVAs, CAA issuewild type present, 1 failure allowed", + domains: "satisfiable-wildcard.com", + maxLookupFailures: 1, + expectedProbSubstring: "During secondary CAA checking: While processing CAA", + expectedProbType: probs.CAAProblem, + expectedDiffLogSubstring: `RemoteSuccesses":0,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`, + localDNSClient: caaMockDNS{}, + remoteVAs: []RemoteVA{ + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + {hijackedVA, hijackedUA}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + va, mockLog := setup(nil, tc.maxLookupFailures, localUA, tc.remoteVAs, tc.localDNSClient) + defer mockLog.Clear() + + // MultiCAAFullResults: false is inherently flaky because of the + // non-deterministic nature of concurrent goroutine returns. We, + // boulder dev, made a decision to skip testing that path and + // eventually make MultiCAAFullResults: true the default. + features.Set(features.Config{ + EnforceMultiCAA: true, + MultiCAAFullResults: true, + }) + defer features.Reset() + + isValidRes, err := va.IsCAAValid(context.TODO(), &vapb.IsCAAValidRequest{ + Domain: tc.domains, + ValidationMethod: string(core.ChallengeTypeDNS01), + AccountURIID: 1, + }) + test.AssertNotError(t, err, "Should not have errored, but did") + + if tc.expectedProbSubstring != "" { + test.AssertContains(t, isValidRes.Problem.Detail, tc.expectedProbSubstring) + } else if isValidRes.Problem != nil { + test.AssertBoxedNil(t, isValidRes.Problem, "IsCAAValidRequest returned a problem, but should not have") + } + + if tc.expectedProbType != "" { + test.AssertEquals(t, string(tc.expectedProbType), isValidRes.Problem.ProblemType) + } + + var invalidRVACount int + for _, x := range va.remoteVAs { + if x.Address == "broken" || x.Address == "hijacked" { + invalidRVACount++ + } + } + + gotRequestProbs := mockLog.GetAllMatching(".IsCAAValid returned problem: ") + test.AssertEquals(t, len(gotRequestProbs), invalidRVACount) + + gotDifferential := mockLog.GetAllMatching("remoteVADifferentials JSON=.*") + if features.Get().MultiCAAFullResults && tc.expectedDiffLogSubstring != "" { + test.AssertEquals(t, len(gotDifferential), 1) + test.AssertContains(t, gotDifferential[0], tc.expectedDiffLogSubstring) + } else { + test.AssertEquals(t, len(gotDifferential), 0) + } + + gotAnyRemoteFailures := mockLog.GetAllMatching("CAA check failed due to remote failures:") + if len(gotAnyRemoteFailures) >= 1 { + // The primary VA only emits this line once. + test.AssertEquals(t, len(gotAnyRemoteFailures), 1) + } else { + test.AssertEquals(t, len(gotAnyRemoteFailures), 0) + } + }) + } +} + func TestCAAFailure(t *testing.T) { chall := createChallenge(core.ChallengeTypeHTTP01) hs := httpSrv(t, chall.Token) defer hs.Close() - va, _ := setup(hs, 0, "", nil) - va.dnsClient = caaMockDNS{} + va, _ := setup(hs, 0, "", nil, caaMockDNS{}) _, prob := va.validate(ctx, dnsi("reserved.com"), 1, chall) if prob == nil { diff --git a/va/dns_test.go b/va/dns_test.go index 704a6bc2c70..c5f7e191916 100644 --- a/va/dns_test.go +++ b/va/dns_test.go @@ -23,7 +23,7 @@ func dnsChallenge() core.Challenge { } func TestDNSValidationEmpty(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) // This test calls PerformValidation directly, because that is where the // metrics checked below are incremented. @@ -40,8 +40,7 @@ func TestDNSValidationEmpty(t *testing.T) { } func TestDNSValidationWrong(t *testing.T) { - va, _ := setup(nil, 0, "", nil) - + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), dnsChallenge()) if prob == nil { t.Fatalf("Successful DNS validation with wrong TXT record") @@ -50,7 +49,7 @@ func TestDNSValidationWrong(t *testing.T) { } func TestDNSValidationWrongMany(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), dnsChallenge()) if prob == nil { @@ -60,7 +59,7 @@ func TestDNSValidationWrongMany(t *testing.T) { } func TestDNSValidationWrongLong(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), dnsChallenge()) if prob == nil { @@ -70,7 +69,7 @@ func TestDNSValidationWrongLong(t *testing.T) { } func TestDNSValidationFailure(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateDNS01(ctx, dnsi("localhost"), dnsChallenge()) @@ -83,7 +82,7 @@ func TestDNSValidationInvalid(t *testing.T) { Value: "790DB180-A274-47A4-855F-31C428CB1072", } - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateDNS01(ctx, notDNS, dnsChallenge()) @@ -91,7 +90,7 @@ func TestDNSValidationInvalid(t *testing.T) { } func TestDNSValidationNotSane(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) chall := dnsChallenge() chall.Token = "" @@ -127,7 +126,7 @@ func TestDNSValidationNotSane(t *testing.T) { } func TestDNSValidationServFail(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("servfail.com"), dnsChallenge()) @@ -135,7 +134,7 @@ func TestDNSValidationServFail(t *testing.T) { } func TestDNSValidationNoServer(t *testing.T) { - va, log := setup(nil, 0, "", nil) + va, log := setup(nil, 0, "", nil, nil) staticProvider, err := bdns.NewStaticProvider([]string{}) test.AssertNotError(t, err, "Couldn't make new static provider") @@ -154,7 +153,7 @@ func TestDNSValidationNoServer(t *testing.T) { } func TestDNSValidationOK(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("good-dns01.com"), dnsChallenge()) @@ -162,7 +161,7 @@ func TestDNSValidationOK(t *testing.T) { } func TestDNSValidationNoAuthorityOK(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("no-authority-dns01.com"), dnsChallenge()) diff --git a/va/http_test.go b/va/http_test.go index 803ab3e4d51..771c703656a 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -60,7 +60,7 @@ func TestDialerMismatchError(t *testing.T) { // will timeout after the expected singleDialTimeout. This ensures timeouts at // the TCP level are handled correctly. func TestPreresolvedDialerTimeout(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) // Timeouts below 50ms tend to be flaky. va.singleDialTimeout = 50 * time.Millisecond @@ -160,7 +160,7 @@ func TestHTTPValidationTarget(t *testing.T) { exampleQuery = "my-path=was&my=own" ) - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { target, err := va.newHTTPValidationTarget( @@ -291,7 +291,7 @@ func TestExtractRequestTarget(t *testing.T) { }, } - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { host, port, err := va.extractRequestTarget(tc.Req) @@ -313,7 +313,7 @@ func TestExtractRequestTarget(t *testing.T) { // generates a DNS error, and checks that a log line with the detailed error is // generated. func TestHTTPValidationDNSError(t *testing.T) { - va, mockLog := setup(nil, 0, "", nil) + va, mockLog := setup(nil, 0, "", nil, nil) _, _, prob := va.fetchHTTP(ctx, "always.error", "/.well-known/acme-challenge/whatever") test.AssertError(t, prob, "Expected validation fetch to fail") @@ -329,7 +329,7 @@ func TestHTTPValidationDNSError(t *testing.T) { // the mock resolver results in valid query/response data being logged in // a format we can decode successfully. func TestHTTPValidationDNSIdMismatchError(t *testing.T) { - va, mockLog := setup(nil, 0, "", nil) + va, mockLog := setup(nil, 0, "", nil, nil) _, _, prob := va.fetchHTTP(ctx, "id.mismatch", "/.well-known/acme-challenge/whatever") test.AssertError(t, prob, "Expected validation fetch to fail") @@ -368,7 +368,7 @@ func TestHTTPValidationDNSIdMismatchError(t *testing.T) { } func TestSetupHTTPValidation(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) mustTarget := func(t *testing.T, host string, port int, path string) *httpValidationTarget { target, err := va.newHTTPValidationTarget( @@ -738,7 +738,7 @@ func TestFetchHTTP(t *testing.T) { // Setup a VA. By providing the testSrv to setup the VA will use the testSrv's // randomly assigned port as its HTTP port. - va, _ := setup(testSrv, 0, "", nil) + va, _ := setup(testSrv, 0, "", nil, nil) // We need to know the randomly assigned HTTP port for testcases as well httpPort := getPort(testSrv) @@ -1190,7 +1190,7 @@ func TestHTTPBadPort(t *testing.T) { hs := httpSrv(t, expectedToken) defer hs.Close() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) // Pick a random port between 40000 and 65000 - with great certainty we won't // have an HTTP server listening on this port and the test will fail as @@ -1216,7 +1216,7 @@ func TestHTTPKeyAuthorizationFileMismatch(t *testing.T) { }) hs.Start() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateHTTP01(ctx, dnsi("localhost.com"), httpChallenge()) if prob == nil { @@ -1239,7 +1239,7 @@ func TestHTTP(t *testing.T) { // TODO(#1989): close hs hs := httpSrv(t, expectedToken) - va, log := setup(hs, 0, "", nil) + va, log := setup(hs, 0, "", nil, nil) chall := httpChallenge() t.Logf("Trying to validate: %+v\n", chall) @@ -1308,7 +1308,7 @@ func TestHTTPTimeout(t *testing.T) { hs := httpSrv(t, expectedToken) // TODO(#1989): close hs - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) chall := httpChallenge() setChallengeToken(&chall, pathWaitLong) @@ -1350,7 +1350,7 @@ func (mock dnsMockReturnsUnroutable) LookupHost(_ context.Context, hostname stri // error when dial fails. We do this by using a mock DNS client that resolves // everything to an unroutable IP address. func TestHTTPDialTimeout(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) started := time.Now() timeout := 250 * time.Millisecond @@ -1395,7 +1395,7 @@ func TestHTTPDialTimeout(t *testing.T) { func TestHTTPRedirectLookup(t *testing.T) { hs := httpSrv(t, expectedToken) defer hs.Close() - va, log := setup(hs, 0, "", nil) + va, log := setup(hs, 0, "", nil, nil) chall := httpChallenge() setChallengeToken(&chall, pathMoved) @@ -1461,7 +1461,7 @@ func TestHTTPRedirectLookup(t *testing.T) { func TestHTTPRedirectLoop(t *testing.T) { hs := httpSrv(t, expectedToken) defer hs.Close() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) chall := httpChallenge() setChallengeToken(&chall, "looper") @@ -1474,7 +1474,7 @@ func TestHTTPRedirectLoop(t *testing.T) { func TestHTTPRedirectUserAgent(t *testing.T) { hs := httpSrv(t, expectedToken) defer hs.Close() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) va.userAgent = rejectUserAgent chall := httpChallenge() @@ -1514,7 +1514,7 @@ func TestValidateHTTP(t *testing.T) { hs := httpSrv(t, chall.Token) defer hs.Close() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) test.Assert(t, prob == nil, "validation failed") @@ -1525,7 +1525,7 @@ func TestLimitedReader(t *testing.T) { setChallengeToken(&chall, core.NewToken()) hs := httpSrv(t, "012345\xff67890123456789012345678901234567890123456789012345678901234567890123456789") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) defer hs.Close() _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 57122b6735a..20cdfe75448 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -141,7 +141,7 @@ func TestTLSALPN01FailIP(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifier, 0, "expected") test.AssertNotError(t, err, "Error creating test server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) port := getPort(hs) _, prob := va.validateTLSALPN01(ctx, identifier.ACMEIdentifier{ @@ -170,7 +170,7 @@ func slowTLSSrv() *httptest.Server { func TestTLSALPNTimeoutAfterConnect(t *testing.T) { chall := tlsalpnChallenge() hs := slowTLSSrv() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) timeout := 50 * time.Millisecond ctx, cancel := context.WithTimeout(context.Background(), timeout) @@ -207,8 +207,7 @@ func TestTLSALPNTimeoutAfterConnect(t *testing.T) { func TestTLSALPN01DialTimeout(t *testing.T) { chall := tlsalpnChallenge() hs := slowTLSSrv() - va, _ := setup(hs, 0, "", nil) - va.dnsClient = dnsMockReturnsUnroutable{&bdns.MockClient{}} + va, _ := setup(hs, 0, "", nil, dnsMockReturnsUnroutable{&bdns.MockClient{}}) started := time.Now() timeout := 50 * time.Millisecond @@ -258,7 +257,7 @@ func TestTLSALPN01Refused(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifier, 0, "expected") test.AssertNotError(t, err, "Error creating test server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) // Take down validation server and check that validation fails. hs.Close() _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) @@ -277,7 +276,7 @@ func TestTLSALPN01TalkingToHTTP(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifier, 0, "expected") test.AssertNotError(t, err, "Error creating test server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) httpOnly := httpSrv(t, "") va.tlsPort = getPort(httpOnly) @@ -304,7 +303,7 @@ func TestTLSError(t *testing.T) { chall := tlsalpnChallenge() hs := brokenTLSSrv() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) if prob == nil { @@ -320,7 +319,7 @@ func TestDNSError(t *testing.T) { chall := tlsalpnChallenge() hs := brokenTLSSrv() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateTLSALPN01(ctx, dnsi("always.invalid"), chall) if prob == nil { @@ -395,7 +394,7 @@ func TestTLSALPN01Success(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifier, 0, "expected") test.AssertNotError(t, err, "Error creating test server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) if prob != nil { @@ -421,7 +420,7 @@ func TestTLSALPN01ObsoleteFailure(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifierV1Obsolete, 0, "expected") test.AssertNotError(t, err, "Error creating test server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) test.AssertNotNil(t, prob, "expected validation to fail") @@ -435,7 +434,7 @@ func TestValidateTLSALPN01BadChallenge(t *testing.T) { hs, err := tlsalpn01Srv(t, chall2, IdPeAcmeIdentifier, 0, "expected") test.AssertNotError(t, err, "Error creating test server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) @@ -456,7 +455,7 @@ func TestValidateTLSALPN01BrokenSrv(t *testing.T) { chall := tlsalpnChallenge() hs := brokenTLSSrv() - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) if prob == nil { @@ -469,7 +468,7 @@ func TestValidateTLSALPN01UnawareSrv(t *testing.T) { chall := tlsalpnChallenge() hs := tlssniSrvWithNames(t, "expected") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) if prob == nil { @@ -521,7 +520,7 @@ func TestValidateTLSALPN01MalformedExtnValue(t *testing.T) { } hs := tlsalpn01SrvWithCert(t, acmeCert, 0) - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) hs.Close() @@ -561,7 +560,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifier, tc.version, "expected") test.AssertNotError(t, err, "Error creating test server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) if !tc.expectError { @@ -588,7 +587,7 @@ func TestTLSALPN01WrongName(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifier, tls.VersionTLS12, "incorrect") test.AssertNotError(t, err, "failed to set up tls-alpn-01 server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) test.AssertError(t, prob, "validation should have failed") @@ -601,7 +600,7 @@ func TestTLSALPN01ExtraNames(t *testing.T) { hs, err := tlsalpn01Srv(t, chall, IdPeAcmeIdentifier, tls.VersionTLS12, "expected", "extra") test.AssertNotError(t, err, "failed to set up tls-alpn-01 server") - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) test.AssertError(t, prob, "validation should have failed") @@ -659,7 +658,7 @@ func TestTLSALPN01NotSelfSigned(t *testing.T) { hs := tlsalpn01SrvWithCert(t, acmeCert, tls.VersionTLS12) - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) test.AssertError(t, prob, "validation should have failed") @@ -706,7 +705,7 @@ func TestTLSALPN01ExtraIdentifiers(t *testing.T) { hs := tlsalpn01SrvWithCert(t, acmeCert, tls.VersionTLS12) - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) test.AssertError(t, prob, "validation should have failed") @@ -766,7 +765,7 @@ func TestTLSALPN01ExtraSANs(t *testing.T) { hs := tlsalpn01SrvWithCert(t, acmeCert, tls.VersionTLS12) - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) test.AssertError(t, prob, "validation should have failed") @@ -821,7 +820,7 @@ func TestTLSALPN01ExtraAcmeExtensions(t *testing.T) { hs := tlsalpn01SrvWithCert(t, acmeCert, tls.VersionTLS12) - va, _ := setup(hs, 0, "", nil) + va, _ := setup(hs, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) test.AssertError(t, prob, "validation should have failed") diff --git a/va/va.go b/va/va.go index 8f5a9353184..103896574e6 100644 --- a/va/va.go +++ b/va/va.go @@ -65,12 +65,18 @@ var ( h2SettingsFrameErrRegex = regexp.MustCompile(`(?:net\/http\: HTTP\/1\.x transport connection broken: )?malformed HTTP response \"\\x00\\x00\\x[a-f0-9]{2}\\x04\\x00\\x00\\x00\\x00\\x00.*"`) ) -// RemoteVA wraps the vapb.VAClient interface and adds a field containing the -// address of the remote gRPC server since the underlying gRPC client doesn't -// provide a way to extract this metadata which is useful for debugging gRPC -// connection issues. -type RemoteVA struct { +// RemoteClients wraps the vapb.VAClient and vapb.CAAClient interfaces to aid in +// mocking remote VAs for testing. +type RemoteClients struct { vapb.VAClient + vapb.CAAClient +} + +// RemoteVA embeds RemoteClients and adds a field containing the address of the +// remote gRPC server since the underlying gRPC client doesn't provide a way to +// extract this metadata which is useful for debugging gRPC connection issues. +type RemoteVA struct { + RemoteClients Address string } @@ -80,6 +86,11 @@ type vaMetrics struct { remoteValidationTime *prometheus.HistogramVec remoteValidationFailures prometheus.Counter prospectiveRemoteValidationFailures prometheus.Counter + caaCheckTime *prometheus.HistogramVec + localCAACheckTime *prometheus.HistogramVec + remoteCAACheckTime *prometheus.HistogramVec + remoteCAACheckFailures prometheus.Counter + prospectiveRemoteCAACheckFailures prometheus.Counter tlsALPNOIDCounter *prometheus.CounterVec http01Fallbacks prometheus.Counter http01Redirects prometheus.Counter @@ -124,6 +135,42 @@ func initMetrics(stats prometheus.Registerer) *vaMetrics { Help: "Number of validations that would have failed due to remote VAs returning failure if consesus were enforced", }) stats.MustRegister(prospectiveRemoteValidationFailures) + caaCheckTime := prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "caa_check_time", + Help: "Total time taken to check CAA records and aggregate results", + Buckets: metrics.InternetFacingBuckets, + }, + []string{"result"}) + stats.MustRegister(caaCheckTime) + localCAACheckTime := prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "caa_check_time_local", + Help: "Time taken to locally check CAA records", + Buckets: metrics.InternetFacingBuckets, + }, + []string{"result"}) + stats.MustRegister(localCAACheckTime) + remoteCAACheckTime := prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "caa_check_time_remote", + Help: "Time taken to remotely check CAA records", + Buckets: metrics.InternetFacingBuckets, + }, + []string{"result"}) + stats.MustRegister(remoteCAACheckTime) + remoteCAACheckFailures := prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "remote_caa_check_failures", + Help: "Number of CAA checks failed due to remote VAs returning failure when consensus is enforced", + }) + stats.MustRegister(remoteCAACheckFailures) + prospectiveRemoteCAACheckFailures := prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "prospective_remote_caa_check_failures", + Help: "Number of CAA rechecks that would have failed due to remote VAs returning failure if consesus were enforced", + }) + stats.MustRegister(prospectiveRemoteCAACheckFailures) tlsALPNOIDCounter := prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "tls_alpn_oid_usage", @@ -161,6 +208,11 @@ func initMetrics(stats prometheus.Registerer) *vaMetrics { localValidationTime: localValidationTime, remoteValidationFailures: remoteValidationFailures, prospectiveRemoteValidationFailures: prospectiveRemoteValidationFailures, + caaCheckTime: caaCheckTime, + localCAACheckTime: localCAACheckTime, + remoteCAACheckTime: remoteCAACheckTime, + remoteCAACheckFailures: remoteCAACheckFailures, + prospectiveRemoteCAACheckFailures: prospectiveRemoteCAACheckFailures, tlsALPNOIDCounter: tlsALPNOIDCounter, http01Fallbacks: http01Fallbacks, http01Redirects: http01Redirects, @@ -452,11 +504,11 @@ func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identi func (va *ValidationAuthorityImpl) performRemoteValidation( ctx context.Context, req *vapb.PerformValidationRequest, - results chan *remoteValidationResult) { + results chan<- *remoteVAResult) { for _, i := range rand.Perm(len(va.remoteVAs)) { remoteVA := va.remoteVAs[i] go func(rva RemoteVA) { - result := &remoteValidationResult{ + result := &remoteVAResult{ VAHostname: rva.Address, } res, err := rva.PerformValidation(ctx, req) @@ -486,29 +538,29 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( } } -// processRemoteResults evaluates a primary VA result, and a channel of remote -// VA problems to produce a single overall validation result based on configured -// feature flags. The overall result is calculated based on the VA's configured -// `maxRemoteFailures` value. +// processRemoteValidationResults evaluates a primary VA result, and a channel +// of remote VA problems to produce a single overall validation result based on +// configured feature flags. The overall result is calculated based on the VA's +// configured `maxRemoteFailures` value. // -// If the `MultiVAFullResults` feature is enabled then `processRemoteResults` -// will expect to read a result from the `remoteErrors` channel for each VA and -// will not produce an overall result until all remote VAs have responded. In -// this case `logRemoteFailureDifferentials` will also be called to describe the -// differential between the primary and all of the remote VAs. +// If the `MultiVAFullResults` feature is enabled then +// `processRemoteValidationResults` will expect to read a result from the +// `remoteErrors` channel for each VA and will not produce an overall result +// until all remote VAs have responded. In this case `logRemoteDifferentials` +// will also be called to describe the differential between the primary and all +// of the remote VAs. // // If the `MultiVAFullResults` feature flag is not enabled then -// `processRemoteResults` will potentially return before all remote VAs have had -// a chance to respond. This happens if the success or failure threshold is met. -// This doesn't allow for logging the differential between the primary and -// remote VAs but is more performant. -func (va *ValidationAuthorityImpl) processRemoteResults( +// `processRemoteValidationResults` will potentially return before all remote +// VAs have had a chance to respond. This happens if the success or failure +// threshold is met. This doesn't allow for logging the differential between the +// primary and remote VAs but is more performant. +func (va *ValidationAuthorityImpl) processRemoteValidationResults( domain string, acctID int64, challengeType string, primaryResult *probs.ProblemDetails, - remoteResultsChan chan *remoteValidationResult, - numRemoteVAs int) *probs.ProblemDetails { + remoteResultsChan <-chan *remoteVAResult) *probs.ProblemDetails { state := "failure" start := va.clk.Now() @@ -520,11 +572,11 @@ func (va *ValidationAuthorityImpl) processRemoteResults( }).Observe(va.clk.Since(start).Seconds()) }() - required := numRemoteVAs - va.maxRemoteFailures + required := len(va.remoteVAs) - va.maxRemoteFailures good := 0 bad := 0 - var remoteResults []*remoteValidationResult + var remoteResults []*remoteVAResult var firstProb *probs.ProblemDetails // Due to channel behavior this could block indefinitely and we rely on gRPC // honoring the context deadline used in client calls to prevent that from @@ -537,13 +589,11 @@ func (va *ValidationAuthorityImpl) processRemoteResults( } else { bad++ } - // Store the first non-nil problem to return later (if `MultiVAFullResults` // is enabled). if firstProb == nil && result.Problem != nil { firstProb = result.Problem } - // If MultiVAFullResults isn't enabled then return early whenever the // success or failure threshold is met. if !features.Get().MultiVAFullResults { @@ -559,15 +609,14 @@ func (va *ValidationAuthorityImpl) processRemoteResults( // If we haven't returned early because of MultiVAFullResults being enabled // we need to break the loop once all of the VAs have returned a result. - if len(remoteResults) == numRemoteVAs { + if len(remoteResults) == len(va.remoteVAs) { break } } - // If we are using `features.MultiVAFullResults` then we haven't returned // early and can now log the differential between what the primary VA saw and // what all of the remote VAs saw. - va.logRemoteValidationDifferentials( + va.logRemoteDifferentials( domain, acctID, challengeType, @@ -581,6 +630,12 @@ func (va *ValidationAuthorityImpl) processRemoteResults( } else if bad > va.maxRemoteFailures { modifiedProblem := *firstProb modifiedProblem.Detail = "During secondary validation: " + firstProb.Detail + // If the primary result was OK and there were more failures than the allowed + // threshold increment a stat that indicates this overall validation will have + // failed. + if primaryResult == nil { + va.metrics.prospectiveRemoteValidationFailures.Inc() + } return &modifiedProblem } @@ -589,18 +644,18 @@ func (va *ValidationAuthorityImpl) processRemoteResults( return probs.ServerInternal("Too few remote PerformValidation RPC results") } -// logRemoteValidationDifferentials is called by `processRemoteResults` when the -// `MultiVAFullResults` feature flag is enabled. It produces a JSON log line +// logRemoteDifferentials is called by `processRemoteValidationResults` when the +// `MultiVAFullResults` feature flag is enabled and `processRemoteCAAResults` +// `MultiCAAFullResults` feature flag is enabled. It produces a JSON log line // that contains the primary VA result and the results each remote VA returned. -func (va *ValidationAuthorityImpl) logRemoteValidationDifferentials( +func (va *ValidationAuthorityImpl) logRemoteDifferentials( domain string, acctID int64, challengeType string, primaryResult *probs.ProblemDetails, - remoteResults []*remoteValidationResult) { + remoteResults []*remoteVAResult) { - var successes []*remoteValidationResult - var failures []*remoteValidationResult + var successes, failures []*remoteVAResult allEqual := true for _, result := range remoteResults { @@ -619,20 +674,13 @@ func (va *ValidationAuthorityImpl) logRemoteValidationDifferentials( return } - // If the primary result was OK and there were more failures than the allowed - // threshold increment a stat that indicates this overall validation will have - // failed if features.EnforceMultiVA is enabled. - if primaryResult == nil && len(failures) > va.maxRemoteFailures { - va.metrics.prospectiveRemoteValidationFailures.Inc() - } - logOb := struct { Domain string AccountID int64 ChallengeType string PrimaryResult *probs.ProblemDetails RemoteSuccesses int - RemoteFailures []*remoteValidationResult + RemoteFailures []*remoteVAResult }{ Domain: domain, AccountID: acctID, @@ -644,20 +692,20 @@ func (va *ValidationAuthorityImpl) logRemoteValidationDifferentials( logJSON, err := json.Marshal(logOb) if err != nil { - // log a warning - a marshaling failure isn't expected given the data and - // isn't critical enough to break validation for by returning an error to - // the caller. + // log a warning - a marshaling failure isn't expected given the data + // isn't critical enough to break validation by returning an error the + // caller. va.log.Warningf("Could not marshal log object in "+ - "logRemoteValidationDifferentials: %s", err) + "logRemoteDifferential: %s", err) return } va.log.Infof("remoteVADifferentials JSON=%s", string(logJSON)) } -// remoteValidationResult is a struct that combines a problem details instance -// (that may be nil) with the remote VA hostname that produced it. -type remoteValidationResult struct { +// remoteVAResult is a struct that combines a problem details instance (that may +// be nil) with the remote VA hostname that produced it. +type remoteVAResult struct { VAHostname string Problem *probs.ProblemDetails } @@ -676,9 +724,9 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v } vStart := va.clk.Now() - var remoteResults chan *remoteValidationResult + var remoteResults chan *remoteVAResult if remoteVACount := len(va.remoteVAs); remoteVACount > 0 { - remoteResults = make(chan *remoteValidationResult, remoteVACount) + remoteResults = make(chan *remoteVAResult, remoteVACount) go va.performRemoteValidation(ctx, req, remoteResults) } @@ -708,26 +756,25 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v // differentials then collect and log the remote results in a separate go // routine to avoid blocking the primary VA. go func() { - _ = va.processRemoteResults( + _ = va.processRemoteValidationResults( req.Domain, req.Authz.RegID, string(challenge.Type), prob, - remoteResults, - len(va.remoteVAs)) + remoteResults) }() // Since prob was nil and we're not enforcing the results from - // `processRemoteResults` set the challenge status to valid so the - // validationTime metrics increment has the correct result label. + // `processRemoteValidationResults` set the challenge status to + // valid so the validationTime metrics increment has the correct + // result label. challenge.Status = core.StatusValid } else if features.Get().EnforceMultiVA { - remoteProb := va.processRemoteResults( + remoteProb := va.processRemoteValidationResults( req.Domain, req.Authz.RegID, string(challenge.Type), prob, - remoteResults, - len(va.remoteVAs)) + remoteResults) // If the remote result was a non-nil problem then fail the validation if remoteProb != nil { diff --git a/va/va_test.go b/va/va_test.go index 7894200540a..0555b473bea 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -117,7 +117,9 @@ func setChallengeToken(ch *core.Challenge, token string) { ch.ProvidedKeyAuthorization = token + ".9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI" } -func setup(srv *httptest.Server, maxRemoteFailures int, userAgent string, remoteVAs []RemoteVA) (*ValidationAuthorityImpl, *blog.Mock) { +// setup returns an in-memory VA and a mock logger. The default resolver client +// is MockClient{}, but can be overridden. +func setup(srv *httptest.Server, maxRemoteFailures int, userAgent string, remoteVAs []RemoteVA, mockDNSClientOverride bdns.Client) (*ValidationAuthorityImpl, *blog.Mock) { features.Reset() fc := clock.NewFake() @@ -139,6 +141,10 @@ func setup(srv *httptest.Server, maxRemoteFailures int, userAgent string, remote accountURIPrefixes, ) + if mockDNSClientOverride != nil { + va.dnsClient = mockDNSClientOverride + } + // Adjusting industry regulated ACME challenge port settings is fine during // testing if srv != nil { @@ -156,9 +162,10 @@ func setup(srv *httptest.Server, maxRemoteFailures int, userAgent string, remote return va, logger } -func setupRemote(srv *httptest.Server, userAgent string) vapb.VAClient { - innerVA, _ := setup(srv, 0, userAgent, nil) - return &localRemoteVA{remote: *innerVA} +func setupRemote(srv *httptest.Server, userAgent string, mockDNSClientOverride bdns.Client) RemoteClients { + rva, _ := setup(srv, 0, userAgent, nil, mockDNSClientOverride) + + return RemoteClients{VAClient: &inMemVA{*rva}, CAAClient: &inMemVA{*rva}} } type multiSrv struct { @@ -210,8 +217,12 @@ func (v cancelledVA) PerformValidation(_ context.Context, _ *vapb.PerformValidat return nil, context.Canceled } -// brokenRemoteVA is a mock for the vapb.VAClient interface mocked to -// always return errors. +func (v cancelledVA) IsCAAValid(_ context.Context, _ *vapb.IsCAAValidRequest, _ ...grpc.CallOption) (*vapb.IsCAAValidResponse, error) { + return nil, context.Canceled +} + +// brokenRemoteVA is a mock for the VAClient and CAAClient interfaces that always return +// errors. type brokenRemoteVA struct{} // errBrokenRemoteVA is the error returned by a brokenRemoteVA's @@ -223,19 +234,28 @@ func (b brokenRemoteVA) PerformValidation(_ context.Context, _ *vapb.PerformVali return nil, errBrokenRemoteVA } -// localRemoteVA is a wrapper which fulfills the VAClient interface, but then -// forwards requests directly to its inner ValidationAuthorityImpl rather than -// over the network. This lets a local in-memory mock VA act like a remote VA. -type localRemoteVA struct { - remote ValidationAuthorityImpl +func (b brokenRemoteVA) IsCAAValid(_ context.Context, _ *vapb.IsCAAValidRequest, _ ...grpc.CallOption) (*vapb.IsCAAValidResponse, error) { + return nil, errBrokenRemoteVA +} + +// inMemVA is a wrapper which fulfills the VAClient and CAAClient +// interfaces, but then forwards requests directly to its inner +// ValidationAuthorityImpl rather than over the network. This lets a local +// in-memory mock VA act like a remote VA. +type inMemVA struct { + rva ValidationAuthorityImpl } -func (lrva localRemoteVA) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest, _ ...grpc.CallOption) (*vapb.ValidationResult, error) { - return lrva.remote.PerformValidation(ctx, req) +func (inmem inMemVA) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest, _ ...grpc.CallOption) (*vapb.ValidationResult, error) { + return inmem.rva.PerformValidation(ctx, req) +} + +func (inmem inMemVA) IsCAAValid(ctx context.Context, req *vapb.IsCAAValidRequest, _ ...grpc.CallOption) (*vapb.IsCAAValidResponse, error) { + return inmem.rva.IsCAAValid(ctx, req) } func TestValidateMalformedChallenge(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) _, prob := va.validateChallenge(ctx, dnsi("example.com"), createChallenge("fake-type-01")) @@ -243,7 +263,7 @@ func TestValidateMalformedChallenge(t *testing.T) { } func TestPerformValidationInvalid(t *testing.T) { - va, _ := setup(nil, 0, "", nil) + va, _ := setup(nil, 0, "", nil, nil) req := createValidationRequest("foo.com", core.ChallengeTypeDNS01) res, _ := va.PerformValidation(context.Background(), req) @@ -257,7 +277,7 @@ func TestPerformValidationInvalid(t *testing.T) { } func TestPerformValidationValid(t *testing.T) { - va, mockLog := setup(nil, 0, "", nil) + va, mockLog := setup(nil, 0, "", nil, nil) // create a challenge with well known token req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01) @@ -281,7 +301,7 @@ func TestPerformValidationValid(t *testing.T) { // TestPerformValidationWildcard tests that the VA properly strips the `*.` // prefix from a wildcard name provided to the PerformValidation function. func TestPerformValidationWildcard(t *testing.T) { - va, mockLog := setup(nil, 0, "", nil) + va, mockLog := setup(nil, 0, "", nil, nil) // create a challenge with well known token req := createValidationRequest("*.good-dns01.com", core.ChallengeTypeDNS01) @@ -311,7 +331,7 @@ func TestPerformValidationWildcard(t *testing.T) { } func TestDCVAndCAASequencing(t *testing.T) { - va, mockLog := setup(nil, 0, "", nil) + va, mockLog := setup(nil, 0, "", nil, nil) // When performing validation without the CAAAfterValidation flag, CAA should // be checked. @@ -365,14 +385,20 @@ func TestMultiVA(t *testing.T) { ms := httpMultiSrv(t, expectedToken, allowedUAs) defer ms.Close() - remoteVA1 := setupRemote(ms.Server, remoteUA1) - remoteVA2 := setupRemote(ms.Server, remoteUA2) - + remoteVA1 := setupRemote(ms.Server, remoteUA1, nil) + remoteVA2 := setupRemote(ms.Server, remoteUA2, nil) remoteVAs := []RemoteVA{ {remoteVA1, remoteUA1}, {remoteVA2, remoteUA2}, } - + brokenVA := RemoteClients{ + VAClient: brokenRemoteVA{}, + CAAClient: brokenRemoteVA{}, + } + cancelledVA := RemoteClients{ + VAClient: cancelledVA{}, + CAAClient: cancelledVA{}, + } enforceMultiVA := features.Config{ EnforceMultiVA: true, } @@ -391,11 +417,9 @@ func TestMultiVA(t *testing.T) { unauthorized := probs.Unauthorized(fmt.Sprintf( `The key authorization file from the server did not match this challenge. Expected %q (got "???")`, expectedKeyAuthorization)) - expectedInternalErrLine := fmt.Sprintf( `ERR: \[AUDIT\] Remote VA "broken".PerformValidation failed: %s`, errBrokenRemoteVA.Error()) - testCases := []struct { Name string RemoteVAs []RemoteVA @@ -439,7 +463,7 @@ func TestMultiVA(t *testing.T) { Name: "Local VA ok, remote VA internal err, enforce multi VA", RemoteVAs: []RemoteVA{ {remoteVA1, remoteUA1}, - {&brokenRemoteVA{}, "broken"}, + {brokenVA, "broken"}, }, AllowedUAs: allowedUAs, Features: enforceMultiVA, @@ -453,7 +477,7 @@ func TestMultiVA(t *testing.T) { Name: "Local VA ok, remote VA internal err, no enforce multi VA", RemoteVAs: []RemoteVA{ {remoteVA1, remoteUA1}, - {&brokenRemoteVA{}, "broken"}, + {brokenVA, "broken"}, }, AllowedUAs: allowedUAs, Features: noEnforceMultiVA, @@ -485,7 +509,7 @@ func TestMultiVA(t *testing.T) { Name: "Local VA and one remote VA OK, one cancelled VA, enforce multi VA", RemoteVAs: []RemoteVA{ {remoteVA1, remoteUA1}, - {cancelledVA{}, remoteUA2}, + {cancelledVA, remoteUA2}, }, AllowedUAs: allowedUAs, Features: enforceMultiVA, @@ -495,8 +519,8 @@ func TestMultiVA(t *testing.T) { // When enforcing multi-VA, any cancellations are a problem. Name: "Local VA OK, two cancelled remote VAs, enforce multi VA", RemoteVAs: []RemoteVA{ - {cancelledVA{}, remoteUA1}, - {cancelledVA{}, remoteUA2}, + {cancelledVA, remoteUA1}, + {cancelledVA, remoteUA2}, }, AllowedUAs: allowedUAs, Features: enforceMultiVA, @@ -530,7 +554,7 @@ func TestMultiVA(t *testing.T) { ms.setAllowedUAs(tc.AllowedUAs) // Configure a primary VA with testcase remote VAs. - localVA, mockLog := setup(ms.Server, 0, localUA, tc.RemoteVAs) + localVA, mockLog := setup(ms.Server, 0, localUA, tc.RemoteVAs, nil) features.Set(tc.Features) defer features.Reset() @@ -572,8 +596,8 @@ func TestMultiVAEarlyReturn(t *testing.T) { ms := httpMultiSrv(t, expectedToken, allowedUAs) defer ms.Close() - remoteVA1 := setupRemote(ms.Server, remoteUA1) - remoteVA2 := setupRemote(ms.Server, remoteUA2) + remoteVA1 := setupRemote(ms.Server, remoteUA1, nil) + remoteVA2 := setupRemote(ms.Server, remoteUA2, nil) remoteVAs := []RemoteVA{ {remoteVA1, remoteUA1}, @@ -581,7 +605,7 @@ func TestMultiVAEarlyReturn(t *testing.T) { } // Create a local test VA with the two remote VAs - localVA, mockLog := setup(ms.Server, 0, localUA, remoteVAs) + localVA, mockLog := setup(ms.Server, 0, localUA, remoteVAs, nil) testCases := []struct { Name string @@ -660,8 +684,8 @@ func TestMultiVAPolicy(t *testing.T) { ms := httpMultiSrv(t, expectedToken, allowedUAs) defer ms.Close() - remoteVA1 := setupRemote(ms.Server, remoteUA1) - remoteVA2 := setupRemote(ms.Server, remoteUA2) + remoteVA1 := setupRemote(ms.Server, remoteUA1, nil) + remoteVA2 := setupRemote(ms.Server, remoteUA2, nil) remoteVAs := []RemoteVA{ {remoteVA1, remoteUA1}, @@ -669,7 +693,7 @@ func TestMultiVAPolicy(t *testing.T) { } // Create a local test VA with the two remote VAs - localVA, _ := setup(ms.Server, 0, localUA, remoteVAs) + localVA, _ := setup(ms.Server, 0, localUA, remoteVAs, nil) // Ensure multi VA enforcement is enabled, don't wait for full multi VA // results. @@ -740,11 +764,11 @@ func TestDetailedError(t *testing.T) { } } -func TestLogRemoteValidationDifferentials(t *testing.T) { +func TestLogRemoteDifferentials(t *testing.T) { // Create some remote VAs - remoteVA1 := setupRemote(nil, "remote 1") - remoteVA2 := setupRemote(nil, "remote 2") - remoteVA3 := setupRemote(nil, "remote 3") + remoteVA1 := setupRemote(nil, "remote 1", nil) + remoteVA2 := setupRemote(nil, "remote 2", nil) + remoteVA3 := setupRemote(nil, "remote 3", nil) remoteVAs := []RemoteVA{ {remoteVA1, "remote 1"}, {remoteVA2, "remote 2"}, @@ -752,7 +776,7 @@ func TestLogRemoteValidationDifferentials(t *testing.T) { } // Set up a local VA that allows a max of 2 remote failures. - localVA, mockLog := setup(nil, 2, "local 1", remoteVAs) + localVA, mockLog := setup(nil, 2, "local 1", remoteVAs, nil) egProbA := probs.DNS("root DNS servers closed at 4:30pm") egProbB := probs.OrderNotReady("please take a number") @@ -760,13 +784,13 @@ func TestLogRemoteValidationDifferentials(t *testing.T) { testCases := []struct { name string primaryResult *probs.ProblemDetails - remoteProbs []*remoteValidationResult + remoteProbs []*remoteVAResult expectedLog string }{ { name: "remote and primary results equal (all nil)", primaryResult: nil, - remoteProbs: []*remoteValidationResult{ + remoteProbs: []*remoteVAResult{ {Problem: nil, VAHostname: "remoteA"}, {Problem: nil, VAHostname: "remoteB"}, {Problem: nil, VAHostname: "remoteC"}, @@ -775,7 +799,7 @@ func TestLogRemoteValidationDifferentials(t *testing.T) { { name: "remote and primary results equal (not nil)", primaryResult: egProbA, - remoteProbs: []*remoteValidationResult{ + remoteProbs: []*remoteVAResult{ {Problem: egProbA, VAHostname: "remoteA"}, {Problem: egProbA, VAHostname: "remoteB"}, {Problem: egProbA, VAHostname: "remoteC"}, @@ -784,7 +808,7 @@ func TestLogRemoteValidationDifferentials(t *testing.T) { { name: "remote and primary differ (primary nil)", primaryResult: nil, - remoteProbs: []*remoteValidationResult{ + remoteProbs: []*remoteVAResult{ {Problem: egProbA, VAHostname: "remoteA"}, {Problem: nil, VAHostname: "remoteB"}, {Problem: egProbB, VAHostname: "remoteC"}, @@ -794,7 +818,7 @@ func TestLogRemoteValidationDifferentials(t *testing.T) { { name: "remote and primary differ (primary not nil)", primaryResult: egProbA, - remoteProbs: []*remoteValidationResult{ + remoteProbs: []*remoteVAResult{ {Problem: nil, VAHostname: "remoteA"}, {Problem: egProbB, VAHostname: "remoteB"}, {Problem: nil, VAHostname: "remoteC"}, @@ -807,7 +831,7 @@ func TestLogRemoteValidationDifferentials(t *testing.T) { t.Run(tc.name, func(t *testing.T) { mockLog.Clear() - localVA.logRemoteValidationDifferentials( + localVA.logRemoteDifferentials( "example.com", 1999, "blorpus-01", tc.primaryResult, tc.remoteProbs) lines := mockLog.GetAllMatching("remoteVADifferentials JSON=.*")