VA: put most recent, not original, IP in error messages (#7468)

When we present error messages containing IP addresses to end users,
sometimes we're presenting the IP at which we began a chain of
redirects, but not presenting the IP at which the final redirect was
located. This can make it difficult for client operators to identify
exactly which server in their infrastructure is misbehaving.

Change our IP error messages to reference the most-recently-tried
address from the set of all validation records, rather than the address
which started the current (potential) redirect chain.

Fixes #7347

Author: aarongable

va/http.go

@@ -197,12 +197,6 @@ func (vt *httpValidationTarget) nextIP() error {
 	return nil
 }
 
-// ip returns the current *net.IP for the validation target. It may return nil
-// if all possible IPs have been expended by calls to nextIP.
-func (vt *httpValidationTarget) ip() net.IP {
-	return vt.cur
-}
-
 // newHTTPValidationTarget creates a httpValidationTarget for the given host,
 // port, and path. This involves querying DNS for the IP addresses for the host.
 // An error is returned if there are no usable IP addresses or if the DNS
@@ -369,7 +363,7 @@ func (va *ValidationAuthorityImpl) setupHTTPValidation(
 	}
 
 	// Get the target IP to build a preresolved dialer with
-	targetIP := target.ip()
+	targetIP := target.cur
 	if targetIP == nil {
 		return nil,
 			record,
@@ -425,20 +419,12 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
 	ctx context.Context,
 	host string,
 	path string) ([]byte, []core.ValidationRecord, error) {
-
 	// Create a target for the host, port and path with no query parameters
 	target, err := va.newHTTPValidationTarget(ctx, host, va.httpPort, path, "")
 	if err != nil {
 		return nil, nil, err
 	}
 
-	// newIPError implements the error interface. It wraps an error and the IP
-	// of the remote host in an IPError so we can display the IP in the problem
-	// details returned to the client.
-	newIPError := func(target *httpValidationTarget, err error) error {
-		return ipError{ip: target.cur, err: err}
-	}
-
 	// Create an initial GET Request
 	initialURL := url.URL{
 		Scheme: "http",
@@ -447,7 +433,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
 	}
 	initialReq, err := http.NewRequest("GET", initialURL.String(), nil)
 	if err != nil {
-		return nil, nil, newIPError(target, err)
+		return nil, nil, newIPError(target.cur, err)
 	}
 
 	// Add a context to the request. Shave some time from the
@@ -484,7 +470,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
 	// Set up the initial validation request and a base validation record
 	dialer, baseRecord, err := va.setupHTTPValidation(initialReq.URL.String(), target)
 	if err != nil {
-		return nil, []core.ValidationRecord{}, newIPError(target, err)
+		return nil, []core.ValidationRecord{}, newIPError(target.cur, err)
 	}
 
 	// Build a transport for this validation that will use the preresolvedDialer's
@@ -606,16 +592,17 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
 		// have a fallback address to use and must return the original error.
 		advanceTargetIPErr := target.nextIP()
 		if advanceTargetIPErr != nil {
-			return nil, records, newIPError(target, err)
+			return nil, records, newIPError(records[len(records)-1].AddressUsed, err)
 		}
 
 		// setup another validation to retry the target with the new IP and append
 		// the retry record.
 		retryDialer, retryRecord, err := va.setupHTTPValidation(initialReq.URL.String(), target)
-		records = append(records, retryRecord)
 		if err != nil {
-			return nil, records, newIPError(target, err)
+			return nil, records, newIPError(records[len(records)-1].AddressUsed, err)
 		}
+
+		records = append(records, retryRecord)
 		va.metrics.http01Fallbacks.Inc()
 		// Replace the transport's dialer with the preresolvedDialer for the retry
 		// host.
@@ -626,15 +613,15 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
 		// If the retry still failed there isn't anything more to do, return the
 		// error immediately.
 		if err != nil {
-			return nil, records, newIPError(target, err)
+			return nil, records, newIPError(retryRecord.AddressUsed, err)
 		}
 	} else if err != nil {
 		// if the error was not a fallbackErr then return immediately.
-		return nil, records, newIPError(target, err)
+		return nil, records, newIPError(records[len(records)-1].AddressUsed, err)
 	}
 
 	if httpResponse.StatusCode != 200 {
-		return nil, records, newIPError(target, berrors.UnauthorizedError("Invalid response from %s: %d",
+		return nil, records, newIPError(records[len(records)-1].AddressUsed, berrors.UnauthorizedError("Invalid response from %s: %d",
 			records[len(records)-1].URL, httpResponse.StatusCode))
 	}
 
@@ -646,13 +633,13 @@ func (va *ValidationAuthorityImpl) processHTTPValidation(
 		err = closeErr
 	}
 	if err != nil {
-		return nil, records, newIPError(target, berrors.UnauthorizedError("Error reading HTTP response body: %v", err))
+		return nil, records, newIPError(records[len(records)-1].AddressUsed, berrors.UnauthorizedError("Error reading HTTP response body: %v", err))
 	}
 
 	// io.LimitedReader will silently truncate a Reader so if the
 	// resulting payload is the same size as maxResponseSize fail
 	if len(body) >= maxResponseSize {
-		return nil, records, newIPError(target, berrors.UnauthorizedError("Invalid response from %s: %q",
+		return nil, records, newIPError(records[len(records)-1].AddressUsed, berrors.UnauthorizedError("Invalid response from %s: %q",
 			records[len(records)-1].URL, body))
 	}
 

va/http_test.go

@@ -94,7 +94,7 @@ func TestPreresolvedDialerTimeout(t *testing.T) {
 	// Check that the HTTP connection doesn't return too fast, and times
 	// out after the expected time
 	if took < va.singleDialTimeout {
-		t.Fatalf("fetch returned before %s (took: %s) with %#v", va.singleDialTimeout, took, err)
+		t.Fatalf("fetch returned before %s (took: %s) with %q", va.singleDialTimeout, took, err.Error())
 	}
 	if took > 2*va.singleDialTimeout {
 		t.Fatalf("fetch didn't timeout after %s (took: %s)", va.singleDialTimeout, took)
@@ -184,7 +184,7 @@ func TestHTTPValidationTarget(t *testing.T) {
 				// Calling ip() on the target should give the expected IPs in the right
 				// order.
 				for i, expectedIP := range tc.ExpectedIPs {
-					gotIP := target.ip()
+					gotIP := target.cur
 					if gotIP == nil {
 						t.Errorf("Expected IP %d to be %s got nil", i, expectedIP)
 					} else {
@@ -1404,7 +1404,7 @@ func TestHTTPDialTimeout(t *testing.T) {
 	// Check that the HTTP connection doesn't return too fast, and times
 	// out after the expected time
 	if took < (timeout-200*time.Millisecond)/2 {
-		t.Fatalf("HTTP returned before %s (%s) with %#v", timeout, took, err)
+		t.Fatalf("HTTP returned before %s (%s) with %q", timeout, took, err.Error())
 	}
 	if took > 2*timeout {
 		t.Fatalf("HTTP connection didn't timeout after %s seconds", timeout)

va/va.go

@@ -330,6 +330,12 @@ type ipError struct {
 	err error
 }
 
+// newIPError wraps an error and the IP of the remote host in an ipError so we
+// can display the IP in the problem details returned to the client.
+func newIPError(ip net.IP, err error) error {
+	return ipError{ip: ip, err: err}
+}
+
 // Unwrap returns the underlying error.
 func (i ipError) Unwrap() error {
 	return i.err