va: prepare to require minimum of 3 RVAs (#7815)

jsha · web-flow · commit 577a1e38ebd3 · 2024-11-19T10:23:32.000-05:00
To prepare for the MPIC requirement of having a minimum of 3
perspectives, I added code to `NewValidationAuthorityImpl` to error if
there aren't enough remote VAs configured _and_ the current VA is the
primary perspective. Then I fixed all the tests, which involved adding
some backends in the unittests, and spinning up `remoteva-c` in the
integration tests.

As a reminder, the `boulder va` command always considers itself the
primary perspective, while `boulder remoteva` gives itself a perspective
based on its config.

I wound up backing out the code in `NewValidationAuthorityImpl` because
right now our remote VAs are actually running the `boulder va` command,
so they would error out in prod, even though our actual primary
perspective does have enough backends. So this wound up as a test-only
change.
diff --git a/test/config-next/remoteva-c.json b/test/config-next/remoteva-c.json
@@ -0,0 +1,51 @@
+{
+	"rva": {
+		"userAgent": "remoteva-c",
+		"dnsTries": 3,
+		"dnsStaticResolvers": [
+			"10.77.77.77:8343",
+			"10.77.77.77:8443"
+		],
+		"dnsTimeout": "1s",
+		"dnsAllowLoopbackAddresses": true,
+		"issuerDomain": "happy-hacker-ca.invalid",
+		"tls": {
+			"caCertfile": "test/certs/ipki/minica.pem",
+			"certFile": "test/certs/ipki/rva.boulder/cert.pem",
+			"keyFile": "test/certs/ipki/rva.boulder/key.pem"
+		},
+		"skipGRPCClientCertVerification": true,
+		"grpc": {
+			"maxConnectionAge": "30s",
+			"services": {
+				"va.VA": {
+					"clientNames": [
+						"va.boulder"
+					]
+				},
+				"grpc.health.v1.Health": {
+					"clientNames": [
+						"health-checker.boulder"
+					]
+				}
+			}
+		},
+		"features": {
+			"DOH": true
+		},
+		"accountURIPrefixes": [
+			"http://boulder.service.consul:4000/acme/reg/",
+			"http://boulder.service.consul:4001/acme/acct/"
+		],
+		"perspective": "development",
+		"rir": "ARIN"
+	},
+	"syslog": {
+		"stdoutlevel": 4,
+		"sysloglevel": -1
+	},
+	"openTelemetry": {
+		"endpoint": "bjaeger:4317",
+		"sampleratio": 1
+	}
+}
diff --git a/test/config-next/va.json b/test/config-next/va.json
@@ -52,6 +52,11 @@
 				"serverAddress": "rva1.service.consul:9498",
 				"timeout": "15s",
 				"hostOverride": "rva1.boulder"
+			},
+			{
+				"serverAddress": "rva1.service.consul:9499",
+				"timeout": "15s",
+				"hostOverride": "rva1.boulder"
 			}
 		],
 		"accountURIPrefixes": [
diff --git a/test/config/remoteva-c.json b/test/config/remoteva-c.json
@@ -0,0 +1,47 @@
+{
+	"rva": {
+		"userAgent": "remoteva-c",
+		"debugAddr": ":8213",
+		"dnsTries": 3,
+		"dnsProvider": {
+			"dnsAuthority": "consul.service.consul",
+			"srvLookup": {
+				"service": "dns",
+				"domain": "service.consul"
+			}
+		},
+		"dnsTimeout": "1s",
+		"dnsAllowLoopbackAddresses": true,
+		"issuerDomain": "happy-hacker-ca.invalid",
+		"tls": {
+			"caCertfile": "test/certs/ipki/minica.pem",
+			"certFile": "test/certs/ipki/rva.boulder/cert.pem",
+			"keyFile": "test/certs/ipki/rva.boulder/key.pem"
+		},
+		"grpc": {
+			"maxConnectionAge": "30s",
+			"address": ":9899",
+			"services": {
+				"va.VA": {
+					"clientNames": [
+						"va.boulder"
+					]
+				},
+				"grpc.health.v1.Health": {
+					"clientNames": [
+						"health-checker.boulder"
+					]
+				}
+			}
+		},
+		"features": {},
+		"accountURIPrefixes": [
+			"http://boulder.service.consul:4000/acme/reg/",
+			"http://boulder.service.consul:4001/acme/acct/"
+		]
+	},
+	"syslog": {
+		"stdoutlevel": 4,
+		"sysloglevel": 4
+	}
+}
diff --git a/test/config/va.json b/test/config/va.json
@@ -49,6 +49,11 @@
 				"serverAddress": "rva1.service.consul:9498",
 				"timeout": "15s",
 				"hostOverride": "rva1.boulder"
+			},
+			{
+				"serverAddress": "rva1.service.consul:9499",
+				"timeout": "15s",
+				"hostOverride": "rva1.boulder"
 			}
 		],
 		"maxRemoteValidationFailures": 1,
diff --git a/test/consul/config.hcl b/test/consul/config.hcl
@@ -176,6 +176,14 @@ services {
   tags    = ["tcp"] // Required for SRV RR support in gRPC DNS resolution.
 }
 
+services {
+  id      = "rva1-c"
+  name    = "rva1"
+  address = "10.77.77.77"
+  port    = 9499
+  tags    = ["tcp"] // Required for SRV RR support in gRPC DNS resolution.
+}
+
 # TODO(#5294) Remove rva2-a/b in favor of rva1-a/b
 services {
   id      = "rva2-a"
diff --git a/test/startservers.py b/test/startservers.py
@@ -24,6 +24,10 @@
         8012, 9498, 'rva.boulder',
         ('./bin/boulder', 'remoteva', '--config', os.path.join(config_dir, 'remoteva-b.json'), '--addr', ':9498', '--debug-addr', ':8012'),
         None),
+    Service('remoteva-c',
+        8023, 9499, 'rva.boulder',
+        ('./bin/boulder', 'remoteva', '--config', os.path.join(config_dir, 'remoteva-c.json'), '--addr', ':9499', '--debug-addr', ':8023'),
+        None),
     Service('boulder-sa-1',
         8003, 9395, 'sa.boulder',
         ('./bin/boulder', 'boulder-sa', '--config', os.path.join(config_dir, 'sa.json'), '--addr', ':9395', '--debug-addr', ':8003'),
diff --git a/test/v2_integration.py b/test/v2_integration.py
@@ -1005,7 +1005,8 @@ def test_http_multiva_threshold_pass():
 
     # Configure a guestlist that will pass the multiVA threshold test by
     # allowing the primary VA at some, but not all, remotes.
-    guestlist = {"boulder": 1, "boulder-remoteva-a": 1, "boulder-remoteva-b": 1, "remoteva-a": 1}
+    # In particular, remoteva-c is missing.
+    guestlist = {"boulder": 1, "remoteva-a": 1, "remoteva-b": 1}
 
     hostname, cleanup = multiva_setup(client, guestlist)
 
@@ -1021,7 +1022,7 @@ def test_http_multiva_primary_fail_remote_pass():
 
     # Configure a guestlist that will fail the primary VA check but allow all of
     # the remote VAs.
-    guestlist = {"boulder": 0, "boulder-remoteva-a": 1, "boulder-remoteva-b": 1, "remoteva-a": 1, "remoteva-b": 1}
+    guestlist = {"boulder": 0, "remoteva-a": 1, "remoteva-b": 1}
 
     hostname, cleanup = multiva_setup(client, guestlist)
 
diff --git a/va/caa_test.go b/va/caa_test.go
@@ -734,7 +734,7 @@ func TestMultiCAARechecking(t *testing.T) {
 			},
 		},
 		{
-			name:                     "functional localVA, 2 broken RVAs, no CAA records",
+			name:                     "functional localVA, 2 broken RVA, no CAA records",
 			domains:                  "present-dns-only.com",
 			expectedProbSubstring:    "During secondary CAA checking: While processing CAA",
 			expectedProbType:         probs.DNSProblem,
@@ -938,7 +938,7 @@ func TestMultiCAARechecking(t *testing.T) {
 			},
 		},
 		{
-			name:                     "1 hijacked RVA, CAA issuewild type present",
+			name:                     "1 hijacked RVA, CAA issuewild type present, 1 failure allowed",
 			domains:                  "satisfiable-wildcard.com",
 			expectedDiffLogSubstring: `RemoteSuccesses":2,"RemoteFailures":[{"VAHostname":"hijacked","Problem":{"type":"caa","detail":"While processing CAA for`,
 			localDNSClient:           caaMockDNS{},
@@ -949,7 +949,7 @@ func TestMultiCAARechecking(t *testing.T) {
 			},
 		},
 		{
-			name:                     "2 hijacked RVAs, CAA issuewild type present",
+			name:                     "2 hijacked RVAs, CAA issuewild type present, 1 failure allowed",
 			domains:                  "satisfiable-wildcard.com",
 			expectedProbSubstring:    "During secondary CAA checking: While processing CAA",
 			expectedProbType:         probs.CAAProblem,
@@ -962,7 +962,7 @@ func TestMultiCAARechecking(t *testing.T) {
 			},
 		},
 		{
-			name:                     "3 hijacked RVAs, CAA issuewild type present",
+			name:                     "3 hijacked RVAs, CAA issuewild type present, 1 failure allowed",
 			domains:                  "satisfiable-wildcard.com",
 			expectedProbSubstring:    "During secondary CAA checking: While processing CAA",
 			expectedProbType:         probs.CAAProblem,
diff --git a/va/dns_test.go b/va/dns_test.go
@@ -30,7 +30,7 @@ func TestDNSValidationEmpty(t *testing.T) {
 
 	test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{
 		"operation":      opChallAndCAA,
-		"perspective":    PrimaryPerspective,
+		"perspective":    va.perspective,
 		"challenge_type": string(core.ChallengeTypeDNS01),
 		"problem_type":   string(probs.UnauthorizedProblem),
 		"result":         fail,
diff --git a/va/va_test.go b/va/va_test.go
@@ -103,6 +103,9 @@ func createValidationRequest(domain string, challengeType core.AcmeChallenge) *v
 
 // setup returns an in-memory VA and a mock logger. The default resolver client
 // is MockClient{}, but can be overridden.
+//
+// If remoteVAs is nil, this builds a VA that acts like a remote (and does not
+// perform multi-perspective validation). Otherwise it acts like a primary.
 func setup(srv *httptest.Server, userAgent string, remoteVAs []RemoteVA, mockDNSClientOverride bdns.Client) (*ValidationAuthorityImpl, *blog.Mock) {
 	features.Reset()
 	fc := clock.NewFake()
@@ -113,6 +116,13 @@ func setup(srv *httptest.Server, userAgent string, remoteVAs []RemoteVA, mockDNS
 		userAgent = "user agent 1.0"
 	}
 
+	perspective := PrimaryPerspective
+	if len(remoteVAs) == 0 {
+		// We're being set up as a remote. Use a distinct perspective from other remotes
+		// to better simulate what prod will be like.
+		perspective = "example perspective " + core.RandomString(4)
+	}
+
 	va, err := NewValidationAuthorityImpl(
 		&bdns.MockClient{Log: logger},
 		remoteVAs,
@@ -122,9 +132,12 @@ func setup(srv *httptest.Server, userAgent string, remoteVAs []RemoteVA, mockDNS
 		fc,
 		logger,
 		accountURIPrefixes,
-		PrimaryPerspective,
+		perspective,
 		"",
 	)
+	if err != nil {
+		panic(fmt.Sprintf("Failed to create validation authority: %v", err))
+	}
 
 	if mockDNSClientOverride != nil {
 		va.dnsClient = mockDNSClientOverride
@@ -138,9 +151,6 @@ func setup(srv *httptest.Server, userAgent string, remoteVAs []RemoteVA, mockDNS
 		va.tlsPort = port
 	}
 
-	if err != nil {
-		panic(fmt.Sprintf("Failed to create validation authority: %v", err))
-	}
 	return va, logger
 }
 
@@ -255,7 +265,7 @@ func TestPerformValidationInvalid(t *testing.T) {
 	test.Assert(t, res.Problems != nil, "validation succeeded")
 	test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{
 		"operation":      opChallAndCAA,
-		"perspective":    PrimaryPerspective,
+		"perspective":    va.perspective,
 		"challenge_type": string(core.ChallengeTypeDNS01),
 		"problem_type":   string(probs.UnauthorizedProblem),
 		"result":         fail,
@@ -285,7 +295,7 @@ func TestPerformValidationValid(t *testing.T) {
 
 	test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{
 		"operation":      opChallAndCAA,
-		"perspective":    PrimaryPerspective,
+		"perspective":    va.perspective,
 		"challenge_type": string(core.ChallengeTypeDNS01),
 		"problem_type":   "",
 		"result":         pass,
@@ -312,7 +322,7 @@ func TestPerformValidationWildcard(t *testing.T) {
 
 	test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{
 		"operation":      opChallAndCAA,
-		"perspective":    PrimaryPerspective,
+		"perspective":    va.perspective,
 		"challenge_type": string(core.ChallengeTypeDNS01),
 		"problem_type":   "",
 		"result":         pass,
@@ -422,15 +432,6 @@ func TestMultiVA(t *testing.T) {
 			AllowedUAs:   map[string]bool{remoteUA1: true, remoteUA2: true},
 			ExpectedProb: unauthorized,
 		},
-		{
-			// If one out of two remote VAs fail with an internal err it should succeed
-			Name: "Local VA ok, 1/2 remote VA internal err",
-			RemoteVAs: []RemoteVA{
-				{remoteVA1, remoteUA1},
-				{brokenVA, "broken"},
-			},
-			AllowedUAs: allowedUAs,
-		},
 		{
 			// If one out of three remote VAs fails with an internal err it should succeed
 			Name: "Local VA ok, 1/3 remote VA internal err",
@@ -530,12 +531,12 @@ func TestMultiVA(t *testing.T) {
 			AllowedUAs: allowedUAs,
 		},
 		{
-			// If two remote VA cancels, it should fail
-			Name: "Local VA OK, two cancelled remote VAs",
+			// If all remote VAs cancel, it should fail
+			Name: "Local VA OK, three cancelled remote VAs",
 			RemoteVAs: []RemoteVA{
-				{remoteVA1, remoteUA1},
 				{cancelledVA, remoteUA1},
 				{cancelledVA, remoteUA2},
+				{cancelledVA, remoteUA3},
 			},
 			AllowedUAs:   allowedUAs,
 			ExpectedProb: probs.ServerInternal("During secondary validation: Remote PerformValidation RPC canceled"),
@@ -645,24 +646,28 @@ func TestMultiVAPolicy(t *testing.T) {
 	const (
 		remoteUA1 = "remote 1"
 		remoteUA2 = "remote 2"
+		remoteUA3 = "remote 3"
 		localUA   = "local 1"
 	)
-	// Forbid both remote UAs to ensure that multi-va fails
+	// Forbid all remote UAs to ensure that multi-va fails
 	allowedUAs := map[string]bool{
 		localUA:   true,
 		remoteUA1: false,
 		remoteUA2: false,
+		remoteUA3: false,
 	}
 
 	ms := httpMultiSrv(t, expectedToken, allowedUAs)
 	defer ms.Close()
 
 	remoteVA1 := setupRemote(ms.Server, remoteUA1, nil, "", "")
 	remoteVA2 := setupRemote(ms.Server, remoteUA2, nil, "", "")
+	remoteVA3 := setupRemote(ms.Server, remoteUA3, nil, "", "")
 
 	remoteVAs := []RemoteVA{
 		{remoteVA1, remoteUA1},
 		{remoteVA2, remoteUA2},
+		{remoteVA3, remoteUA3},
 	}
 
 	// Create a local test VA with the two remote VAs
@@ -681,6 +686,7 @@ func TestMultiVALogging(t *testing.T) {
 	const (
 		rva1UA  = "remote 1"
 		rva2UA  = "remote 2"
+		rva3UA  = "remote 3"
 		localUA = "local 1"
 	)
 
@@ -689,10 +695,12 @@ func TestMultiVALogging(t *testing.T) {
 
 	rva1 := setupRemote(ms.Server, rva1UA, nil, "dev-arin", "ARIN")
 	rva2 := setupRemote(ms.Server, rva2UA, nil, "dev-ripe", "RIPE")
+	rva3 := setupRemote(ms.Server, rva3UA, nil, "dev-ripe", "RIPE")
 
 	remoteVAs := []RemoteVA{
 		{rva1, rva1UA},
 		{rva2, rva2UA},
+		{rva3, rva3UA},
 	}
 	va, _ := setup(ms.Server, localUA, remoteVAs, nil)
 	req := createValidationRequest("letsencrypt.org", core.ChallengeTypeHTTP01)
