diff --git a/pkg/issuer/acme/http/http.go b/pkg/issuer/acme/http/http.go index 2914d5c2d..9bd9e5e65 100644 --- a/pkg/issuer/acme/http/http.go +++ b/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)