Automatically disable the HTTP port when client certs are required

[cescoffier]

This is particularly important when mTLS is used.

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

IMHO if REQUIRED is set then no plain port should be open by default, so the proposal is to fail the build if insecure requests are not disabled but doc that users can open it if they really need it

[sberyozkin]

@cescoffier

the proposal is to fail the build if insecure requests are not disabled but doc that users can open it if they really need it

That was incomplete, having REQUEST would allow such requests and enable a semi secured transport level protection.

I honestly don't think disabling insecure requests on the main router with REQUIRED can break anyone's code, as REQUIRED is a very purposeful mode.

I can open a PR if you'd like me to.

[sberyozkin]

My only doubt is whether IMHO if REQUIRED is set then no plain port should be open by default is correct or not. May be it sounds too restrictive as users may want to limit MTLS to specific paths, but it appears to me a mainstream MTLS case is to ensure the client is authenticated at the transport level, so having to disable the insecure port seems like an extra action too many in this case. It would be fairer if users who wanted to relax this all around MTLS protection would open the insecure port if they want to...
@cescoffier @michalvavrik @gsmet , does it sound reasonable ? I think we should try to be more opinionated in this regard, if MTLS is REQUIRED, 100% HTTPS is enforced, that is not too drastic, if it appears to be the case to some users, just open the insecure port, instead of users who want to do 100% HTTPS having to do it :-)

[michalvavrik]

My only doubt is whether IMHO if REQUIRED is set then no plain port should be open by default is correct or not. May be it sounds too restrictive as users may want to limit MTLS to specific paths, but it appears to me a mainstream MTLS case is to ensure the client is authenticated at the transport level, so having to disable the insecure port seems like an extra action too many in this case. It would be fairer if users who wanted to relax this all around MTLS protection would open the insecure port if they want to...
@cescoffier @michalvavrik @gsmet , does it sound reasonable ? I think we should try to be more opinionated in this regard, if MTLS is REQUIRED, 100% HTTPS is enforced, that is not too drastic, if it appears to be the case to some users, just open the insecure port, instead of users who want to do 100% HTTPS having to do it :-)

I think it sounds reasonable. It's also potentially breaking change, FWIW +1

[cescoffier]

@sberyozkin There is a problem with this solution. Both REQUIRED and REDIRECTED make sense. It's up to the user to decide.

I agree that REQUIRED is probably better, but it is a strong opinion. So, we would need to log something explaining that we took this decision because the user didn't set any value.

[cescoffier]

@sberyozkin is there an easy way to know if mTLS is configured? I was wondering if looking if tlsClientAuth is set to REQUIRED. However, I do not believe it capture everything.

Disabling insecure ports by default is a breaking change, difficult to evaluate the consequences

[sberyozkin]

@cescoffier Right, at build time it is checked if it is set to REQUIRED or REQUEST and if yes then it is assumed it is enabled.

I've dismissed my review, even though I feel it is worth be stricter by default, when REQUIRED is set, I don't understand all the consequences of disabling insecure ports by default.
Also, when the Quarkus Security is used to enforce the authentication alongside REQUIRED, insecure HTTP requests will be rejected, but thinking more about it, disabling them earlier at the transport level may actually be more performant.

Please fix the conflict, sorry it took me a while to think about it

[cescoffier]

@sberyozkin Can you review, I automatically disable the plain server.

[sberyozkin]

@cescoffier

I automatically disable the plain server.

That is the Quarkus opinionated way :-) 👍
Worth trying for 3.7.0.CR1 and see if there will be some concerns, may be only start with the warning, let me comment there

[sberyozkin]

I can propose a migration note once this one is merged, even though technically it is a breaking change, as suggested above, I have high hopes very few users if any will notice it

[cescoffier]

The CI issue is most probably related.

[sberyozkin]

@cescoffier FYI, I prototyped https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.7#insecure-http-port-is-disabled-when-mtls-client-authentication-is-required.

Please do not hesitate to change/fix it as you prefer, it would be fine. Thanks

[cescoffier]

@sberyozkin I changed it a bit.

[sberyozkin]

@cescoffier This one is a new test, which got into the build after the rebase :-), I can push an update to it to have it accepting insecure requests or the test can validate that the HTTP connection is closed to verify the insecure port is effectively disabled - how to correctly check it though ?

[cescoffier]

@sberyozkin Connection refused is exactly what we want in this case. Could you adapt the test to check for that exception? Otherwise, enable the the "plain" HTTP server.

[cescoffier]

There are a few import * statement to cleanup.

[quarkus-bot]

Failing Jobs - Building 6f8706c

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	Maven Tests - JDK 17	Build	Failures	Logs	Raw logs	🚧
✔️	Maven Tests - JDK 17 Windows					🚧

Full information is available in the Build summary check run.

Failures

⚙️ Maven Tests - JDK 17 #

- Failing: integration-tests/maven 

📦 integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 967 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "d9b423a4-86c5-42ce-ac2c-206f98e12f26" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AbstractHamcrestCondition.await(AbstractHamcrestCondition.java:86)

✖ io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 967 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: io.quarkus.maven.it.DevMojoIT expected "d9b423a4-86c5-42ce-ac2c-206f98e12f26" but was "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." within 2 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AbstractHamcrestCondition.await(AbstractHamcrestCondition.java:86)

[sberyozkin]

These test failure are not related