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.

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

va: log MPIC summaries #7817

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
65 changes: 57 additions & 8 deletions va/va.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,15 +264,39 @@ func NewValidationAuthorityImpl(
return va, nil
}

// mpicSummary is returned by remoteDoDCV and contains a summary of the
// validation results for logging purposes.
type mpicSummary struct {
// PassedPerspectives are the perspectives that PassedPerspectives validation.
PassedPerspectives []string `json:"passedPerspectives"`

// FailedPerspectives are the perspectives that FailedPerspectives validation.
// Note: If the failure was based on a gRPC error like a timeout, this will
// contain a hostname instead of a perspective.
FailedPerspectives []string `json:"failedPerspectives"`

// PassedRIRs are the Regional Internet Registries that passing
// perspectives belonged to.
PassedRIRs []string `json:"passedRIRs"`

// QuorumResult is the Multi-Perspective Issuance Corroboration quorum
// result, per BRs Section 5.4.1, Requirement 2.7 (i.e., "3/4" which should
// be interpreted as "Three (3) out of four (4) attempted Network
// Perspectives corroborated the determinations made by the Primary Network
// Perspective).
QuorumResult string `json:"quorumResult"`
}

// Used for audit logging
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"`
Error string `json:",omitempty"`
InternalError string `json:",omitempty"`
MPICSummary *mpicSummary `json:",omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MPIC summary must always be logged, even if it's empty because we caught a local failure. Our logs don't distinguish between a local problem and a remote problem so it's useful to positively state that we did not attempt MPIC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will change this to not omitempty.

}

// ipError is an error type used to pass though the IP address of the remote
Expand Down Expand Up @@ -429,13 +453,16 @@ func (va *ValidationAuthorityImpl) observeLatency(op, perspective, challType, pr
// returns a problem if too many remote perspectives failed to corroborate
// domain control, or nil if enough succeeded to surpass our corroboration
// threshold.
//
// Returns an mpicSummary and a ProblemDetails. The mpicSummary will have
// meaningful contents even if the ProblemDetails is non-nil.
func (va *ValidationAuthorityImpl) performRemoteValidation(
ctx context.Context,
req *vapb.PerformValidationRequest,
) *probs.ProblemDetails {
) (*mpicSummary, *probs.ProblemDetails) {
remoteVACount := len(va.remoteVAs)
if remoteVACount == 0 {
return nil
return nil, nil
}

type response struct {
Expand All @@ -459,6 +486,7 @@ func (va *ValidationAuthorityImpl) performRemoteValidation(
required := remoteVACount - va.maxRemoteFailures
var passed []string
var failed []string
var passedRIRs []string
var firstProb *probs.ProblemDetails

for resp := range responses {
Expand Down Expand Up @@ -487,6 +515,7 @@ func (va *ValidationAuthorityImpl) performRemoteValidation(
} else {
// The remote VA returned a successful result.
passed = append(passed, resp.result.Perspective)
passedRIRs = append(passedRIRs, resp.result.Rir)
}

if firstProb == nil && currProb != nil {
Expand All @@ -496,12 +525,12 @@ func (va *ValidationAuthorityImpl) performRemoteValidation(

if len(passed) >= required {
// Enough successful responses to reach quorum.
return nil
return va.summarizeMPIC(passed, failed, passedRIRs), nil
}
if len(failed) > va.maxRemoteFailures {
// Too many failed responses to reach quorum.
firstProb.Detail = fmt.Sprintf("During secondary validation: %s", firstProb.Detail)
return firstProb
return va.summarizeMPIC(passed, failed, passedRIRs), firstProb
}

// If we somehow haven't returned early, we need to break the loop once all
Expand All @@ -513,7 +542,25 @@ func (va *ValidationAuthorityImpl) performRemoteValidation(

// This condition should not occur - it indicates the passed/failed counts
// neither met the required threshold nor the maxRemoteFailures threshold.
return probs.ServerInternal("Too few remote PerformValidation RPC results")
return va.summarizeMPIC(passed, failed, passedRIRs), probs.ServerInternal("Too few remote PerformValidation RPC results")
}

func (va *ValidationAuthorityImpl) summarizeMPIC(passed, failed, passedRIRs []string) *mpicSummary {
if passed == nil {
passed = []string{}
}
if failed == nil {
failed = []string{}
}
if passedRIRs == nil {
passedRIRs = []string{}
}

return &mpicSummary{
PassedPerspectives: passed,
FailedPerspectives: failed,
PassedRIRs: passedRIRs,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're collecting up the RIRs but we're not ensuring that passing validations came from at least two unique RIRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true. My goal here was to add logging, not enforcement.

}
}

// logRemoteResults is called by `processRemoteCAAResults` when the
Expand Down Expand Up @@ -692,6 +739,8 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v
// singular problem, because the remote VAs have already audit-logged their
// own validation records, and it's not helpful to present multiple large
// errors to the end user.
prob = va.performRemoteValidation(ctx, req)
mpicSummary, prob := va.performRemoteValidation(ctx, req)
logEvent.MPICSummary = mpicSummary

return bgrpc.ValidationResultToPB(records, filterProblemDetails(prob))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bgrpc.ValidationResultToPB must be modified to pass through the RIR and Perspective or the Primary VA will just get back empty strings.

}
Loading