Boulder: Investigate HTTP-01 challenges and 308 redirects

[cpu]

There was a report in the community forum about HTTP-01 challenges failing when a 308 redirect is used in place of 301 or otherwise. Our existing integration tests use 302.

I looked at the Go stdlib and the VA's redirect processing code and couldn't find an explicit reason that a 307 or 308 redirect wouldn't work.

Based on the error message from the OP:

"urn:ietf:params:acme:error:unauthorized :: The client lacks sufficient authorization :: Invalid response from https://www.sharparam.com/.well-known/acme-challenge/AKeTKrbC6CL3uSKwRKlfvsbn-zRTbuV6EikGmKA9EK0 [2606:4700:30::681c:1959]: 308."

I believe this is the site of the rejection in the VA's http-01 code, _after_ all the redirects have been processed and we're evaluating the terminal HTTP response for the HTTP-01 challenge: https://github.com/letsencrypt/boulder/blob/3de2831c329932a58814110102df884d3d576e5f/va/http.go#L614-L617

It seems very strange that the terminal request would have a status code of 308. The stdlib http docs do describe 307/308 acting differently than other codes, particularly with respect to the body and method being reused but I wouldn't expect these differences to matter for the VA code in question.

If the server replies with a redirect, the Client first uses the CheckRedirect function to determine whether the redirect should be followed. If permitted, a 301, 302, or 303 redirect causes subsequent requests to use HTTP method GET (or HEAD if the original request was HEAD), with no body. A 307 or 308 redirect preserves the original HTTP method and body, provided that the Request.GetBody function is defined. The NewRequest function automatically sets GetBody for common standard library body types.

I think it makes sense to try and reproduce this issue to understand it better. It may be an end user misconfiguration, it may be a VA bug. Ultimately if we decide to block some subset of 3xx redirect status codes we should make a better error message.

[jsha]

I thought this might be due to our CheckRedirect returning an error for an unrelated reason, and causing net/http to return the last result, which would be a 308, but in that case client.Do would pass through the error, which would cause processHTTPValidation to return early with that error.

https://godoc.org/net/http

// CheckRedirect specifies the policy for handling redirects.
// If CheckRedirect is not nil, the client calls it before
// following an HTTP redirect. The arguments req and via are
// the upcoming request and the requests made already, oldest
// first. If CheckRedirect returns an error, the Client's Get
// method returns both the previous Response (with its Body
// closed) and CheckRedirect's error (wrapped in a url.Error)
// instead of issuing the Request req.
// As a special case, if CheckRedirect returns ErrUseLastResponse,
// then the most recent response is returned with its body
// unclosed, along with a nil error.

[cpu]

I started to pipe through the changes that would be needed to write an integration test for this to get a chance to see what happens at runtime without ambiguity:

• https://github.com/letsencrypt/challtestsrv/tree/cpu-custom-redirect-status-code
• https://github.com/letsencrypt/pebble/tree/cpu-custom-redirect-status-code

I made it as far as building a Boulder tools image locally that used the cpu-custom-redirect-status-code branch of pebble for pebble-challtestsrv but then ran into some Certbot venv thing and put everything on hold to focus on my assigned work.

Leaving these pieces here to pick up later.

[jsha]

I posted over at https://community.letsencrypt.org/t/certbot-does-not-follow-308-redirects/95607/4?u=jsha - it's possible this is an Nginx problem (failing to set a Location header for 308's) rather than a Boulder problem.

[cpu]

it's possible this is an Nginx problem (failing to set a Location header for 308's) rather than a Boulder problem.

That would make sense! I saw in the stdlib net/http/client.go code that redirectBehavior() calls this out specifically:

        // Treat 307 and 308 specially, since they're new in
        // Go 1.8, and they also require re-sending the request body.
        if resp.Header.Get("Location") == "" {
            // 308s have been observed in the wild being served
            // without Location headers. Since Go 1.7 and earlier
            // didn't follow these codes, just stop here instead
            // of returning an error.
            // See Issue 17773.
            shouldRedirect = false
            break
        }

I think the key part is "just stop here instead of returning an error". I bet that would lead to a condition where no error was returned and the terminal request has the 307/308 status code and gets rejected by the VA.

[Sharparam]

Like @jsha pointed out the missing Location header was probably the issue. Nginx 1.14 (or some version after 1.10) adds a Location header (missing in 1.10 which is what the server was running).

Funnily enough, in 1.14 the URL is no longer present in the response body.

[cpu]

Thanks for confirming @Sharparam! This was an interesting little puzzle :-)

I'm going to close this issue. I don't think this is a common enough corner-case to merit building out a more specific error message and switching the redirect code or properly sending a Location header fix the problem.