va: Check for RIR and Perspective mismatches at runtime when they're …

…provided (#7841) - Ensure the Perspective and RIR reported by each remoteVA in the *vapb.ValidationResult returned by VA.PerformValidation, matches the expected local configuration when that configuration is present. - Correct "AfriNIC" to "AFRINIC", everywhere. Part of #7819
letsencrypt · Dec 6, 2024 · 87104b0 · 87104b0
1 parent 749f9af
commit 87104b0
Show file tree

Hide file tree

Showing 9 changed files with 96 additions and 22 deletions.
diff --git a/cmd/boulder-va/main.go b/cmd/boulder-va/main.go
@@ -43,10 +43,10 @@ type RemoteVAGRPCClientConfig struct {
 	//   - RIPE
 	//   - APNIC
 	//   - LACNIC
-	//   - AfriNIC
+	//   - AFRINIC
 	//
 	// TODO(#7615): Make mandatory.
-	RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
+	RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AFRINIC"`
 }
 
 type Config struct {

diff --git a/cmd/remoteva/main.go b/cmd/remoteva/main.go
@@ -27,7 +27,7 @@ type Config struct {
 		// of RVAs deployed in the same datacenter.
 		//
 		// TODO(#7615): Make mandatory.
-		Perspective string `validate:"omitempty"`
+		Perspective string `omitempty:"omitempty"`
 
 		// RIR indicates the Regional Internet Registry where this RVA is
 		// located. This field is used to identify the RIR region from which a
@@ -38,10 +38,10 @@ type Config struct {
 		//   - RIPE
 		//   - APNIC
 		//   - LACNIC
-		//   - AfriNIC
+		//   - AFRINIC
 		//
 		// TODO(#7615): Make mandatory.
-		RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AfriNIC"`
+		RIR string `validate:"omitempty,oneof=ARIN RIPE APNIC LACNIC AFRINIC"`
 
 		// SkipGRPCClientCertVerification, when disabled as it should typically
 		// be, will cause the remoteva server (which receives gRPCs from a

diff --git a/test/config/remoteva-a.json b/test/config/remoteva-a.json
@@ -38,7 +38,9 @@
 		"accountURIPrefixes": [
 			"http://boulder.service.consul:4000/acme/reg/",
 			"http://boulder.service.consul:4001/acme/acct/"
-		]
+		],
+		"perspective": "dadaist",
+		"rir": "ARIN"
 	},
 	"syslog": {
 		"stdoutlevel": 4,

diff --git a/test/config/remoteva-b.json b/test/config/remoteva-b.json
@@ -38,7 +38,9 @@
 		"accountURIPrefixes": [
 			"http://boulder.service.consul:4000/acme/reg/",
 			"http://boulder.service.consul:4001/acme/acct/"
-		]
+		],
+		"perspective": "surrealist",
+		"rir": "RIPE"
 	},
 	"syslog": {
 		"stdoutlevel": 4,

diff --git a/test/config/remoteva-c.json b/test/config/remoteva-c.json
@@ -38,7 +38,9 @@
 		"accountURIPrefixes": [
 			"http://boulder.service.consul:4000/acme/reg/",
 			"http://boulder.service.consul:4001/acme/acct/"
-		]
+		],
+		"perspective": "cubist",
+		"rir": "ARIN"
 	},
 	"syslog": {
 		"stdoutlevel": 4,

diff --git a/test/config/va.json b/test/config/va.json
@@ -43,17 +43,23 @@
 			{
 				"serverAddress": "rva1.service.consul:9397",
 				"timeout": "15s",
-				"hostOverride": "rva1.boulder"
+				"hostOverride": "rva1.boulder",
+				"perspective": "dadaist",
+				"rir": "ARIN"
 			},
 			{
 				"serverAddress": "rva1.service.consul:9498",
 				"timeout": "15s",
-				"hostOverride": "rva1.boulder"
+				"hostOverride": "rva1.boulder",
+				"perspective": "surrealist",
+				"rir": "RIPE"
 			},
 			{
 				"serverAddress": "rva1.service.consul:9499",
 				"timeout": "15s",
-				"hostOverride": "rva1.boulder"
+				"hostOverride": "rva1.boulder",
+				"perspective": "cubist",
+				"rir": "ARIN"
 			}
 		],
 		"maxRemoteValidationFailures": 1,

diff --git a/va/caa.go b/va/caa.go
@@ -112,12 +112,19 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC
 		// It will also later be serialized in JSON, which defaults to UTF-8. Make
 		// sure it is UTF-8 clean now.
 		prob = filterProblemDetails(prob)
-		return &vapb.IsCAAValidResponse{Problem: &corepb.ProblemDetails{
-			ProblemType: string(prob.Type),
-			Detail:      replaceInvalidUTF8([]byte(prob.Detail)),
-		}}, nil
+		return &vapb.IsCAAValidResponse{
+			Problem: &corepb.ProblemDetails{
+				ProblemType: string(prob.Type),
+				Detail:      replaceInvalidUTF8([]byte(prob.Detail)),
+			},
+			Perspective: va.perspective,
+			Rir:         va.rir,
+		}, nil
 	} else {
-		return &vapb.IsCAAValidResponse{}, nil
+		return &vapb.IsCAAValidResponse{
+			Perspective: va.perspective,
+			Rir:         va.rir,
+		}, nil
 	}
 }
 

diff --git a/va/va.go b/va/va.go
@@ -241,7 +241,7 @@ func NewValidationAuthorityImpl(
 
 	for i, va1 := range remoteVAs {
 		for j, va2 := range remoteVAs {
-			// TODO(#7819): Remove the != "" check once perspective is required.
+			// TODO(#7615): Remove the != "" check once perspective is required.
 			if i != j && va1.Perspective == va2.Perspective && va1.Perspective != "" {
 				return nil, fmt.Errorf("duplicate remote VA perspective %q", va1.Perspective)
 			}
@@ -504,6 +504,19 @@ func (va *ValidationAuthorityImpl) performRemoteOperation(ctx context.Context, o
 	for _, i := range rand.Perm(remoteVACount) {
 		go func(rva RemoteVA) {
 			res, err := op(subCtx, rva, req)
+			if err != nil {
+				responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
+				return
+			}
+			// TODO(#7615): Remove the != "" checks once perspective and rir are required.
+			if (rva.Perspective != "" && res.GetPerspective() != "" && res.GetPerspective() != rva.Perspective) ||
+				(rva.RIR != "" && res.GetRir() != "" && res.GetRir() != rva.RIR) {
+				err = fmt.Errorf(
+					"Expected perspective %q (%q) but got reply from %q (%q) - misconfiguration likely", rva.Perspective, rva.RIR, res.GetPerspective(), res.GetRir(),
+				)
+				responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
+				return
+			}
 			responses <- &response{rva.Address, rva.Perspective, rva.RIR, res, err}
 		}(va.remoteVAs[i])
 	}
@@ -538,7 +551,7 @@ func (va *ValidationAuthorityImpl) performRemoteOperation(ctx context.Context, o
 			}
 			if isCAACheck {
 				// We're checking CAA, log the problem.
-				va.log.Errf("Operation on Remote VA (%s) returned a problem: %s", resp.addr, err)
+				va.log.Errf("Operation on Remote VA (%s) returned a problem: %s", resp.addr, currProb)
 			}
 		} else {
 			// The remote VA returned a successful result.

diff --git a/va/va_test.go b/va/va_test.go
@@ -160,7 +160,7 @@ func setupRemote(srv *httptest.Server, userAgent string, mockDNSClientOverride b
 	rva.perspective = perspective
 	rva.rir = rir
 
-	return RemoteClients{VAClient: &inMemVA{*rva}, CAAClient: &inMemVA{*rva}}
+	return RemoteClients{VAClient: &inMemVA{rva}, CAAClient: &inMemVA{rva}}
 }
 
 // RIRs
@@ -296,14 +296,14 @@ func (b brokenRemoteVA) IsCAAValid(_ context.Context, _ *vapb.IsCAAValidRequest,
 // ValidationAuthorityImpl rather than over the network. This lets a local
 // in-memory mock VA act like a remote VA.
 type inMemVA struct {
-	rva ValidationAuthorityImpl
+	rva *ValidationAuthorityImpl
 }
 
-func (inmem inMemVA) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest, _ ...grpc.CallOption) (*vapb.ValidationResult, error) {
+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) {
+func (inmem *inMemVA) IsCAAValid(ctx context.Context, req *vapb.IsCAAValidRequest, _ ...grpc.CallOption) (*vapb.IsCAAValidResponse, error) {
 	return inmem.rva.IsCAAValid(ctx, req)
 }
 
@@ -333,6 +333,48 @@ func TestNewValidationAuthorityImplWithDuplicateRemotes(t *testing.T) {
 	test.AssertContains(t, err.Error(), "duplicate remote VA perspective \"dadaist\"")
 }
 
+func TestPerformValidationWithMismatchedRemoteVAPerspectives(t *testing.T) {
+	mismatched1 := RemoteVA{
+		RemoteClients: setupRemote(nil, "", nil, "dadaist", arin),
+		Perspective:   "baroque",
+		RIR:           arin,
+	}
+	mismatched2 := RemoteVA{
+		RemoteClients: setupRemote(nil, "", nil, "impressionist", ripe),
+		Perspective:   "minimalist",
+		RIR:           ripe,
+	}
+	remoteVAs := setupRemotes([]remoteConf{{rir: ripe}}, nil)
+	remoteVAs = append(remoteVAs, mismatched1, mismatched2)
+	va, mockLog := setup(nil, "", remoteVAs, nil)
+
+	req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01)
+	res, _ := va.PerformValidation(context.Background(), req)
+	test.AssertNotNil(t, res.GetProblem(), "validation succeeded with mismatched remote VA perspectives")
+	test.AssertEquals(t, len(mockLog.GetAllMatching("Expected perspective")), 2)
+}
+
+func TestPerformValidationWithMismatchedRemoteVARIRs(t *testing.T) {
+	mismatched1 := RemoteVA{
+		RemoteClients: setupRemote(nil, "", nil, "dadaist", arin),
+		Perspective:   "dadaist",
+		RIR:           ripe,
+	}
+	mismatched2 := RemoteVA{
+		RemoteClients: setupRemote(nil, "", nil, "impressionist", ripe),
+		Perspective:   "impressionist",
+		RIR:           arin,
+	}
+	remoteVAs := setupRemotes([]remoteConf{{rir: ripe}}, nil)
+	remoteVAs = append(remoteVAs, mismatched1, mismatched2)
+	va, mockLog := setup(nil, "", remoteVAs, nil)
+
+	req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01)
+	res, _ := va.PerformValidation(context.Background(), req)
+	test.AssertNotNil(t, res.GetProblem(), "validation succeeded with mismatched remote VA perspectives")
+	test.AssertEquals(t, len(mockLog.GetAllMatching("Expected perspective")), 2)
+}
+
 func TestValidateMalformedChallenge(t *testing.T) {
 	va, _ := setup(nil, "", nil, nil)