Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VA: Remove logging of RIR and Perspective #7818

Merged
merged 2 commits into from
Nov 15, 2024
Merged

VA: Remove logging of RIR and Perspective #7818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b432c90
VA: Remove logging of RIR and Perspective
beautifulentropy Nov 15, 2024
278798f
Fix unsed parameter
beautifulentropy Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions va/caa_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down
14 changes: 0 additions & 14 deletions va/va.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
Expand Down
55 changes: 15 additions & 40 deletions va/va_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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"},
Expand Down