va: Handle IPv4 fallback for HTTP(S) redirects

[sheurich]

Fixes #8029

When an HTTP-01 validation follows a redirect to a dual-stack host and the IPv6 dial fails, the VA retried the original validation target instead of the redirect target, so IPv6-to-IPv4 fallback never applied past the first hop. A host reachable only over IPv4 behind a redirect failed validation despite publishing both address types.

The fallback path now tracks the target and URL most recently selected by redirect handling. On a qualifying dial failure it advances that target's IP queue and retries with a fresh bodyless GET against the failed hop, resuming from that request rather than replaying earlier redirects (which would re-run prior hops and trip loop detection). The single retry becomes a loop so each hop can fall back independently; it terminates when a target exhausts its addresses (nextIP) or redirects hit maxRedirects. http01Fallbacks now counts one increment per fallback hop.

setHTTP01RequestHeaders centralizes the Boulder-controlled User-Agent and Accept headers so the initial request and every fallback retry share one setup.

Tests:

• Exact va: Handle IPv4 fallback for HTTP(S) redirects #8029 reproduction: an HTTP host redirecting to HTTPS on the same dual-stack host, each hop requiring IPv6-to-IPv4 fallback; asserts all four validation records and the redirect (1) and fallback (2) counters.
• Redirect-target exhaustion: the redirect target fails on both families (a closed local port), asserting the final problem, all records, and counters.
• A redirect-to-dual-homed-host case added to TestFetchHTTP covering fallback at the redirect target.

[aarongable]

This change looks really good; thank you for taking the time to pick apart this hairy mess of loops and redirects and fallbacks. I have several comments, but none are critical -- the test comments are stylistic, and most of the production code comments are about improving this further to address other long-standing (but minor) issues, not about the correctness of this PR itself.

[aarongable]

fallbackErr only checks that the underlying error occurred during dial, not later in the request. If the context deadline expires during the dial, then that'll be treated as a fallback error, we'll advance to the next IP, attempt to dial, and immediately fail. This will make it look like it's actually the next IP that timed out, not this one. Not sure of the best way to fix this (and it's basically a pre-existing bug), but worth considering.

Separately, I'd prefer not to have this be a loop. While yes, newHTTPValidationTarget itself will prevent us from falling back multiple times (by virtue of only populating two IPs in the first place), we know that we only want to fallback once. Having this be a loop gives the false impression that we might fall back many times. Maybe we should simplify httpValidationTarget to just have a defaultIP and a fallbackIP, and get rid of all of the tried/cur/next machinery.

[jsha]

If the context deadline expires during the dial, then that'll be treated as a fallback error, we'll advance to the next IP, attempt to dial, and immediately fail. This will make it look like it's actually the next IP that timed out, not this one. Not sure of the best way to fix this (and it's basically a pre-existing bug), but worth considering.

Seems like we could fix this by having fallbackErr return false when errors.Is(err, context.DeadlineExceeded) (or context.Canceled).

[aarongable]

Using NewRequestWithContext here is a good improvement. Let's take it even further:

• Upgrade your new setHTTP01RequestHeaders helper into newHTTP01Request
  • This new helper will take a context and a URL as inputs
  • It will call NewRequestWithContext and set the request headers, then return the request object
• Use this new helper both here and at lines 485-507 (i.e. move the context creation and dealine setting up a bit, and then pass that context into the new helper to get initialReq).

[aarongable]

Always use t.Context() instead of context.Background(). (Same comment in TestFetchHTTPRedirectDoubleFallback.)

[aarongable]

Because these are brand new test functions, please follow the go style guide convention of not using test assertion helpers. We're moving away from our test.AssertFoo helpers, only continuing to use them when editing existing test functions, not adding new ones.

[aarongable]

When go does an HTTP 3XX redirect, the follow-up request automatically has the Referer: header set. This fallback retry won't, since we're building it manually. It would be nice to set the Referer header if this isn't the first hop. But this is definitely a pre-existing issue, so don't worry about it too much.

[jsha]

Thanks for working on this! The change looks correct to me, but it makes our dependence on the CheckRedirect callback even trickier to follow. In particular, having the outer call pick up where a CheckRedirect previously left off seems to me like it has a high potential for bugs.

I believe @aarongable is working on an additional refactor that will remove reliance on CheckRedirect and have Boulder implement its own redirect following. I think that will result in clearer code, where the "first try" and the "fallback try" are adjacent to each other. I think it makes sense to only land this fix alongside that refactor.

BTW, I was curious how we wound up with the current setup: given that we only want to fallback on dial errors, it's strange to implement that fallback at the HTTP level. Wouldn't it be easier to implement a custom dialer that falls back, and let the HTTP layer be oblivious?

Turns out we used to do that, and abandoned it after encountering bugs: #3939, #3889. Here's what the old custom dialer looked like:

boulder/va/va.go

Lines 240 to 340 in d9d2f4e

	// http01Dialer is a struct that exists to provide a dialer like object with
	// a `DialContext` method that can be given to an http.Transport for HTTP-01
	// validation. The primary purpose of the http01Dialer's DialContext method
	// is to circumvent traditional DNS lookup and to use the IP addresses in the
	// addr slice.
	type http01Dialer struct {
	addrs []net.IP
	hostname string
	port string
	stats metrics.Scope
	dialerCount int
	addrInfoChan chan addrRecord
	}
	// realDialer is used to create a true `net.Dialer` that can be used once an IP
	// address to connect to is determined. It increments the `dialerCount` integer
	// to track how many "fresh" dialer instances have been created during a
	// `DialContext` for testing purposes.
	func (d *http01Dialer) realDialer() *net.Dialer {
	// Record that we created a new instance of a real net.Dialer
	d.dialerCount++
	return &net.Dialer{Timeout: singleDialTimeout}
	}
	// DialContext processes the IP addresses from the inner validation record, using
	// `realDialer` to make connections as required. For dual-homed hosts an initial
	// IPv6 connection will be made followed by a IPv4 connection if there is a failure
	// with the IPv6 connection.
	func (d *http01Dialer) DialContext(ctx context.Context, _, _ string) (net.Conn, error) {
	deadline, ok := ctx.Deadline()
	if !ok {
	// Shouldn't happen: All requests should have a deadline by this point.
	deadline = time.Now().Add(100 * time.Second)
	} else {
	// Set the context deadline slightly shorter than the HTTP deadline, so we
	// get the dial error rather than a generic "deadline exceeded" error. This
	// lets us give a more specific error to the subscriber.
	deadline = deadline.Add(-10 * time.Millisecond)
	}
	ctx, cancel := context.WithDeadline(ctx, deadline)
	defer cancel()
	var realDialer *net.Dialer
	var addrInfo addrRecord
	// Split the available addresses into v4 and v6 addresses
	v4, v6 := availableAddresses(d.addrs)
	// If there is at least one IPv6 address then try it first
	if len(v6) > 0 {
	address := net.JoinHostPort(v6[0].String(), d.port)
	addrInfo.used = v6[0]
	realDialer = d.realDialer()
	conn, err := realDialer.DialContext(ctx, "tcp", address)
	// If there is no error, return immediately
	if err == nil {
	d.addrInfoChan <- addrInfo
	return conn, err
	}
	// Otherwise, we note that we tried an address and fall back to trying IPv4
	addrInfo.tried = append(addrInfo.tried, addrInfo.used)
	d.stats.Inc("IPv4Fallback", 1)
	}
	// If there are no IPv4 addresses and we tried an IPv6 address return an
	// error - there's nothing left to try
	if len(v4) == 0 && len(addrInfo.tried) > 0 {
	d.addrInfoChan <- addrInfo
	return nil,
	fmt.Errorf("Unable to contact %q at %q, no IPv4 addresses to try as fallback",
	d.hostname, addrInfo.tried[0])
	} else if len(v4) == 0 && len(addrInfo.tried) == 0 {
	// It shouldn't be possible that there are no IPv4 addresses and no previous
	// attempts at an IPv6 address connection but be defensive about it anyway
	d.addrInfoChan <- addrInfo
	return nil, fmt.Errorf("no IP addresses found for %q", d.hostname)
	}
	// Otherwise if there are no IPv6 addresses, or there was an error
	// talking to the first IPv6 address, try the first IPv4 address
	addrInfo.used = v4[0]
	d.addrInfoChan <- addrInfo
	realDialer = d.realDialer()
	return realDialer.DialContext(ctx, "tcp", net.JoinHostPort(v4[0].String(), d.port))
	}
	// availableAddresses takes a ValidationRecord and splits the AddressesResolved
	// into a list of IPv4 and IPv6 addresses.
	func availableAddresses(allAddrs []net.IP) (v4 []net.IP, v6 []net.IP) {
	for _, addr := range allAddrs {
	if addr.To4() != nil {
	v4 = append(v4, addr)
	} else {
	v6 = append(v6, addr)
	}
	}
	return
	}

The current version does the dial, builds an HTTP request to a URL containing an IP address, and overrides the Host header to the host we actually want.

[jsha]

If the context deadline expires during the dial, then that'll be treated as a fallback error, we'll advance to the next IP, attempt to dial, and immediately fail. This will make it look like it's actually the next IP that timed out, not this one. Not sure of the best way to fix this (and it's basically a pre-existing bug), but worth considering.

Seems like we could fix this by having fallbackErr return false when errors.Is(err, context.DeadlineExceeded) (or context.Canceled).