Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate HTTP-01 challenges and 308 redirects #4256

Closed
cpu opened this issue Jun 11, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@cpu
Copy link
Member

commented Jun 11, 2019

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:

boulder/va/http.go

Lines 614 to 617 in 3de2831

if httpResponse.StatusCode != 200 {
return nil, records, berrors.UnauthorizedError("Invalid response from %s [%s]: %d",
records[len(records)-1].URL, records[len(records)-1].AddressUsed, httpResponse.StatusCode)
}

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

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

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:

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

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

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

This comment has been minimized.

Copy link

commented Jun 13, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

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.

@cpu cpu closed this Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.