va: Handle IPv4 fallback for HTTP(S) redirects #8905
+263
−37
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -431,6 +431,24 @@ func fallbackErr(err error) bool { | |
| return false | ||
| } | ||
|
|
||
| // setHTTP01RequestHeaders sets the headers Boulder sends on every HTTP-01 | ||
| // request, including each fallback retry. | ||
| func (va *ValidationAuthorityImpl) setHTTP01RequestHeaders(req *http.Request) { | ||
| if va.userAgent != "" { | ||
| req.Header.Set("User-Agent", va.userAgent) | ||
| } | ||
| // Some of our users use mod_security. Mod_security sees a lack of Accept | ||
| // headers as bot behavior and rejects requests. While this is a bug in | ||
| // mod_security's rules (given that the HTTP specs disagree with that | ||
| // requirement), we add the Accept header now in order to fix our | ||
| // mod_security users' mysterious breakages. See | ||
| // <https://github.com/SpiderLabs/owasp-modsecurity-crs/issues/265> and | ||
| // <https://github.com/letsencrypt/boulder/issues/1019>. This was done | ||
| // because it's a one-line fix with no downside. We're not likely to want to | ||
| // do many more things to satisfy misunderstandings around HTTP. | ||
| req.Header.Set("Accept", "*/*") | ||
| } | ||
|
|
||
| // processHTTPValidation performs an HTTP validation for the given host, port | ||
| // and path. If successful the body of the HTTP response is returned along with | ||
| // the validation records created during the validation. If not successful | ||
|
|
@@ -486,19 +504,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( | |
| ctx, cancel := context.WithDeadline(ctx, deadline) | ||
| defer cancel() | ||
| initialReq = initialReq.WithContext(ctx) | ||
| va.setHTTP01RequestHeaders(initialReq) | ||
|
|
||
| // Set up the initial validation request and a base validation record | ||
| dialer, baseRecord, err := va.setupHTTPValidation(initialReq.URL.String(), target) | ||
|
|
@@ -515,6 +521,12 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( | |
| // addresses explicitly, not following redirects to ports != [80,443], etc) | ||
| records := []core.ValidationRecord{baseRecord} | ||
| numRedirects := 0 | ||
|
|
||
| // Track the target and URL selected for dialing so fallback retries the | ||
| // request that failed, including redirect targets. | ||
| // See https://github.com/letsencrypt/boulder/issues/8029. | ||
| currentTarget := target | ||
| currentReqURL := initialReq.URL.String() | ||
| processRedirect := func(req *http.Request, via []*http.Request) error { | ||
| va.log.Debugf("processing a HTTP redirect from the server to %q", req.URL.String()) | ||
| // Only process up to maxRedirect redirects | ||
|
|
@@ -595,6 +607,8 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( | |
| // Replace the transport's DialContext with the new preresolvedDialer for | ||
| // the redirect. | ||
| transport.DialContext = redirDialer.DialContext | ||
| currentTarget = redirTarget | ||
| currentReqURL = req.URL.String() | ||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -609,20 +623,25 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( | |
| // followed. | ||
| httpResponse, err := client.Do(initialReq) | ||
| // If there was an error and its a kind of error we consider a fallback error, | ||
| // then try to fallback. A fallback retry may itself dial a redirect target | ||
| // reached via processRedirect, so loop to allow a further fallback for | ||
| // that target rather than giving up after a single retry. Each target's | ||
| // own nextIP() bounds how many times it can be retried. | ||
| for err != nil && fallbackErr(err) { | ||
| // Try to advance to another IP for the target we were dialing when | ||
| // the failure occurred: the initial target, or the most recent | ||
| // redirect target. If there was an error advancing we don't have a | ||
| // fallback address to use and must return the original error. | ||
| advanceTargetIPErr := currentTarget.nextIP() | ||
| if advanceTargetIPErr != nil { | ||
| return nil, records, newIPError(records[len(records)-1].AddressUsed, err) | ||
| } | ||
|
|
||
| // setup another validation to retry the current target with the new | ||
| // IP and append the retry record. | ||
| retryDialer, retryRecord, setupErr := va.setupHTTPValidation(currentReqURL, currentTarget) | ||
| if setupErr != nil { | ||
| return nil, records, newIPError(records[len(records)-1].AddressUsed, setupErr) | ||
| } | ||
|
|
||
| records = append(records, retryRecord) | ||
|
|
@@ -631,15 +650,23 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( | |
| // host. | ||
| transport.DialContext = retryDialer.DialContext | ||
|
|
||
| // Resume from the request whose dial failed. Replaying initialReq would | ||
| // repeat earlier redirects and trip loop detection. | ||
| retryReq, retryReqErr := http.NewRequestWithContext(ctx, "GET", currentReqURL, nil) | ||
|
Contributor
There was a problem hiding this comment. Using NewRequestWithContext here is a good improvement. Let's take it even further:
|
||
| if retryReqErr != nil { | ||
| return nil, records, newIPError(records[len(records)-1].AddressUsed, retryReqErr) | ||
| } | ||
| va.setHTTP01RequestHeaders(retryReq) | ||
|
|
||
| // Perform the retry. This may itself follow further redirects via | ||
| // processRedirect, which updates currentTarget and currentReqURL; if | ||
| // the resulting dial fails, the loop condition re-evaluates | ||
| // fallbackErr(err) against that new target. | ||
| httpResponse, err = client.Do(retryReq) | ||
|
Contributor
There was a problem hiding this comment. 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. |
||
| } | ||
| if err != nil { | ||
| // if the error was not (or was no longer) a fallbackErr, return | ||
| // immediately. | ||
| return nil, records, newIPError(records[len(records)-1].AddressUsed, err) | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import ( | |
| "unicode/utf8" | ||
|
|
||
| "github.com/miekg/dns" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
|
|
||
| "github.com/letsencrypt/boulder/bdns" | ||
| "github.com/letsencrypt/boulder/core" | ||
|
|
@@ -54,13 +55,12 @@ func (c *ipFakeDNS) LookupA(_ context.Context, hostname string) (*bdns.Result[*d | |
| if c.ip != nil && c.ip.To4() != nil { | ||
| ip = c.ip | ||
| } | ||
| // ipv6.localhost has no IPv4 address. | ||
| if hostname == "ipv6.localhost" { | ||
| return wrapA() | ||
| } | ||
| // All other valid test hosts, including the dual-homed hosts, have an IPv4 | ||
| // address. | ||
| return wrapA(ip) | ||
| } | ||
|
|
||
|
|
@@ -78,8 +78,10 @@ func (c *ipFakeDNS) LookupAAAA(_ context.Context, hostname string) (*bdns.Result | |
| ip = c.ip | ||
| } | ||
|
|
||
| // The dual-homed test hosts have both an IPv6 and an IPv4 address: | ||
| // LookupA supplies the IPv4 address while this branch supplies the IPv6 | ||
| // address. | ||
| if hostname == "ipv4.and.ipv6.localhost" || hostname == "ipv4.and.ipv6.example.com" { | ||
| return wrapAAAA(ip) | ||
| } | ||
| if hostname == "ipv6.localhost" { | ||
|
|
@@ -679,6 +681,19 @@ func httpTestSrv(t *testing.T, ipv6 bool) *httptest.Server { | |
| ) | ||
| }) | ||
|
|
||
| // A path that redirects to a dual-homed host (both IPv6 and IPv4 | ||
| // addresses) whose name ends in a valid IANA-registered TLD. On the | ||
| // IPv4-only test server, the redirect target's preferred IPv6 address has | ||
| // nothing listening, forcing a fallback to its IPv4 address. | ||
| mux.HandleFunc("/redir-dual-host", func(resp http.ResponseWriter, req *http.Request) { | ||
| http.Redirect( | ||
| resp, | ||
| req, | ||
| fmt.Sprintf("http://ipv4.and.ipv6.example.com:%d/ok", httpPort), | ||
| http.StatusMovedPermanently, | ||
| ) | ||
| }) | ||
|
|
||
| mux.HandleFunc("/bad-status-code", func(resp http.ResponseWriter, req *http.Request) { | ||
| resp.WriteHeader(http.StatusGone) | ||
| fmt.Fprint(resp, "sorry, I'm gone") | ||
|
|
@@ -1167,6 +1182,40 @@ func TestFetchHTTP(t *testing.T) { | |
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "Redirect to dual homed host with broken IPv6, working IPv4", | ||
| Ident: identifier.NewDNS("example.com"), | ||
| Path: "/redir-dual-host", | ||
| ExpectedBody: "ok", | ||
| ExpectedRecords: []core.ValidationRecord{ | ||
| { | ||
| Hostname: "example.com", | ||
| Port: strconv.Itoa(httpPortIPv4), | ||
| URL: "http://example.com/redir-dual-host", | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("127.0.0.1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(httpPortIPv4), | ||
| URL: fmt.Sprintf("http://ipv4.and.ipv6.example.com:%d/ok", httpPortIPv4), | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| // First attempt at the redirect target used its IPv6 address. | ||
| AddressUsed: netip.MustParseAddr("::1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(httpPortIPv4), | ||
| URL: fmt.Sprintf("http://ipv4.and.ipv6.example.com:%d/ok", httpPortIPv4), | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| // The retry fell back to the redirect target's IPv4 address. | ||
| AddressUsed: netip.MustParseAddr("127.0.0.1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "Working IPv4 only", | ||
| Ident: identifier.NewDNS("example.com"), | ||
|
|
@@ -1258,6 +1307,156 @@ func TestFetchHTTP(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestFetchHTTPRedirectFallbackExhaustion verifies that when a redirect target | ||
| // exhausts all its addresses, the error is attributed to the last one tried. | ||
| func TestFetchHTTPRedirectFallbackExhaustion(t *testing.T) { | ||
| // Pick a closed local port (per TestHTTPBadPort) for the redirect target so | ||
| // both of its addresses fail fast with connection refused instead of timing | ||
| // out on an unreachable IP. | ||
| badPort := 40000 + mrand.IntN(25000) | ||
|
|
||
| redirURL := fmt.Sprintf("https://ipv4.and.ipv6.example.com:%d/ok", badPort) | ||
| mux := http.NewServeMux() | ||
| mux.HandleFunc("/redir-to-https", func(resp http.ResponseWriter, req *http.Request) { | ||
| http.Redirect(resp, req, redirURL, http.StatusMovedPermanently) | ||
| }) | ||
| testSrv := httptest.NewServer(mux) | ||
| defer testSrv.Close() | ||
| httpPort := getPort(testSrv) | ||
| if badPort == httpPort { | ||
| badPort = httpPort + 1 | ||
| redirURL = fmt.Sprintf("https://ipv4.and.ipv6.example.com:%d/ok", badPort) | ||
| } | ||
|
|
||
| va, _ := setup(testSrv, "", nil, &ipFakeDNS{}) | ||
| // Accept the closed redirect port so redirect policy permits the hop. | ||
| va.httpsPort = badPort | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) | ||
|
Contributor
There was a problem hiding this comment. Always use t.Context() instead of context.Background(). (Same comment in TestFetchHTTPRedirectDoubleFallback.) |
||
| defer cancel() | ||
|
|
||
| _, records, err := va.processHTTPValidation(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), "/redir-to-https") | ||
| test.AssertError(t, err, "redirect target with no reachable address should fail") | ||
|
Contributor
There was a problem hiding this comment. 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 |
||
|
|
||
| // The problem is attributed to the target's last address tried: 127.0.0.1. | ||
| prob := detailedError(err) | ||
| test.AssertMarshaledEquals(t, prob, probs.Connection(fmt.Sprintf( | ||
| "127.0.0.1: Fetching %s: Connection refused", redirURL))) | ||
|
|
||
| // Three records: the reachable initial raw-IP request that received the | ||
| // redirect, the target's IPv6 attempt, and the target's IPv4 fallback. | ||
| expectedRecords := []core.ValidationRecord{ | ||
| { | ||
| Hostname: "127.0.0.1", | ||
| Port: strconv.Itoa(httpPort), | ||
| URL: "http://127.0.0.1/redir-to-https", | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("127.0.0.1"), | ||
| }, | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(badPort), | ||
| URL: redirURL, | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("::1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(badPort), | ||
| URL: redirURL, | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("127.0.0.1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| } | ||
| test.AssertMarshaledEquals(t, records, expectedRecords) | ||
|
|
||
| // One redirect hop was followed, and the target fell back IPv6-to-IPv4 once | ||
| // before exhausting its addresses. | ||
| test.AssertMetricWithLabelsEquals(t, va.metrics.http01Redirects, prometheus.Labels{}, 1) | ||
| test.AssertMetricWithLabelsEquals(t, va.metrics.http01Fallbacks, prometheus.Labels{}, 1) | ||
| } | ||
|
|
||
| // TestFetchHTTPRedirectDoubleFallback covers #8029: the initial HTTP host and | ||
| // its HTTPS redirect target each require an independent IPv6-to-IPv4 fallback. | ||
| func TestFetchHTTPRedirectDoubleFallback(t *testing.T) { | ||
| // Both servers listen on IPv4 only while ipFakeDNS prefers IPv6. | ||
| httpsMux := http.NewServeMux() | ||
| httpsMux.HandleFunc("/ok", func(resp http.ResponseWriter, req *http.Request) { | ||
| resp.WriteHeader(http.StatusOK) | ||
| fmt.Fprint(resp, "ok") | ||
| }) | ||
| httpsSrv := httptest.NewTLSServer(httpsMux) | ||
| defer httpsSrv.Close() | ||
| httpsPort := getPort(httpsSrv) | ||
|
|
||
| httpMux := http.NewServeMux() | ||
| httpMux.HandleFunc("/redir-to-https", func(resp http.ResponseWriter, req *http.Request) { | ||
| http.Redirect( | ||
| resp, | ||
| req, | ||
| fmt.Sprintf("https://ipv4.and.ipv6.example.com:%d/ok", httpsPort), | ||
| http.StatusMovedPermanently, | ||
| ) | ||
| }) | ||
| httpSrv := httptest.NewServer(httpMux) | ||
| defer httpSrv.Close() | ||
| httpPort := getPort(httpSrv) | ||
|
|
||
| va, _ := setup(httpSrv, "", nil, &ipFakeDNS{}) | ||
| // Allow the explicit HTTPS test port as a redirect target. | ||
| va.httpsPort = httpsPort | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) | ||
| defer cancel() | ||
|
|
||
| body, records, err := va.processHTTPValidation(ctx, identifier.NewDNS("ipv4.and.ipv6.example.com"), "/redir-to-https") | ||
| test.AssertNotError(t, err, "HTTP-to-HTTPS double fallback should have succeeded") | ||
| test.AssertEquals(t, string(body), "ok") | ||
|
|
||
| initialURL := "http://ipv4.and.ipv6.example.com/redir-to-https" | ||
| httpsURL := fmt.Sprintf("https://ipv4.and.ipv6.example.com:%d/ok", httpsPort) | ||
| expectedRecords := []core.ValidationRecord{ | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(httpPort), | ||
| URL: initialURL, | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("::1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(httpPort), | ||
| URL: initialURL, | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("127.0.0.1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(httpsPort), | ||
| URL: httpsURL, | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("::1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| { | ||
| Hostname: "ipv4.and.ipv6.example.com", | ||
| Port: strconv.Itoa(httpsPort), | ||
| URL: httpsURL, | ||
| AddressesResolved: []netip.Addr{netip.MustParseAddr("::1"), netip.MustParseAddr("127.0.0.1")}, | ||
| AddressUsed: netip.MustParseAddr("127.0.0.1"), | ||
| ResolverAddrs: []string{"ipFakeDNS", "ipFakeDNS"}, | ||
| }, | ||
| } | ||
| test.AssertMarshaledEquals(t, records, expectedRecords) | ||
|
|
||
| test.AssertMetricWithLabelsEquals(t, va.metrics.http01Redirects, prometheus.Labels{}, 1) | ||
| test.AssertMetricWithLabelsEquals(t, va.metrics.http01Fallbacks, prometheus.Labels{}, 2) | ||
| } | ||
|
|
||
| // All paths that get assigned to tokens MUST be valid tokens | ||
| const pathWrongToken = "i6lNAC4lOOLYCl-A08VJt9z_tKYvVk63Dumo8icsBjQ" | ||
| const path404 = "404" | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we could fix this by having
fallbackErrreturnfalsewhenerrors.Is(err, context.DeadlineExceeded)(orcontext.Canceled).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That unfortunately doesn't work, because context.DeadlineExceeded could be returned by an inner context (e.g. a child context created by net.Dial itself, with a shorter timeout). I think the correct check is
if fallbackErr(err) && ctx.Err() == nil, to ensure we're only bailing out if our context has ended.