Permalink
Browse files

follow location on http get-requests

  • Loading branch information...
lukas2511 committed Feb 3, 2018
1 parent 45f5c17 commit 7a0e71c6c2ccc6e98abca5ea1c7de28053e90c02
Showing with 1 addition and 1 deletion.
  1. +1 −1 dehydrated
@@ -458,7 +458,7 @@ http_request() {
statuscode="$(curl ${ip_version:-} ${CURL_OPTS} -s -w "%{http_code}" -o "${tempcont}" "${2}" -I)"
curlret="${?}"
elif [[ "${1}" = "get" ]]; then
statuscode="$(curl ${ip_version:-} ${CURL_OPTS} -s -w "%{http_code}" -o "${tempcont}" "${2}")"
statuscode="$(curl ${ip_version:-} ${CURL_OPTS} -L -s -w "%{http_code}" -o "${tempcont}" "${2}")"
curlret="${?}"
elif [[ "${1}" = "post" ]]; then
statuscode="$(curl ${ip_version:-} ${CURL_OPTS} -s -w "%{http_code}" -o "${tempcont}" "${2}" -d "${3}")"

13 comments on commit 7a0e71c

@pvizeli

This comment has been minimized.

pvizeli replied Feb 24, 2018

Can we have a 0.5.1 release with this?

@lukas2511

This comment has been minimized.

Owner

lukas2511 replied Feb 24, 2018

@pvizeli I'm planning to release 0.6.0 in about 2-3 weeks

@elarzee

This comment has been minimized.

elarzee replied Mar 11, 2018

shouldn't the "-L" option go into every curl invocation?
plus "--post301" for POST-ing?

@lukas2511

This comment has been minimized.

Owner

lukas2511 replied Mar 11, 2018

@elarzee normally there shouldn't be any redirects to begin with since (nearly) all URLs are dictated by the CA to begin with, so i'm a bit conservative about following every redirect

@elarzee

This comment has been minimized.

elarzee replied Mar 14, 2018

i understand your concern (and appreciate your conservativism, especially when dealing with security-relevant stuff!). but there are redirects (since two or three weeks).
applying your patch didn't do it for me. and, running operations for rather fidgety start-ups, i added the -L to all curl invocations without much hesitation / consideration to make my clients happy.
the blame, as i see it, is basically on letsencrypt, right? for introducing redirects.

@lukas2511

This comment has been minimized.

Owner

lukas2511 replied Mar 14, 2018

@elarzee i don't know about any redirects other than for GET requests and with the current ACMEv2 implementation they shouldn't happen anymore since the affected part of the code will be completely skipped

@CyrilBrulebois

This comment has been minimized.

CyrilBrulebois replied Mar 15, 2018

Working together with the dehydrated maintainer in Debian, the release team would like to double check that cherry-picking this patch indeed fixes issues with 301 redirects.

I've seen this personally in various crontab mails during last week-end, but I can't reproduce this issue with the production infrastructure at the moment. Were those redirects temporary and handled by some parts of the LE infrastructure? Looking at boulder's git log, I can't find anything relevant over the last few weeks.

Others mention this seems to have been intermittent (e.g. https://community.letsencrypt.org/t/dehydrated-caused-rate-limits-to-be-reached/52477/3)

Any hints appreciated, thanks!

@elarzee

This comment has been minimized.

elarzee replied Mar 15, 2018

for me, the fix did do the trick. though i have to admit i wouldn't bet my life on whether the redirect was there for POST requests, as @lukas2511 insinuates. i had assumed so, and simply (and very professionally... ;) added the -L everywhere.
and i can confirm, there are no 301s, currently. i checked on three different hosts, with the patch removed.

@lukas2511

This comment has been minimized.

Owner

lukas2511 replied Mar 15, 2018

@CyrilBrulebois the issue only affected the walk_chain function, which is basically the only part where dehydrated doesn't directly use URLs given by the api directory but rather parses the certificates for issuer urls. For ACMEv2 (>=dehydrated 0.6.0) this doesn't happen anymore, for the old API version it only consisted of GET requests, those now do follow redirects.

Since there is so much confusion and it's probably not much danger I guess I'll just allow the other methods to follow redirects too...

@CyrilBrulebois

This comment has been minimized.

CyrilBrulebois replied Mar 15, 2018

Thanks for the clarification.

Debian stable is based on 0.3.1, so isn't using ACMEv2; with the -L added by this commit, redirects in GET requests are followed, but I'd like to figure out why I can't reproduce the issue (with an unpatched client) right now, whereas it failed for a bunch of machines last week-end…

@lukas2511

This comment has been minimized.

Owner

lukas2511 replied Mar 15, 2018

@CyrilBrulebois It was unrelated to the ACME protocol itself, it was something that happened to the location behind the issuer URI of the certificate (e.g. http://cert.stg-int-x1.letsencrypt.org/) which dehydrated uses to find the chain using the walk_chain method. When I first heard this myself I checked and found that this URL seemed to be load-balanced with a few servers replying with a 301 and others replying with the certificate. That's why some had this issue and others didn't and it's possible that those redirects are now gone and with that the issue magically disappeared (for now).

@lukas2511

This comment has been minimized.

Owner

lukas2511 replied Mar 15, 2018

@elarzee @CyrilBrulebois I had another look, and I don't think following redirects everywhere is a good idea... there are some flaws with that:

  • following redirects in the head method results in curl printing those redirect headers too, making parsing a bit more complex
  • --post301 has only been implemented around 2007. This might sound a bit odd but since dehydrated kinda made a name for itself being compatible with very old systems this actually is a bit annoying

Those both are definitively fixable, but as stated before only URLs given by the CA are actually ever used with POST and HEAD requests and those URLs come directly as absolute URLs from the API. There should never be a reason for a 301 redirect for these, I'd consider redirects behind those URLs a bug from the CA. So I'm saving myself from a few bad workarounds that never had to exist to begin with.

The only method actually seeing user input and parsed URLs is the GET request and with this commit redirects will be followed on those.

@elarzee

This comment has been minimized.

elarzee replied Mar 15, 2018

@lukas2511 you got me totally convinced. you might have guessed, i'm not exactly an expert on the matter...
thanks for your work!

Please sign in to comment.