From 94bcebd658b4a45a071faa40f50000585f8216e1 Mon Sep 17 00:00:00 2001
From: Daniel McCarney <daniel@binaryparadox.net>
Date: Mon, 27 Aug 2018 12:20:54 -0400
Subject: [PATCH] VA: Ignore cancelled errs from remote VAs. (#3827)

If the context provided to a remote VA's `PerformValidation` is
cancelled we should not treat the returned context cancelled error as an
unexpected error and should instead ignore it as an expected result.
---
 va/va.go      | 11 +++++++++--
 va/va_test.go | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/va/va.go b/va/va.go
index 71e64eda2..b4269f443 100644
--- a/va/va.go
+++ b/va/va.go
@@ -27,6 +27,7 @@ import (
 	"golang.org/x/net/context"
 
 	"github.com/letsencrypt/boulder/bdns"
+	"github.com/letsencrypt/boulder/canceled"
 	"github.com/letsencrypt/boulder/cmd"
 	"github.com/letsencrypt/boulder/core"
 	berrors "github.com/letsencrypt/boulder/errors"
@@ -960,14 +961,20 @@ func (va *ValidationAuthorityImpl) performRemoteValidation(ctx context.Context,
 					// If the non-nil err was a non-nil *probs.ProblemDetails then we can
 					// log it at an info level. It's a normal non-success validation
 					// result and the remote VA will have logged more detail.
-					va.log.Infof("Remote VA %q.PerformValidation failed: %s", rva.Addresses, err)
+					va.log.Infof("Remote VA %q.PerformValidation returned problem: %s", rva.Addresses, err)
 				} else if ok && p == nil {
 					// If the non-nil err was a nil *probs.ProblemDetails then we don't need to do
 					// anything. There isn't really an error here.
 					err = nil
+				} else if canceled.Is(err) {
+					// If the non-nil err was a canceled error, ignore it. That's fine it
+					// just means we cancelled the remote VA request before it was
+					// finished because we didn't care about its result.
+					err = nil
 				} else if !ok {
 					// Otherwise, the non-nil err was *not* a *probs.ProblemDetails and
-					// represents something that will later be returned as a server internal error
+					// was *not* a context cancelleded error and represents something that
+					// will later be returned as a server internal error
 					// without detail if the number of errors is >= va.maxRemoteFailures.
 					// Log it at the error level so we can debug from logs.
 					va.log.Errf("Remote VA %q.PerformValidation failed: %s", rva.Addresses, err)
diff --git a/va/va_test.go b/va/va_test.go
index cbf04cbb9..1e16c7444 100644
--- a/va/va_test.go
+++ b/va/va_test.go
@@ -1593,6 +1593,18 @@ func httpMultiSrv(t *testing.T, token string, allowedUAs map[string]struct{}) *m
 	return ms
 }
 
+// cancelledVA is a mock that always returns context.Canceled for
+// PerformValidation or IsSafeDomain calls
+type cancelledVA struct{}
+
+func (v cancelledVA) PerformValidation(_ context.Context, _ string, _ core.Challenge, _ core.Authorization) ([]core.ValidationRecord, error) {
+	return nil, context.Canceled
+}
+
+func (v cancelledVA) IsSafeDomain(_ context.Context, _ *vaPB.IsSafeDomainRequest) (*vaPB.IsDomainSafe, error) {
+	return nil, context.Canceled
+}
+
 func TestPerformRemoteValidation(t *testing.T) {
 	// Create a new challenge to use for the httpSrv
 	chall := core.HTTPChallenge01()
@@ -1639,6 +1651,35 @@ func TestPerformRemoteValidation(t *testing.T) {
 	ms.allowedUAs["remote 2"] = struct{}{}
 	ms.mu.Unlock()
 
+	localVA.remoteVAs = []RemoteVA{
+		{remoteVA1, "remote 1"},
+		{cancelledVA{}, "remote 2"},
+	}
+
+	// One remote cancelled, should return no err
+	localVA.performRemoteValidation(context.Background(), "localhost", chall, core.Authorization{}, probCh)
+	prob = <-probCh
+	if prob != nil {
+		t.Errorf("performRemoteValidation returned unexpected err from cancelled context: %s", prob)
+	}
+
+	localVA.remoteVAs = []RemoteVA{
+		{cancelledVA{}, "remote 1"},
+		{cancelledVA{}, "remote 2"},
+	}
+
+	// Both remotes cancelled, should return no err
+	localVA.performRemoteValidation(context.Background(), "localhost", chall, core.Authorization{}, probCh)
+	prob = <-probCh
+	if prob != nil {
+		t.Errorf("performRemoteValidation returned unexpected err from cancelled context: %s", prob)
+	}
+
+	localVA.remoteVAs = []RemoteVA{
+		{remoteVA1, "remote 1"},
+		{remoteVA2, "remote 2"},
+	}
+
 	// Both local and remotes working, should succeed
 	_, err := localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{})
 	if err != nil {