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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[enh] external http checker fallback for LE checks #623

Open
wants to merge 2 commits into
base: stretch-unstable
from

Conversation

Projects
None yet
2 participants
@Psycojoker
Copy link
Member

Psycojoker commented Jan 22, 2019

The problem

In some annoying network configuration, a YunoHost installation can't do a HTTP loopback on itself using its public ip (a bit like hairpining for rooters) but still can be reached from the outside which is what LE will use to install its certificate. In consequence, people needs to had --no-checks while it's going to work, our test (and acme-tiny one) is too restrictive.

Generally, this kind of YunoHost instance are NATed as a vm/container.

Solution

When the "let's test I'm accessible by HTTP" test fails, fallback on an external HTTP checker that will do the same test but from outside which will avoid the bug where HTTP can loopback from localhost.

The outside http checker is https://github.com/YunoHost/check-http that I wrote and test and you can get the general workflow documentation here: https://github.com/YunoHost/check-http/blob/master/server.py#L51

PR Status

Untested because http-check isn't deploy yet (we need a YunoHost application for that if we want to deploy it correctly).

On the other hand this is a pretty safe PR since it only add a new fallback behavior, in the worst case it will only failed to do the external HTTP check in a situation where it was already failing.

How to test

You need a YunoHost instance where you can do HTTP loopback, had this patch, wait for http-check to be deployed and then you can try to renew/install a certificate without --no-checks.

I have an instance in that situation and I can do this test but ... we need to merge this PR for our toolchan to build a .deb x( ... (or I can overwrite the local files I guess 馃 ... but that's dirty).

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
# this mean that the domain couldn't be reached with a known exception
# (timeout for now)
# also talks about teapot
if response.status_code == 418:

This comment has been minimized.

@alexAubin

alexAubin Jan 22, 2019

Member

Teapots are awesome 馃憤 but I can't see any code in the server.py returning 418 ? Or am I missing something 馃

This comment has been minimized.

@Psycojoker

Psycojoker Jan 22, 2019

Author Member

Oupsi I forgot to push >_<

Here it is YunoHost/check-http@7d868ec

This comment has been minimized.

@Psycojoker

Psycojoker Jan 22, 2019

Author Member

Also I'm using 418 because I couldn't find any other code that would be "200 but actually it's False" :/

# care about local loopback, it wants outside reachability)

try:
response = requests.post("https://http-check.yunohost.org/check/", data={"domain": domain})

This comment has been minimized.

@alexAubin

alexAubin Jan 22, 2019

Member

That's more a comment on the server side than the client side, but shouldn't we actually check that this is a yunohost instance rather than just if it answers to HTTP request ? (maybe checking for SSOwat headers ? idk) I'm concerned about routers running a web server themselves (this is more and more the case) hence that would bring false-positives in that case.

More generally I'm wondering if this ain't a bit like reinventing sort of the ACME challenge ourselves (if we wanted to cover more complicated use case with several yunohost in reverse-proxy config and so on, we would be generating a random id and check if the server sees the correct id) and if we just shouldn't use the staging server of Let's Encrypt instead of running our own service with the (small, but existent) maintenance cost that comes with it :s

This comment has been minimized.

@Psycojoker

Psycojoker Jan 22, 2019

Author Member

That's more a comment on the server side than the client side, but shouldn't we actually check that this is a yunohost instance rather than just if it answers to HTTP request ? (maybe checking for SSOwat headers ? idk) I'm concerned about routers running a web server themselves (this is more and more the case) hence that would bring false-positives in that case.

That would be the next level of check, that would indeed be better and:

we would be generating a random id and check if the server sees the correct id

Would also be the way I would do it. Right now I wanted to only do the next step to fix the most common bugs and escalate to a more advanced solution if needed (I really don't like huge PR that are hard to read).

Also because I don't have a lot of energy and motivation right now and it was already quite some work to do >_<

More generally I'm wondering if this ain't a bit like reinventing sort of the ACME challenge ourselves [...] and if we just shouldn't use the staging server of Let's Encrypt instead of running our own service with the (small, but existent) maintenance cost that comes with it :s

On one hand yes, on the other hand I had in mind to use this new server in the future diagnostic tool to check if a domain is correctly configured so that would be more than just just redoing LE check but I understand your sentiment.

This comment has been minimized.

@alexAubin

alexAubin Jan 22, 2019

Member

check if a domain is correctly configured so that would be more than just just redoing LE check but I understand your sentiment.

Well in fact yes, I was thinking that the whole ip/domain rate thing looks pretty neat and that would definitely be useful to check the ports (instead of the old ports.yunohost.org which i believe don't have any rate limiting thing and isn't 100% reliable :P)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment