From 2502113ac38a36d1b5a7729f3cee58bf359dab4d Mon Sep 17 00:00:00 2001 From: Samantha Frank Date: Fri, 15 Nov 2024 14:54:03 -0500 Subject: [PATCH] VA: Remove logging of RIR and Perspective (#7818) --- va/caa_test.go | 8 ++++---- va/va.go | 14 ------------- va/va_test.go | 55 ++++++++++++++------------------------------------ 3 files changed, 19 insertions(+), 58 deletions(-) diff --git a/va/caa_test.go b/va/caa_test.go index 477d3b84b3d..9b9789d57b8 100644 --- a/va/caa_test.go +++ b/va/caa_test.go @@ -589,7 +589,7 @@ func (b caaBrokenDNS) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, s } func TestDisabledMultiCAARechecking(t *testing.T) { - brokenRVA, _ := setupRemote(nil, "broken", caaBrokenDNS{}, "", "") + brokenRVA := setupRemote(nil, "broken", caaBrokenDNS{}, "", "") remoteVAs := []RemoteVA{{brokenRVA, "broken"}} va, _ := setup(nil, 0, "local", remoteVAs, nil) @@ -663,10 +663,10 @@ func TestMultiCAARechecking(t *testing.T) { brokenUA = "broken" hijackedUA = "hijacked" ) - remoteVA, _ := setupRemote(nil, remoteUA, nil, "", "") - brokenVA, _ := setupRemote(nil, brokenUA, caaBrokenDNS{}, "", "") + remoteVA := setupRemote(nil, remoteUA, nil, "", "") + brokenVA := setupRemote(nil, brokenUA, caaBrokenDNS{}, "", "") // Returns incorrect results - hijackedVA, _ := setupRemote(nil, hijackedUA, caaHijackedDNS{}, "", "") + hijackedVA := setupRemote(nil, hijackedUA, caaHijackedDNS{}, "", "") testCases := []struct { name string diff --git a/va/va.go b/va/va.go index 17c03cf6e01..994110a08c2 100644 --- a/va/va.go +++ b/va/va.go @@ -322,8 +322,6 @@ type verificationRequestEvent struct { ValidationLatency float64 Error string `json:",omitempty"` InternalError string `json:",omitempty"` - Perspective string `json:",omitempty"` - RIR string `json:",omitempty"` } // ipError is an error type used to pass though the IP address of the remote @@ -678,18 +676,6 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v logEvent.Challenge.Status = core.StatusValid } - if va.perspective != "" && va.perspective != PrimaryPerspective { - // This validation was performed by a remote VA. According to the - // requirements in section 5.4.1 (2) vii of the BRs we need to log - // the perspective used. Additionally, we'll log the RIR where this - // RVA is located. - // - // TODO(#7615): Make these fields mandatory for non-Primary - // perspectives and remove the (va.perspective != "") check. - logEvent.Perspective = va.perspective - logEvent.RIR = va.rir - } - va.metrics.localValidationTime.With(prometheus.Labels{ "type": string(logEvent.Challenge.Type), "result": string(logEvent.Challenge.Status), diff --git a/va/va_test.go b/va/va_test.go index 8a5c07776a2..90a949766af 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -148,12 +148,12 @@ func setup(srv *httptest.Server, maxRemoteFailures int, userAgent string, remote return va, logger } -func setupRemote(srv *httptest.Server, userAgent string, mockDNSClientOverride bdns.Client, perspective, rir string) (RemoteClients, *blog.Mock) { - rva, log := setup(srv, 0, userAgent, nil, mockDNSClientOverride) +func setupRemote(srv *httptest.Server, userAgent string, mockDNSClientOverride bdns.Client, perspective, rir string) RemoteClients { + rva, _ := setup(srv, 0, userAgent, nil, mockDNSClientOverride) rva.perspective = perspective rva.rir = rir - return RemoteClients{VAClient: &inMemVA{*rva}, CAAClient: &inMemVA{*rva}}, log + return RemoteClients{VAClient: &inMemVA{*rva}, CAAClient: &inMemVA{*rva}} } type multiSrv struct { @@ -373,8 +373,8 @@ func TestMultiVA(t *testing.T) { ms := httpMultiSrv(t, expectedToken, allowedUAs) defer ms.Close() - remoteVA1, _ := setupRemote(ms.Server, remoteUA1, nil, "", "") - remoteVA2, _ := setupRemote(ms.Server, remoteUA2, nil, "", "") + remoteVA1 := setupRemote(ms.Server, remoteUA1, nil, "", "") + remoteVA2 := setupRemote(ms.Server, remoteUA2, nil, "", "") remoteVAs := []RemoteVA{ {remoteVA1, remoteUA1}, {remoteVA2, remoteUA2}, @@ -511,8 +511,8 @@ func TestMultiVAEarlyReturn(t *testing.T) { ms := httpMultiSrv(t, expectedToken, allowedUAs) defer ms.Close() - remoteVA1, _ := setupRemote(ms.Server, remoteUA1, nil, "", "") - remoteVA2, _ := setupRemote(ms.Server, remoteUA2, nil, "", "") + remoteVA1 := setupRemote(ms.Server, remoteUA1, nil, "", "") + remoteVA2 := setupRemote(ms.Server, remoteUA2, nil, "", "") remoteVAs := []RemoteVA{ {remoteVA1, remoteUA1}, @@ -561,8 +561,8 @@ func TestMultiVAPolicy(t *testing.T) { ms := httpMultiSrv(t, expectedToken, allowedUAs) defer ms.Close() - remoteVA1, _ := setupRemote(ms.Server, remoteUA1, nil, "", "") - remoteVA2, _ := setupRemote(ms.Server, remoteUA2, nil, "", "") + remoteVA1 := setupRemote(ms.Server, remoteUA1, nil, "", "") + remoteVA2 := setupRemote(ms.Server, remoteUA2, nil, "", "") remoteVAs := []RemoteVA{ {remoteVA1, remoteUA1}, @@ -591,43 +591,18 @@ func TestMultiVALogging(t *testing.T) { ms := httpMultiSrv(t, expectedToken, map[string]bool{localUA: true, rva1UA: true, rva2UA: true}) defer ms.Close() - rva1, rva1Log := setupRemote(ms.Server, rva1UA, nil, "dev-arin", "ARIN") - rva2, rva2Log := setupRemote(ms.Server, rva2UA, nil, "dev-ripe", "RIPE") + rva1 := setupRemote(ms.Server, rva1UA, nil, "dev-arin", "ARIN") + rva2 := setupRemote(ms.Server, rva2UA, nil, "dev-ripe", "RIPE") remoteVAs := []RemoteVA{ {rva1, rva1UA}, {rva2, rva2UA}, } - va, vaLog := setup(ms.Server, 0, localUA, remoteVAs, nil) + va, _ := setup(ms.Server, 0, localUA, remoteVAs, nil) req := createValidationRequest("letsencrypt.org", core.ChallengeTypeHTTP01) res, err := va.PerformValidation(ctx, req) test.Assert(t, res.Problems == nil, fmt.Sprintf("validation failed with: %#v", res.Problems)) test.AssertNotError(t, err, "performing validation") - - // We do not log perspective or RIR for the local VAs. - // We expect these log lines to be available immediately. - test.Assert(t, len(vaLog.GetAllMatching(`"Perspective"`)) == 0, "expected no logged perspective for primary") - test.Assert(t, len(vaLog.GetAllMatching(`"RIR"`)) == 0, "expected no logged RIR for primary") - - // We do log perspective and RIR for the remote VAs. - // - // Because the remote VAs are operating on different goroutines, we aren't guaranteed their - // log lines have arrived yet. Give it a few tries. - for i := 0; i < 10; i++ { - if len(rva1Log.GetAllMatching(`"Perspective":"dev-arin"`)) >= 1 && - len(rva1Log.GetAllMatching(`"RIR":"ARIN"`)) >= 1 && - len(rva2Log.GetAllMatching(`"Perspective":"dev-ripe"`)) >= 1 && - len(rva2Log.GetAllMatching(`"RIR":"RIPE"`)) >= 1 { - break - } - if i == 9 { - t.Logf("VA:\n%s\n", strings.Join(vaLog.GetAll(), "\n")) - t.Logf("RVA 1:\n%s\n", strings.Join(rva1Log.GetAll(), "\n")) - t.Logf("RVA 2:\n%s\n", strings.Join(rva2Log.GetAll(), "\n")) - t.Errorf("expected perspective and RIR logs for remote VAs, but they never arrived") - } - time.Sleep(100 * time.Millisecond) - } } func TestDetailedError(t *testing.T) { @@ -684,9 +659,9 @@ func TestDetailedError(t *testing.T) { func TestLogRemoteDifferentials(t *testing.T) { // Create some remote VAs - remoteVA1, _ := setupRemote(nil, "remote 1", nil, "", "") - remoteVA2, _ := setupRemote(nil, "remote 2", nil, "", "") - remoteVA3, _ := setupRemote(nil, "remote 3", nil, "", "") + 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"},