va: compute maxRemoteFailures based on MPIC

cmd/boulder-va/main.go

@@ -18,8 +18,9 @@ import (
 type Config struct {
 	VA struct {
 		vaConfig.Common
-		RemoteVAs                   []cmd.GRPCClientConfig `validate:"omitempty,dive"`
-		MaxRemoteValidationFailures int                    `validate:"omitempty,min=0,required_with=RemoteVAs"`
+		RemoteVAs []cmd.GRPCClientConfig `validate:"omitempty,dive"`
+		// Deprecated and ignored
+		MaxRemoteValidationFailures int `validate:"omitempty,min=0,required_with=RemoteVAs"`
 		Features                    features.Config
 	}
 
@@ -109,7 +110,6 @@ func main() {
 	vai, err := va.NewValidationAuthorityImpl(
 		resolver,
 		remotes,
-		c.VA.MaxRemoteValidationFailures,
 		c.VA.UserAgent,
 		c.VA.IssuerDomain,
 		scope,

cmd/remoteva/main.go

@@ -135,7 +135,6 @@ func main() {
 	vai, err := va.NewValidationAuthorityImpl(
 		resolver,
 		nil, // Our RVAs will never have RVAs of their own.
-		0,   // Only the VA is concerned with max validation failures
 		c.RVA.UserAgent,
 		c.RVA.IssuerDomain,
 		scope,

docs/multi-va.md

@@ -38,7 +38,7 @@ and
 as their config files.
 
 We require that almost all remote validation requests succeed; the exact number
-is controlled by the VA's `maxRemoteFailures` config variable. If the number of
+is controlled by the VA based on the thresholds required by MPIC. If the number of
 failing remote VAs exceeds that threshold, validation is terminated. If the
 number of successful remote VAs is high enough that it would be impossible for
 the outstanding remote VAs to exceed that threshold, validation immediately

features/features.go

@@ -53,8 +53,8 @@ type Config struct {
 
 	// 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.
+	// number of failures is greater than the number allowed by MPIC.
+	// Only used when EnforceMultiCAA is true.
 	MultiCAAFullResults bool
 
 	// MultipleCertificateProfiles, when enabled, triggers the following

test/config-next/va.json

@@ -54,7 +54,6 @@
 				"hostOverride": "rva1.boulder"
 			}
 		],
-		"maxRemoteValidationFailures": 1,
 		"accountURIPrefixes": [
 			"http://boulder.service.consul:4000/acme/reg/",
 			"http://boulder.service.consul:4001/acme/acct/"

va/caa_test.go

@@ -191,7 +191,7 @@ func (mock caaMockDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA,
 }
 
 func TestCAATimeout(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, caaMockDNS{})
+	va, _ := setup(nil, "", nil, caaMockDNS{})
 
 	params := &caaParams{
 		accountURIID:     12345,
@@ -408,7 +408,7 @@ func TestCAAChecking(t *testing.T) {
 	method := core.ChallengeTypeHTTP01
 	params := &caaParams{accountURIID: accountURIID, validationMethod: method}
 
-	va, _ := setup(nil, 0, "", nil, caaMockDNS{})
+	va, _ := setup(nil, "", nil, caaMockDNS{})
 	va.accountURIPrefixes = []string{"https://letsencrypt.org/acct/reg/"}
 
 	for _, caaTest := range testCases {
@@ -431,7 +431,7 @@ func TestCAAChecking(t *testing.T) {
 }
 
 func TestCAALogging(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, caaMockDNS{})
+	va, _ := setup(nil, "", nil, caaMockDNS{})
 
 	testCases := []struct {
 		Name            string
@@ -521,7 +521,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, caaMockDNS{})
+	va, _ := setup(nil, "", nil, caaMockDNS{})
 
 	// Call IsCAAValid with a domain we know fails with a generic error from the
 	// caaMockDNS.
@@ -546,7 +546,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, caaMockDNS{})
+	va, _ := setup(nil, "", nil, caaMockDNS{})
 
 	// Calling IsCAAValid without a ValidationMethod should fail.
 	_, err := va.IsCAAValid(ctx, &vapb.IsCAAValidRequest{
@@ -592,7 +592,7 @@ func (b caaBrokenDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, s
 func TestDisabledMultiCAARechecking(t *testing.T) {
 	brokenRVA := setupRemote(nil, "broken", caaBrokenDNS{}, "", "")
 	remoteVAs := []RemoteVA{{brokenRVA, "broken"}}
-	va, _ := setup(nil, 0, "local", remoteVAs, nil)
+	va, _ := setup(nil, "local", remoteVAs, nil)
 
 	features.Set(features.Config{
 		EnforceMultiCAA:     false,
@@ -671,7 +671,6 @@ func TestMultiCAARechecking(t *testing.T) {
 
 	testCases := []struct {
 		name                     string
-		maxLookupFailures        int
 		domains                  string
 		remoteVAs                []RemoteVA
 		expectedProbSubstring    string
@@ -719,13 +718,31 @@ func TestMultiCAARechecking(t *testing.T) {
 		{
 			name:                     "functional localVA, 1 broken RVA, no CAA records",
 			domains:                  "present-dns-only.com",
+			localDNSClient:           caaMockDNS{},
+			expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
+			remoteVAs: []RemoteVA{
+				{brokenVA, brokenUA},
+				{remoteVA, remoteUA},
+				{remoteVA, remoteUA},
+			},
+			expectedLabels: prometheus.Labels{
+				"operation":      opCAA,
+				"perspective":    allPerspectives,
+				"challenge_type": string(core.ChallengeTypeDNS01),
+				"problem_type":   "",
+				"result":         pass,
+			},
+		},
+		{
+			name:                     "functional localVA, 2 broken RVAs, 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`,
+			expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
 			localDNSClient:           caaMockDNS{},
 			remoteVAs: []RemoteVA{
 				{brokenVA, brokenUA},
-				{remoteVA, remoteUA},
+				{brokenVA, brokenUA},
 				{remoteVA, remoteUA},
 			},
 			expectedLabels: prometheus.Labels{
@@ -776,15 +793,33 @@ func TestMultiCAARechecking(t *testing.T) {
 		{
 			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},
 			},
+			expectedLabels: prometheus.Labels{
+				"operation":      opCAA,
+				"perspective":    allPerspectives,
+				"challenge_type": string(core.ChallengeTypeDNS01),
+				"problem_type":   "",
+				"result":         pass,
+			},
+		},
+		{
+			name:                     "functional localVA, 2 broken RVA, CAA issue type present",
+			domains:                  "present.com",
+			expectedProbSubstring:    "During secondary CAA checking: While processing CAA",
+			expectedProbType:         probs.DNSProblem,
+			expectedDiffLogSubstring: `RemoteSuccesses":1,"RemoteFailures":[{"VAHostname":"broken","Problem":{"type":"dns","detail":"While processing CAA for`,
+			localDNSClient:           caaMockDNS{},
+			remoteVAs: []RemoteVA{
+				{brokenVA, brokenUA},
+				{brokenVA, brokenUA},
+				{remoteVA, remoteUA},
+			},
 			expectedLabels: prometheus.Labels{
 				"operation":      opCAA,
 				"perspective":    allPerspectives,
@@ -831,8 +866,6 @@ func TestMultiCAARechecking(t *testing.T) {
 		{
 			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{
@@ -870,8 +903,6 @@ func TestMultiCAARechecking(t *testing.T) {
 		{
 			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{
@@ -907,9 +938,8 @@ func TestMultiCAARechecking(t *testing.T) {
 			},
 		},
 		{
-			name:                     "1 hijacked RVA, CAA issuewild type present, 1 failure allowed",
+			name:                     "1 hijacked RVA, CAA issuewild type present",
 			domains:                  "satisfiable-wildcard.com",
-			maxLookupFailures:        1,
 			expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`,
 			localDNSClient:           caaMockDNS{},
 			remoteVAs: []RemoteVA{
@@ -919,9 +949,8 @@ func TestMultiCAARechecking(t *testing.T) {
 			},
 		},
 		{
-			name:                     "2 hijacked RVAs, CAA issuewild type present, 1 failure allowed",
+			name:                     "2 hijacked RVAs, CAA issuewild type present",
 			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`,
@@ -933,9 +962,8 @@ func TestMultiCAARechecking(t *testing.T) {
 			},
 		},
 		{
-			name:                     "3 hijacked RVAs, CAA issuewild type present, 1 failure allowed",
+			name:                     "3 hijacked RVAs, CAA issuewild type present",
 			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`,
@@ -950,7 +978,7 @@ func TestMultiCAARechecking(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			va, mockLog := setup(nil, tc.maxLookupFailures, localUA, tc.remoteVAs, tc.localDNSClient)
+			va, mockLog := setup(nil, localUA, tc.remoteVAs, tc.localDNSClient)
 			defer mockLog.Clear()
 
 			// MultiCAAFullResults: false is inherently flaky because of the
@@ -971,12 +999,14 @@ func TestMultiCAARechecking(t *testing.T) {
 			test.AssertNotError(t, err, "Should not have errored, but did")
 
 			if tc.expectedProbSubstring != "" {
+				test.AssertNotNil(t, isValidRes.Problem, "IsCAAValidRequest returned nil problem, but should not have")
 				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.AssertNotNil(t, isValidRes.Problem, "IsCAAValidRequest returned nil problem, but should not have")
 				test.AssertEquals(t, string(tc.expectedProbType), isValidRes.Problem.ProblemType)
 			}
 
@@ -1017,7 +1047,7 @@ func TestCAAFailure(t *testing.T) {
 	hs := httpSrv(t, expectedToken)
 	defer hs.Close()
 
-	va, _ := setup(hs, 0, "", nil, caaMockDNS{})
+	va, _ := setup(hs, "", nil, caaMockDNS{})
 
 	err := va.checkCAA(ctx, dnsi("reserved.com"), &caaParams{1, core.ChallengeTypeHTTP01})
 	if err == nil {

va/dns_test.go

@@ -19,7 +19,7 @@ import (
 )
 
 func TestDNSValidationEmpty(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	// This test calls PerformValidation directly, because that is where the
 	// metrics checked below are incremented.
@@ -38,7 +38,7 @@ func TestDNSValidationEmpty(t *testing.T) {
 }
 
 func TestDNSValidationWrong(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 	_, err := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), expectedKeyAuthorization)
 	if err == nil {
 		t.Fatalf("Successful DNS validation with wrong TXT record")
@@ -48,7 +48,7 @@ func TestDNSValidationWrong(t *testing.T) {
 }
 
 func TestDNSValidationWrongMany(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	_, err := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), expectedKeyAuthorization)
 	if err == nil {
@@ -59,7 +59,7 @@ func TestDNSValidationWrongMany(t *testing.T) {
 }
 
 func TestDNSValidationWrongLong(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	_, err := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), expectedKeyAuthorization)
 	if err == nil {
@@ -70,7 +70,7 @@ func TestDNSValidationWrongLong(t *testing.T) {
 }
 
 func TestDNSValidationFailure(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	_, err := va.validateDNS01(ctx, dnsi("localhost"), expectedKeyAuthorization)
 	prob := detailedError(err)
@@ -84,7 +84,7 @@ func TestDNSValidationInvalid(t *testing.T) {
 		Value: "790DB180-A274-47A4-855F-31C428CB1072",
 	}
 
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	_, err := va.validateDNS01(ctx, notDNS, expectedKeyAuthorization)
 	prob := detailedError(err)
@@ -93,7 +93,7 @@ func TestDNSValidationInvalid(t *testing.T) {
 }
 
 func TestDNSValidationServFail(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	_, err := va.validateDNS01(ctx, dnsi("servfail.com"), expectedKeyAuthorization)
 
@@ -102,7 +102,7 @@ func TestDNSValidationServFail(t *testing.T) {
 }
 
 func TestDNSValidationNoServer(t *testing.T) {
-	va, log := setup(nil, 0, "", nil, nil)
+	va, log := setup(nil, "", nil, nil)
 	staticProvider, err := bdns.NewStaticProvider([]string{})
 	test.AssertNotError(t, err, "Couldn't make new static provider")
 
@@ -121,15 +121,15 @@ func TestDNSValidationNoServer(t *testing.T) {
 }
 
 func TestDNSValidationOK(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	_, prob := va.validateDNS01(ctx, dnsi("good-dns01.com"), expectedKeyAuthorization)
 
 	test.Assert(t, prob == nil, "Should be valid.")
 }
 
 func TestDNSValidationNoAuthorityOK(t *testing.T) {
-	va, _ := setup(nil, 0, "", nil, nil)
+	va, _ := setup(nil, "", nil, nil)
 
 	_, prob := va.validateDNS01(ctx, dnsi("no-authority-dns01.com"), expectedKeyAuthorization)