From ffd3f65c18b463360c578af3d36e248c8c6db487 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 10:34:39 -0500 Subject: [PATCH 01/10] Remove Perspective and RIR from ValidationRecords --- core/objects.go | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/core/objects.go b/core/objects.go index 01d964c228d..6b4e260e55b 100644 --- a/core/objects.go +++ b/core/objects.go @@ -150,24 +150,6 @@ type ValidationRecord struct { // lookup for AddressUsed. During recursive A and AAAA lookups, a record may // instead look like A:host:port or AAAA:host:port ResolverAddrs []string `json:"resolverAddrs,omitempty"` - - // Perspective uniquely identifies the Network Perspective used to perform - // the validation, as specified in BRs Section 5.4.1, Requirement 2.7 - // ("Multi-Perspective Issuance Corroboration attempts from each Network - // Perspective"). It should uniquely identify either the Primary Perspective - // (VA) or a group of RVAs deployed in the same datacenter. - Perspective string `json:"perspective,omitempty"` - - // RIR indicates the Regional Internet Registry where this RVA is located. - // This field is used to identify the RIR region from which a given - // validation was performed, as specified in the "Phased Implementation - // Timeline" in BRs Section 3.2.2.9. It must be one of the following values: - // - ARIN - // - RIPE - // - APNIC - // - LACNIC - // - AfriNIC - RIR string `json:"rir,omitempty"` } // Challenge is an aggregate of all data needed for any challenges. From a03bd3e0b3fc1c5787ff8fc7cd2f3b47a25fd251 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 10:39:19 -0500 Subject: [PATCH 02/10] Make ValidationResultToPB Perspective and RIR aware --- grpc/pb-marshalling.go | 8 +++++--- grpc/pb-marshalling_test.go | 4 +++- ra/ra.go | 2 +- va/va.go | 4 ++-- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/grpc/pb-marshalling.go b/grpc/pb-marshalling.go index 417e5cc6191..c05ec9ecd2a 100644 --- a/grpc/pb-marshalling.go +++ b/grpc/pb-marshalling.go @@ -179,7 +179,7 @@ func PBToValidationRecord(in *corepb.ValidationRecord) (record core.ValidationRe }, nil } -func ValidationResultToPB(records []core.ValidationRecord, prob *probs.ProblemDetails) (*vapb.ValidationResult, error) { +func ValidationResultToPB(records []core.ValidationRecord, prob *probs.ProblemDetails, perspective, rir string) (*vapb.ValidationResult, error) { recordAry := make([]*corepb.ValidationRecord, len(records)) var err error for i, v := range records { @@ -193,8 +193,10 @@ func ValidationResultToPB(records []core.ValidationRecord, prob *probs.ProblemDe return nil, err } return &vapb.ValidationResult{ - Records: recordAry, - Problems: marshalledProbs, + Records: recordAry, + Problems: marshalledProbs, + Perspective: perspective, + Rir: rir, }, nil } diff --git a/grpc/pb-marshalling_test.go b/grpc/pb-marshalling_test.go index 332a3cb8317..55d951467bd 100644 --- a/grpc/pb-marshalling_test.go +++ b/grpc/pb-marshalling_test.go @@ -154,9 +154,11 @@ func TestValidationResult(t *testing.T) { result := []core.ValidationRecord{vrA, vrB} prob := &probs.ProblemDetails{Type: probs.TLSProblem, Detail: "asd", HTTPStatus: 200} - pb, err := ValidationResultToPB(result, prob) + pb, err := ValidationResultToPB(result, prob, "surreal", "ARIN") test.AssertNotError(t, err, "ValidationResultToPB failed") test.Assert(t, pb != nil, "Returned vapb.ValidationResult is nil") + test.AssertEquals(t, pb.Perspective, "surreal") + test.AssertEquals(t, pb.Rir, "ARIN") reconResult, reconProb, err := pbToValidationResult(pb) test.AssertNotError(t, err, "pbToValidationResult failed") diff --git a/ra/ra.go b/ra/ra.go index 765456e8838..b4eaa4be009 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1792,7 +1792,7 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI } else { expires = ra.clk.Now().Add(ra.authorizationLifetime) } - vr, err := bgrpc.ValidationResultToPB(challenge.ValidationRecord, challenge.Error) + vr, err := bgrpc.ValidationResultToPB(challenge.ValidationRecord, challenge.Error, "", "") if err != nil { return err } diff --git a/va/va.go b/va/va.go index 34cc3b68b01..05070771dca 100644 --- a/va/va.go +++ b/va/va.go @@ -701,7 +701,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v if err != nil { logEvent.InternalError = err.Error() prob = detailedError(err) - return bgrpc.ValidationResultToPB(records, filterProblemDetails(prob)) + return bgrpc.ValidationResultToPB(records, filterProblemDetails(prob), va.perspective, va.rir) } // Do remote validation. We do this after local validation is complete to @@ -710,5 +710,5 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v // own validation records, and it's not helpful to present multiple large // errors to the end user. prob = va.performRemoteValidation(ctx, req) - return bgrpc.ValidationResultToPB(records, filterProblemDetails(prob)) + return bgrpc.ValidationResultToPB(records, filterProblemDetails(prob), va.perspective, va.rir) } From 27e1ee42fd83c507f0093201eae0b5e1537f669a Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 11:34:14 -0500 Subject: [PATCH 03/10] Update comment for VA.PerformValidation --- va/va.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/va/va.go b/va/va.go index 05070771dca..16499a2e007 100644 --- a/va/va.go +++ b/va/va.go @@ -623,9 +623,14 @@ func (va *ValidationAuthorityImpl) performLocalValidation( return records, nil } -// PerformValidation validates the challenge for the domain in the request. -// The returned result will always contain a list of validation records, even -// when it also contains a problem. +// PerformValidation conducts a local Domain Control Validation (DCV) and CAA +// check for the specified challenge and dnsName. When invoked on the primary +// Validation Authority (VA) and the local validation succeeds, it also performs +// DCV and CAA checks using the configured remote VAs. The method returns a +// ValidationResult and an error if the validation fails. The ValidationResult +// always includes a list of ValidationRecords, even when it also contains a +// Problem. PerformValidation does NOT implement Multi-Perspective Issuance +// Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { if core.IsAnyNilOrZero(req, req.DnsName, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") From ef3da91121b1a6276310a30af76d983f0e76f775 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 11:45:48 -0500 Subject: [PATCH 04/10] Make verificationRequestEvent more like doDCVAuditLog --- va/caa.go | 8 ++++---- va/va.go | 27 ++++++++++++++------------- va/va_test.go | 10 +++++----- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/va/caa.go b/va/caa.go index da35eb2f4bd..7e02c4ca3d6 100644 --- a/va/caa.go +++ b/va/caa.go @@ -36,10 +36,10 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC return nil, berrors.InternalServerError("incomplete IsCAAValid request") } logEvent := verificationRequestEvent{ - // TODO(#7061) Plumb req.Authz.Id as "ID:" through from the RA to + // TODO(#7061) Plumb req.Authz.Id as "AuthzID:" through from the RA to // correlate which authz triggered this request. - Requester: req.AccountURIID, - Hostname: req.Domain, + Requester: req.AccountURIID, + Identifier: req.Domain, } challType := core.AcmeChallenge(req.ValidationMethod) @@ -76,7 +76,7 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC va.observeLatency(opCAA, allPerspectives, string(challType), probType, outcome, va.clk.Since(start)) } // Log the total check latency. - logEvent.ValidationLatency = va.clk.Since(start).Round(time.Millisecond).Seconds() + logEvent.Latency = va.clk.Since(start).Round(time.Millisecond).Seconds() va.log.AuditObject("CAA check result", logEvent) }() diff --git a/va/va.go b/va/va.go index 16499a2e007..9041554a8a6 100644 --- a/va/va.go +++ b/va/va.go @@ -281,15 +281,16 @@ func maxAllowedFailures(perspectiveCount int) int { return 2 } -// Used for audit logging +// verificationRequestEvent is logged once for each validation attempt. Its +// fields are exported for logging purposes. type verificationRequestEvent struct { - ID string `json:",omitempty"` - Requester int64 `json:",omitempty"` - Hostname string `json:",omitempty"` - Challenge core.Challenge `json:",omitempty"` - ValidationLatency float64 - Error string `json:",omitempty"` - InternalError string `json:",omitempty"` + AuthzID string + Requester int64 + Identifier string + Challenge core.Challenge + Error string `json:",omitempty"` + InternalError string `json:",omitempty"` + Latency float64 } // ipError is an error type used to pass though the IP address of the remote @@ -653,10 +654,10 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v var localLatency time.Duration start := va.clk.Now() logEvent := verificationRequestEvent{ - ID: req.Authz.Id, - Requester: req.Authz.RegID, - Hostname: req.DnsName, - Challenge: chall, + AuthzID: req.Authz.Id, + Requester: req.Authz.RegID, + Identifier: req.DnsName, + Challenge: chall, } defer func() { probType := "" @@ -678,7 +679,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v } // Log the total validation latency. - logEvent.ValidationLatency = time.Since(start).Round(time.Millisecond).Seconds() + logEvent.Latency = time.Since(start).Round(time.Millisecond).Seconds() va.log.AuditObject("Validation result", logEvent) }() diff --git a/va/va_test.go b/va/va_test.go index 83a9de15189..6391a3922df 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -304,8 +304,8 @@ func TestPerformValidationValid(t *testing.T) { if len(resultLog) != 1 { t.Fatalf("Wrong number of matching lines for 'Validation result'") } - if !strings.Contains(resultLog[0], `"Hostname":"good-dns01.com"`) { - t.Error("PerformValidation didn't log validation hostname.") + if !strings.Contains(resultLog[0], `"Identifier":"good-dns01.com"`) { + t.Error("PerformValidation didn't log validation identifier.") } } @@ -332,9 +332,9 @@ func TestPerformValidationWildcard(t *testing.T) { t.Fatalf("Wrong number of matching lines for 'Validation result'") } - // We expect that the top level Hostname reflect the wildcard name - if !strings.Contains(resultLog[0], `"Hostname":"*.good-dns01.com"`) { - t.Errorf("PerformValidation didn't log correct validation hostname.") + // We expect that the top level Identifier reflect the wildcard name + if !strings.Contains(resultLog[0], `"Identifier":"*.good-dns01.com"`) { + t.Errorf("PerformValidation didn't log correct validation identifier.") } // We expect that the ValidationRecord contain the correct non-wildcard // hostname that was validated From 8e8a5bbc8949cf2b152a813dd06c57563f0057c8 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 12:30:16 -0500 Subject: [PATCH 05/10] Improve method comment --- va/va.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/va/va.go b/va/va.go index 9041554a8a6..897a3654a12 100644 --- a/va/va.go +++ b/va/va.go @@ -627,11 +627,12 @@ func (va *ValidationAuthorityImpl) performLocalValidation( // PerformValidation conducts a local Domain Control Validation (DCV) and CAA // check for the specified challenge and dnsName. When invoked on the primary // Validation Authority (VA) and the local validation succeeds, it also performs -// DCV and CAA checks using the configured remote VAs. The method returns a -// ValidationResult and an error if the validation fails. The ValidationResult -// always includes a list of ValidationRecords, even when it also contains a -// Problem. PerformValidation does NOT implement Multi-Perspective Issuance -// Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. +// DCV and CAA checks using the configured remote VAs. An error is only returned +// in cases of internal error (e.g., network failure). When a requester fails a +// DCV or CAA check, a corresponding problem is included in the ValidationResult. +// The ValidationResult always includes a list of ValidationRecords, even when +// it also contains a Problem. This method does NOT implement Multi-Perspective +// Issuance Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { if core.IsAnyNilOrZero(req, req.DnsName, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") From 263875bd5cf811bcb90bc97d0c3be1a69131062c Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 17:27:01 -0500 Subject: [PATCH 06/10] Update logging to match remoteDoDCV --- va/va.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/va/va.go b/va/va.go index 897a3654a12..26ab59c7903 100644 --- a/va/va.go +++ b/va/va.go @@ -487,10 +487,10 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( failed = append(failed, resp.addr) if canceled.Is(resp.err) { - currProb = probs.ServerInternal("Remote PerformValidation RPC canceled") + currProb = probs.ServerInternal("Secondary domain validation RPC canceled") } else { va.log.Errf("Remote VA %q.PerformValidation failed: %s", resp.addr, resp.err) - currProb = probs.ServerInternal("Remote PerformValidation RPC failed") + currProb = probs.ServerInternal("Secondary domain validation RPC failed") } } else if resp.result.Problems != nil { // The remote VA returned a problem. @@ -500,7 +500,7 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( currProb, err = bgrpc.PBToProblemDetails(resp.result.Problems) if err != nil { va.log.Errf("Remote VA %q.PerformValidation returned malformed problem: %s", resp.addr, err) - currProb = probs.ServerInternal("Remote PerformValidation RPC returned malformed result") + currProb = probs.ServerInternal("Secondary domain validation RPC returned malformed result") } } else { // The remote VA returned a successful result. @@ -518,7 +518,7 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( } if len(failed) > va.maxRemoteFailures { // Too many failed responses to reach quorum. - firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail) + firstProb.Detail = fmt.Sprintf("During secondary domain validation: %s", firstProb.Detail) return firstProb } From 94177a07462faacb17884fe053dfac0043737a06 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 17:31:10 -0500 Subject: [PATCH 07/10] Address comment comment --- va/va.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/va/va.go b/va/va.go index 26ab59c7903..8dc61b3ad32 100644 --- a/va/va.go +++ b/va/va.go @@ -627,11 +627,12 @@ func (va *ValidationAuthorityImpl) performLocalValidation( // PerformValidation conducts a local Domain Control Validation (DCV) and CAA // check for the specified challenge and dnsName. When invoked on the primary // Validation Authority (VA) and the local validation succeeds, it also performs -// DCV and CAA checks using the configured remote VAs. An error is only returned -// in cases of internal error (e.g., network failure). When a requester fails a -// DCV or CAA check, a corresponding problem is included in the ValidationResult. -// The ValidationResult always includes a list of ValidationRecords, even when -// it also contains a Problem. This method does NOT implement Multi-Perspective +// DCV and CAA checks using the configured remote VAs. Failed validations are +// indicated by a non-nil Problems in the returned ValidationResult. +// PerformValidation returns error only for internal logic errors (and the +// client may receive errors from gRPC in the event of a communication problem). +// ValidationResult always includes a list of ValidationRecords, even when it +// also contains Problems. This method does NOT implement Multi-Perspective // Issuance Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { if core.IsAnyNilOrZero(req, req.DnsName, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { From b7dfe6c14fc40b9f8e0ae60a07fd12ecae367fb9 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 17:37:02 -0500 Subject: [PATCH 08/10] Collapse response struct --- va/va.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/va/va.go b/va/va.go index 8dc61b3ad32..7639c9368ca 100644 --- a/va/va.go +++ b/va/va.go @@ -466,11 +466,7 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( for _, i := range rand.Perm(remoteVACount) { go func(rva RemoteVA, out chan<- *response) { res, err := rva.PerformValidation(ctx, req) - out <- &response{ - addr: rva.Address, - result: res, - err: err, - } + out <- &response{rva.Address, res, err} }(va.remoteVAs[i], responses) } From b4ea1b4be007fdc54f152e82b1abc1dfdf3bd982 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 18:04:18 -0500 Subject: [PATCH 09/10] Fix logs in tests. --- test/v2_integration.py | 4 ++-- va/va_test.go | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/v2_integration.py b/test/v2_integration.py index 230d63ba289..c129eef1358 100644 --- a/test/v2_integration.py +++ b/test/v2_integration.py @@ -1083,8 +1083,8 @@ def test_http_multiva_threshold_fail(): raise(Exception("no HTTP-01 challenge in failed authz")) if httpChall.error.typ != "urn:ietf:params:acme:error:unauthorized": raise(Exception("expected unauthorized prob, found {0}".format(httpChall.error.typ))) - if not httpChall.error.detail.startswith("During secondary validation: "): - raise(Exception("expected 'During secondary validation' problem detail, found {0}".format(httpChall.error.detail))) + if not httpChall.error.detail.startswith("During secondary domain validation: "): + raise(Exception("expected 'During secondary domain validation' problem detail, found {0}".format(httpChall.error.detail))) class FakeH2ServerHandler(socketserver.BaseRequestHandler): """ diff --git a/va/va_test.go b/va/va_test.go index 6391a3922df..e19731cd4a5 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -451,7 +451,7 @@ func TestMultiVA(t *testing.T) { {brokenVA, "broken"}, }, AllowedUAs: allowedUAs, - ExpectedProb: probs.ServerInternal("During secondary validation: Remote PerformValidation RPC failed"), + ExpectedProb: probs.ServerInternal("During secondary domain validation: Secondary domain validation RPC failed"), // The real failure cause should be logged ExpectedLog: expectedInternalErrLine, }, @@ -478,7 +478,7 @@ func TestMultiVA(t *testing.T) { {brokenVA, "broken"}, }, AllowedUAs: allowedUAs, - ExpectedProb: probs.ServerInternal("During secondary validation: Remote PerformValidation RPC failed"), + ExpectedProb: probs.ServerInternal("During secondary domain validation: Secondary domain validation RPC failed"), // The real failure cause should be logged ExpectedLog: expectedInternalErrLine, }, @@ -507,7 +507,7 @@ func TestMultiVA(t *testing.T) { {brokenVA, "broken"}, }, AllowedUAs: allowedUAs, - ExpectedProb: probs.ServerInternal("During secondary validation: Remote PerformValidation RPC failed"), + ExpectedProb: probs.ServerInternal("During secondary domain validation: Secondary domain validation RPC failed"), // The real failure cause should be logged ExpectedLog: expectedInternalErrLine, }, @@ -517,7 +517,7 @@ func TestMultiVA(t *testing.T) { RemoteVAs: remoteVAs, AllowedUAs: map[string]bool{localUA: true, remoteUA2: true}, ExpectedProb: probs.Unauthorized(fmt.Sprintf( - `During secondary validation: The key authorization file from the server did not match this challenge. Expected %q (got "???")`, + `During secondary domain validation: The key authorization file from the server did not match this challenge. Expected %q (got "???")`, expectedKeyAuthorization)), }, { @@ -539,7 +539,7 @@ func TestMultiVA(t *testing.T) { {cancelledVA, remoteUA3}, }, AllowedUAs: allowedUAs, - ExpectedProb: probs.ServerInternal("During secondary validation: Remote PerformValidation RPC canceled"), + ExpectedProb: probs.ServerInternal("During secondary domain validation: Secondary domain validation RPC canceled"), }, { // With the local and remote VAs seeing diff problems, we expect a problem. @@ -547,7 +547,7 @@ func TestMultiVA(t *testing.T) { RemoteVAs: remoteVAs, AllowedUAs: map[string]bool{localUA: true}, ExpectedProb: probs.Unauthorized(fmt.Sprintf( - `During secondary validation: The key authorization file from the server did not match this challenge. Expected %q (got "???")`, + `During secondary domain validation: The key authorization file from the server did not match this challenge. Expected %q (got "???")`, expectedKeyAuthorization)), }, } From ce14a93aee07480cd00c3a25e69e5084e292fb50 Mon Sep 17 00:00:00 2001 From: Samantha Date: Tue, 19 Nov 2024 19:10:04 -0500 Subject: [PATCH 10/10] No longer take channel argument in go func() --- va/va.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/va/va.go b/va/va.go index 7639c9368ca..f96008eb79b 100644 --- a/va/va.go +++ b/va/va.go @@ -464,10 +464,10 @@ func (va *ValidationAuthorityImpl) performRemoteValidation( responses := make(chan *response, remoteVACount) for _, i := range rand.Perm(remoteVACount) { - go func(rva RemoteVA, out chan<- *response) { + go func(rva RemoteVA) { res, err := rva.PerformValidation(ctx, req) - out <- &response{rva.Address, res, err} - }(va.remoteVAs[i], responses) + responses <- &response{rva.Address, res, err} + }(va.remoteVAs[i]) } required := remoteVACount - va.maxRemoteFailures