HTTP-01: Do our own redirect handling

[aarongable]

Note

This PR builds on #8905, only the most recent commit is new. This PR also addresses all of my review comments left on that PR; I'd be happy landing that PR as-is as long as this PR follows it.

This PR makes two large simplifications to the VA's HTTP-01 validation code, and a number of smaller modifications as knock-on effects.

The first large simplification is the replacement of Go's default redirect handling with a straightforward loop that we manage directly. Although hooking into Go's http.Client.CheckRedirect function was useful, it required our code to be non-linear because that function is called as a callback. We'd set up a redirect request for Go to make, it would magically happen in the background, and then our straight-line code would deal with the result, including potentially falling back from an IPv6 attempt to an IPv4 attempt outside Go's redirect handling. This has resulted in numerous bugs over time.

The new code instead tells Go to never follow redirects on its own, and just return the redirect response to us. We then handle every validation response with a few simple cases:

• if it was an error we're willing to retry, fall back to IPv4;
• if it was some other form of error, error out;
• if it was a redirect, enforce our redirect policy and then follow it; or
• if it was some other form of success, return the result to the validation checker.

This does introduce some additional complexity (e.g. having to track the referer ourselves, and not attach it when redirecting from https to http), but I think the tradeoff is worth it.

The second large simplification is in how we keep track of what requests we're making. Historically we've had both an httpValidationTarget and a core.ValidationRecord which stored redundant information and were used almost interchangeably. This change gets rid of httpValidationTarget. Prior to each iteration of the loop described above, we construct a ValidationRecord. Each loop appends that record to the list prior to making the validation request, so that any errors encountered in the process will be associated with the most recent entry in the list of records. And the functions which actually make the request extract their parameters (the IP address, the port, the path, etc) directly from the ValidationRecord, ensuring that we're recording exactly what we do, with no drift.

Smaller knock-on changes include:

• reducing the total number of requests we're willing to make from 12 (as it currently stands: 1 initial request, 10 redirects, and 1 fallback) or 22 (as Shiloh's PR makes it: 1 initial request, 10 redirects, and one fallback per other request), to 10 (as most readers probably thought we were doing in the past);
• combining extractRequestTarget and processRedirect (the old closure) into a single newValidationRecordFromRedirect function;
• changing the preresolvedDialer to store the port as an int, so we don't have to convert it back and forth between the validation record and the dialer;
• upgrading our body reading to use the new core.ErrOnLimitReader; and
• of course many updates to the tests.

An LLM was used to review and improve an earlier version of this PR, and to do most of the unit test case updates.

[jsha]

(partial review)

[jsha]

Suggested change

	// with its AddressUsed replaced by the host's first IPv4 address if an
	// with its AddressUsed replaced by the first IPv4 address from AddressesResolved if an

[jsha]

FYI I think we can delete this code as unreachable, probably as a followup.

TLS v1.1 and below was disabled by default in Go 1.18: https://go.dev/doc/go1.18#tls10

We added this code to give an informative message in advance of the disabling: https://github.com/letsencrypt/boulder/pull/6043/changes#diff-32a1785ca91daea6a2eba2d5df5b1f33c7fc950ecedecd068bd46015f7f34dcd. After updating to Go 1.18, we temporarily re-enabled TLS 1.0 / 1.1 via GODEBUG so we could show this message. But we don't set that anymore and we don't set MinVersion in our tls.Config, so I think we'll never reach this anymore. Which is fine - it's been long enough that we don't need the informative message anymore.

[jsha]

Really this function creates a validation record based on a URL. The IdentifierType is secondary and I think we should get rid of it. It creates a bit of confusion because IdentifierType refers to ACME identifiers, but a redirect from http://example.com/.well-known/acme-challenge/foo to http://192.0.2.0/foo doesn't create a new ACME identifier; it just creates a new URL.

Below, we switch on typ to decide whether to do a DNS lookup, but I think we could just as well switch on whether the URL's Host() is a valid IP address.

We do want to maintain the property that a validation for an IP address can never do a DNS lookup on the first fetch. If we remove the typ parameter here, we would achieve that by relying on the proper construction and parsing of the URL, and proper validation of the ident.Value of "dns" identifiers. However, if we wanted even more belt-and-suspenders, this function could take an allowDNSResolution bool that is false on initial request for DNS identifiers, but otherwise always true. IMO that would be clearer than passing through the identifier.IdentifierType.

[jsha]

Looking below at 338-339, I start to see more of why this function takes a typ: in newValidationRecordFromRedirect we do some parsing on the host to see if it's an IP or a hostname, then do appropriate checks: no zone on an IPv6 IP address; not a private IP address; ends in a public suffix as a hostname. Those checks are only really needed on redirects since we already validate incoming identifiers higher in the stack. But I think we should move them up to here in newValidationRecord even if they're slightly duplicative on the first request. That makes it really clear that they are applied in all cases (e.g. does newValidationRecordFromFallback need to check for private IP addresses? probably not because we can only fall back if the address came from DNS, and the bdns package also checks... but that's a complicated chain of logic).

https://github.com/letsencrypt/boulder/pull/8961/changes#diff-32a1785ca91daea6a2eba2d5df5b1f33c7fc950ecedecd068bd46015f7f34dcdR338-R339

[jsha]

I think we shouldn't apply this fixup on all redirects. It was added in #8020 because when we construct the initial URL, we have a bare IP address and it's our responsibility to wrap it in brackets. If someone else redirects us to a malformed URL that contains an IPv6 address without brackets, we should not try to correct it for them.

So this tweak should move up to the first invocation of newValidationRecord and the justification should talk about spec compliance instead of url.Hostname() output.

[jsha]

Alright, done reviewing. Pretty excited about this change!

[jsha]

I was confused about why we had this status code check in two places, and then realized they're non-identical: 303 will fall through to "received disallowed redirect status code" in newValidationRecordFromRedirect, which preserves existing behavior.

What do you think of moving the BR-compliant redirect status check here, with a special error case for 303 to preserve existing behavior?

[jsha]

Looking below at 338-339, I start to see more of why this function takes a typ: in newValidationRecordFromRedirect we do some parsing on the host to see if it's an IP or a hostname, then do appropriate checks: no zone on an IPv6 IP address; not a private IP address; ends in a public suffix as a hostname. Those checks are only really needed on redirects since we already validate incoming identifiers higher in the stack. But I think we should move them up to here in newValidationRecord even if they're slightly duplicative on the first request. That makes it really clear that they are applied in all cases (e.g. does newValidationRecordFromFallback need to check for private IP addresses? probably not because we can only fall back if the address came from DNS, and the bdns package also checks... but that's a complicated chain of logic).

https://github.com/letsencrypt/boulder/pull/8961/changes#diff-32a1785ca91daea6a2eba2d5df5b1f33c7fc950ecedecd068bd46015f7f34dcdR338-R339