fix: enforce HTTPS_METHOD on missing cert as well

[pini-gh]

There is no reason to silently switch to noredirect on missing cert.

The default being HTTPS_METHOD=redirect most of the test cases need to set HTTPS_METHOD=nohttps or HTTPS_METHOD=noredirect.

[buchdag]

@pini-gh If we're updating a behaviour of nginx-proxy, shouldn't we update the test assertions rather than the test setups ?

[pini-gh]

I don't think so. The related tests use HTTP URLs, while default HTTPS_METHOD is redirect, i.e. HTTP cannot be tested.

To explicitly enable HTTP mode we need to set HTTPS_METHOD=nohttps or HTTPS_METHOD=noredirect.

The other way around would be to configure valid certificates for all these tests, which seems cumbersome to me.

[buchdag]

Okay, got it. I'd prefer HTTPS_METHOD=noredirect where possible because it's kinda "less specific" than HTTPS_METHOD=nohttps, as in we're not specifically testing the absence of an HTTPS listener here. Does that make sense ?

[pini-gh]

To me HTTPS_METHOD=nohttps is more appropriate as it states explicitly that the current test doesn't involve HTTPS at all. Moreover, these tests don't have valid certificates, so HTTPS wouldn't work anyway.

But I won't object setting noredirect if you stick to it. You tell me.

[pini-gh]

Because this PR may change the behavior of existing setups I could add an environment variable ENFORCE_HTTPS_METHOD with default value false. What do you think?

[buchdag]

After carefully re-reading and testing this PR I'm afraid we're going in the wrong direction, we actually do want to silently switch to noredirect when there is no certificate present for a given vhost, because HTTPS_METHOD should only affect vhost that have HTTPS enabled (ie that have a certificate):

• HTTPS not enabled -> noredirect and HTTPS_METHOD has no effect
• HTTPS enabled -> redirect by default, overridden by HTTPS_METHOD

As you remarked in your previous message, changing this to make the proxy enforce HTTPS_METHOD on HTTP only vhost would break pretty much every HTTP only config. I think I misled you in #2446 and drew an invalid parallel between the behaviour of nohttp and the breaking changes we made to VIRTUAL_HOST a few years ago.

To get a better view of the proxy behaviour and see if things need to be fixed, I tried to test it a bit more exhaustively and in an easier to understand way than in the fallback test:

Unknown vhosts

SSL_ERROR_SYSCALL is what I got testing with curl on my host, I'm not certain how to interpret it or why I get this when at least one HTTP / HTTPS vhost is running.

default certificate	at least one HTTP vhost	at least one HTTPS vhost	HTTPS Method (on proxy)	HTTP response	HTTPS response	Certificate served
❌	❌	❌	not set	503	444 (TLS Error)	n/a
❌	❌	❌	redirect	503	444 (TLS Error)	n/a
❌	❌	❌	noredirect	503	444 (TLS Error)	n/a
❌	❌	❌	nohttp	503	444 (TLS Error)	n/a
❌	❌	❌	nohttps	503	444 (TLS Error)	n/a
🟢	❌	❌	not set	503	503	default
🟢	❌	❌	redirect	503	503	default
🟢	❌	❌	noredirect	503	503	default
🟢	❌	❌	nohttp	503	503	default
🟢	❌	❌	nohttps	503	503	default
❌	🟢	❌	not set	503	444 (TLS Error)	n/a
❌	🟢	❌	redirect	503	444 (TLS Error)	n/a
❌	🟢	❌	noredirect	503	444 (TLS Error)	n/a
❌	🟢	❌	nohttp	503	444 (TLS Error)	n/a
❌	🟢	❌	nohttps	503	SSL_ERROR_SYSCALL	n/a
🟢	🟢	❌	not set	503	503	default
🟢	🟢	❌	redirect	503	503	default
🟢	🟢	❌	noredirect	503	503	default
🟢	🟢	❌	nohttp	503	503	default
🟢	🟢	❌	nohttps	503	SSL_ERROR_SYSCALL	n/a
❌	🟢	🟢	not set	503	444 (TLS Error)	n/a
❌	🟢	🟢	redirect	503	444 (TLS Error)	n/a
❌	🟢	🟢	noredirect	503	444 (TLS Error)	n/a
❌	🟢	🟢	nohttp	503	444 (TLS Error)	n/a
❌	🟢	🟢	nohttps	503	SSL_ERROR_SYSCALL	n/a
🟢	🟢	🟢	not set	503	503	default
🟢	🟢	🟢	redirect	503	503	default
🟢	🟢	🟢	noredirect	503	503	default
🟢	🟢	🟢	nohttp	503	503	default
🟢	🟢	🟢	nohttps	503	SSL_ERROR_SYSCALL	n/a

Known vhosts

The only "issue" I see here is nohttps behaving differently in the absence and presence of a default certificate.

default certificate	vhost certificate	HTTPS Method	HTTP response	HTTPS response	Certificate served
❌	❌	not set	200 ✅	444 ⛔️	n/a
❌	❌	redirect	200 ✅	444 ⛔️	n/a
❌	❌	noredirect	200 ✅	444 ⛔️	n/a
❌	❌	nohttp	200 ✅	444 ⛔️	n/a
❌	❌	nohttps	200 ✅	444 ⛔️	n/a
🟢	❌	not set	200 ✅	500 ⛔️	default
🟢	❌	redirect	200 ✅	500 ⛔️	default
🟢	❌	noredirect	200 ✅	500 ⛔️	default
🟢	❌	nohttp	200 ✅	500 ⛔️	default
🟢	❌	nohttps	200 ✅	503 ⛔️	default
❌	🟢	not set	301 ➡️	200 ✅	vhost
❌	🟢	redirect	301 ➡️	200 ✅	vhost
❌	🟢	noredirect	200 ✅	200 ✅	vhost
❌	🟢	nohttp	503 ⛔️	200 ✅	vhost
❌	🟢	nohttps	200 ✅	444 ⛔️	n/a
🟢	🟢	not set	301 ➡️	200 ✅	vhost
🟢	🟢	redirect	301 ➡️	200 ✅	vhost
🟢	🟢	noredirect	200 ✅	200 ✅	vhost
🟢	🟢	nohttp	503 ⛔️	200 ✅	vhost
🟢	🟢	nohttps	200 ✅	503 ⛔️	default

Let me know if you see other behaviours that seem incorrect or if I forgot some cases.

[pini-gh]

HTTPS_METHOD should only affect vhost that have HTTPS enabled (ie that have a certificate)

That's where I'd want a different behavior. A certificate going missing could be a bug or a mistake. For my sites I don't want HTTP silently enabled on such cases. Then I need a way to enforce redirect or nohttp.

There are several ways to achieve this. If we don't want to change the current default behavior I see two ways:

1. Guard the change with environment variable ENFORCE_HTTPS_METHOD
2. Add new values for HTTPS_METHOD:
   • redirect_enforced
   • nohttp_enforced

Would you agree on one of these?

EDIT: Fixed typo noredirect -> redirect

[pini-gh]

I've created PR #2465 to make the current behavior explicit and documented.

[buchdag]

That's where I'd want a different behavior. A certificate going missing could be a bug or a mistake. For my sites I don't want HTTP silently enabled on such cases.

Ok I totally get this use case 👍

Ideally I'd prefer if we did not introduce an entirely new environment variable (to avoid adding one dimension to the previous behaviour tables), so redirect_enforced / nohttp_enforced.

[pini-gh]

May I insist on a using a new variable? Say ENABLE_HTTP_ON_MISSING_CERT (default true)? Because it precisely conveys what it is for. While redirect_enforced and nohttp_enforced look like poor workarounds compared to it.

The new variable doesn't add a full new dimension. In both cases we just need a couple of tests where we check that HTTP is not enabled on missing cert when HTTPS_METHOD=redirect or HTTPS_METHOD=nohttp.

[buchdag]

I guess I really liked my tables above ^^

Let's roll with ENABLE_HTTP_ON_MISSING_CERT, defaulting to true.

[pini-gh]

Let's roll with ENABLE_HTTP_ON_MISSING_CERT, defaulting to true.

This PR should be ready then :)

[buchdag]

I'm not certain formulating it this way really highlight the use case for the new environment variable.

If you're not in a hurry I can try to come up with something.