adds an explicit timeout when pre-checking HTTP-01 challenges

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>

pkg/issuer/acme/http/http.go

@@ -191,23 +191,57 @@ func testReachability(ctx context.Context, url *url.URL, key string) error {
 	}
 	req.Header.Set("User-Agent", pkgutil.CertManagerUserAgent)
 
-	// ACME spec says that a verifier should try
-	// on http port 80 first, but follow any redirects may be thrown its way
-	// The redirects may be HTTPS and its certificate may be invalid (they are trying to get a
-	// certificate after all).
-	// TODO(dmo): figure out if we need to add a more specific timeout for
-	// individual checks
+	// The ACME spec says that a verifier should try on http port 80 first, but to follow any
+	// redirects which may be returned. Let's Encrypt, in practice, follows redirects for HTTP
+	// and HTTPS services on ports 80 and 443 respectively, but the spec doesn't seem to require
+	// anything other than the initial connection being on port 80.
+
+	// For further reading, the spec also discusses redirect following in section 10.2:
+	// https://datatracker.ietf.org/doc/html/rfc8555#section-10.2
+
+	// TODO: Since Let's Encrypt will only accept redirects to port 80 and port 443, and we follow
+	// any redirect here, this could lead to a failure mode where we determine that the endpoint is reachable
+	// but it'll certainly fail when tried by the actual verifier; it's an edge case, but we might be able
+	// to handle this better.
+
+	// The timeouts here are inspired by the timeouts used by Boulder - i.e., Let's Encrypt - when
+	// validating HTTP01 challenges for real.
+	// Boulder http.Transport: https://github.com/letsencrypt/boulder/blob/30a516737c9daa4c88c8c47070c25a5e7033cdcf/va/http.go#L146-L160
+	// Boulder http.Client:    https://github.com/letsencrypt/boulder/blob/30a516737c9daa4c88c8c47070c25a5e7033cdcf/va/http.go#L567-L572
+
+	// Boulder uses a much more complex timeout setup involving shaving time off the deadline to be able to differentiate
+	// between timeouts at different stages of the connection and in turn provide for better error messages. We're a little
+	// more blunt than that, and just use a static timeout of 10 seconds in http.Client.
+
+	// That said, IdleConnTimeout is not covered by `Timeout` in http.Client, so we also set it in our Transport
+
+	// See https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/#clienttimeouts for details on timeouts
 	transport := &http.Transport{
 		Proxy: http.ProxyFromEnvironment,
 		// we're only doing 1 request, make the code around this
 		// simpler by disabling keepalives
 		DisableKeepAlives: true,
+
+		// boulder sets this to 1 because "0" means "unlimited"
+		MaxIdleConns: 1,
+
+		// IdleConnTimeout's value is taken from Boulder
+		IdleConnTimeout: time.Second,
+
 		TLSClientConfig: &tls.Config{
+			// If we're following a redirect, it's permissible for it to be HTTPS and
+			// its certificate may be invalid (they are trying to get a certificate, after all!)
+			// See: https://letsencrypt.org/docs/challenge-types/#http-01-challenge
+			// > When redirected to an HTTPS URL, it does not validate certificates (since
+			// > this challenge is intended to bootstrap valid certificates, it may encounter
+			// > self-signed or expired certificates along the way).
 			InsecureSkipVerify: true,
 		},
 	}
-	client := http.Client{
+
+	client := &http.Client{
 		Transport: transport,
+		Timeout:   time.Second * 10,
 	}
 
 	response, err := client.Do(req)