feat: improve acme-challenge handling

[pini-gh]

So that there is no need anymore for the Let's Encrypt companion to fiddle with vhosts nginx configuration.

Bcause it creates http servers to handle acme challenge requests, a few tests which previously refused http connexion now return 503.

[buchdag]

I'm not certain about silently running an HTTP server for the sole purpose of the acme-challenge when using nohttp.

To me using nohttp when wanting to obtain ACME TLS certificate with the HTTP challenge should fail without exception, because not blocking port 80 at all is part of LE's best practice (with very sound arguments imho), and the RFC8555 section 8.3 itself indicate that first connection of an ACME HTTP challenge must happen on port 80, making it by design incompatible with attempt to block this port:

3. Dereference the URL using an HTTP GET request. This request MUST be sent to TCP port 80 on the HTTP server

I think we should instead update the documentation to warn people that nohttp will be incompatible with the ACME HTTP challenge, like in acme-companion's documentation.

Explicit use of nohttp by users should be done knowingly (ie not for security misconceptions) and observed entirely by nginx-proxy.

[buchdag]

The use case of people absolutely having to block port 80 yet still wanting ACME TLS certificate should rather be handled by making acme-companion compatible with DNS challenge (the next "big" feature I'd like to get merged).

[pini-gh]

There is a very simple solution when rigorously no http server is wanted: do not bind port 80.

What makes me uncomfortable is acme-companion fiddling into the nginx configuration. It shouldn't have to.

How about enabling acme challenge if and only if:

• the nginx-proxy container has ACME_CHALLENGE=true
• or the app container has ACME_CHALLENGE=true?
• or the app container defines the LETSENCRYPT_HOST variable,

If you agree on none of these proposals, I'd be OK with not enabling acme challenge when HTTPS_METHOD=nohttp.

What do you think?

[buchdag]

What makes me uncomfortable is acme-companion fiddling into the nginx configuration. It shouldn't have to.

Agreed, that's been a long standing rough edge of the integration between acme-companion and nginx-proxy.

Regarding nohttp, in addition to the above arguments I think the proxy should honor it even if it result in a "faulty" configuration (kind of like what you did for VIRTUAL_PORT a few years ago), rather than automagically correcting the user's error without its knowledge (ie the previous behaviour of VIRTUAL_PORT).

But I might be missing valid use cases of using nohttp and still wanting to be able to complete ACME HTTP challenge, do you have some examples ?

If we decide to enable ACME challenge when HTTPS_METHOD=nohttp, I'd go with the third solution (based on wether the app container defines the LETSENCRYPT_HOST variable or not).

[pini-gh]

But I might be missing valid use cases of using nohttp and still wanting to be able to complete ACME HTTP challenge, do you have some examples ?

Unfortunately I don't know any use case for HTTPS_METHOD=nohttp.

If we decide to enable ACME challenge when HTTPS_METHOD=nohttp, I'd go with the third solution (based on wether the app container defines the LETSENCRYPT_HOST variable or not).

If you want to give it more thinking I could split this PR in two:

1. Handle acme challenge but for HTTPS_METHOD=nohttp
2. When HTTPS_METHOD=nohttp handle acme challenge only if the app container defines LETSENCRYPT_HOST.

Actually I'm not sure (2) is needed, So I'll go for the split.

[buchdag]

That was maybe unclear but the only gripe I had was with nohttp (so only with 2.), the rest of the PR make sense to me and I was about to ping you on this issue. I don't have any issue merging a PR for 1. 👍

Edit : I think we might have to enable the challenge only for app containers that have the LETSENCRYPT_HOST variable set, I think I remember a discussion about this a while ago with someone saying that a hardcoded ACME challenge location was undesirable to them because they were doing their challenge validation themselves (ie not with acme-companion), with a challenge location setup differently. I'll try to search for this discussion.

[pini-gh]

Edit : I think we might have to enable the challenge only for app containers that have the LETSENCRYPT_HOST variable set,

This might break existing use cases where acme-companion is not used but acme challenge is needed. We could do the other way around: do not enable acme challenge when NO_ACME_HTTP_CHALLENGE=true.

EDIT: But I'd prefer to implement this via another PR.

[pini-gh]

EDIT: But I'd prefer to implement this via another PR.

This is #2447.

[buchdag]

I feel like we're trying to change behaviours that aren't really related to ACME challenge here. Could we list the incorrect ACME challenge behaviours we want to fix ?

[buchdag]

I'm not sure about this new behaviour. In my mind an unknown hostname should always be a 503, regardless of the configuration of the proxied service containers. Having a container with HTTPS_METHOD=nohttp should not change how the proxy respond to non configured hostnames.

Maybe there is a confusion around HTTPS_METHOD=nohttp but to me it means disable HTTP entirely for this vhost, and this vhost only.

[pini-gh]

I'm not sure about this new behaviour. In my mind an unknown hostname should always be a 503, regardless of the configuration of the proxied service containers. Having a container with HTTPS_METHOD=nohttp should not change how the proxy respond to non configured hostnames.

This behavior is not new and not my idea. It was introduced by #2186 and could be reverted by unconditionally enabling the http fallback server.

Maybe there is a confusion around HTTPS_METHOD=nohttp but to me it means disable HTTP entirely for this vhost, and this vhost only.

Agreed.

[pini-gh]

Could we list the incorrect ACME challenge behaviours we want to fix ?

Only one case: when HTTPS_METHOD=noredirect.

EDIT: this is highlighted by this change

[buchdag]

I'm sorry, I think I'm getting a bit lost here and making you circle around the issue this PR is trying to fix.

I feel like this (inserted between L764 and L765):

        {{- if (eq $vhost.https_method "noredirect") }}
    location /.well-known/acme-challenge/ {
        auth_basic off;
        allow all;
        root /usr/share/nginx/html;
        try_files $uri =404;
        break;
    }
        {{- end }}

Should be enough to fix the ACME challenge location for HTTPS_METHOD=noredirect. Am I missing something or misunderstanding the current template ?

From what I remember of #2186, the aim was mostly to stop the proxy from serving incorrect TLS certificates in some cases. If it introduced unwanted behaviour, could we discuss and fix them in a separate PR ?

[pini-gh]

Okay then, let's go step by step.