Provide fluent API to set up the mutual TLS client authentication

[michalvavrik]

• adds mTLS portion of the Provide a fluent API to set up Quarkus Security #16728 enhancement request

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

Hmm, seems that in the DEV mode "after failed start" scenario config needs to be propagated from the recorder, I'll adjust logic accordingly.

[sberyozkin]

Hi @michalvavrik, thanks, PR looks good to me, as far as the iterative fluent API evolution is concerned.

But, I'm not exactly sure if it gives an advantage, at this moment of time, over the configuration - where we still need to setup everything related to the actual MTLS mechanics.

It only enables a switch for the client auth required, but MTLS is activated at the transport level even without this flag, without quarkus.http.ssl.client-auth=required, given that, and the presence of the possibly multiple TLS registry entries, I think we need to discuss it more how to do (M)TLS with the fluent API.

[michalvavrik]

Hi @michalvavrik, thanks, PR looks good to me, as far as the iterative fluent API evolution is concerned.

But, I'm not exactly sure if it gives an advantage, at this moment of time, over the configuration - where we still need to setup everything related to the actual MTLS mechanics.

As far as advantage (or should we call this motivation) goes:

1. we want to provide a programmatic way to configure what users can do in a properties file because some users prefer it
2. this allows to enable mTLS client authentication at runtime unlike the quarkus.http.ssl.client-auth build-time configuration property (and no, we can't simply move that property to runtime because it required code changes related as you can see)
3. it allows flexibility in mapping certificate to SecurityIdentity roles, you documented that users can use the augmentor, but this API requires less code than definining the augmentor and is more straightforward, because it is in a one place.

It only enables a switch for the client auth required, but MTLS is activated at the transport level even without this flag without quarkus.http.ssl.client-auth=required

I am quite sure you are wrong. Can you rephrase this so that I can understand what you mean, please? There can be something I am missing. Without this PR you need to set quarkus.http.ssl.client-auth to get the client auth. With this PR you don't need to set this property in your properties file You can also disable insecure requests in the properties file (which is done by default when the property is REQUIRED), but that is not mTLS.

given that, and the presence of the possibly multiple TLS registry entries, I think we need to discuss it more how to do (M)TLS with the fluent API.

I understand what you try to say, that users need to configure TLS registry, but:

1. we cannot do that in one PR, this PR is already big enough
2. people configure TLS registry regardless of client authentication (like for HTTPS or for OIDC and so on...) so I think the fluent API for security needs to deal with security not the TLS registry

I am happy to follow-up with TLS registry. If you say that the way we provide TLS registry fluent API will change this API, maybe, in that case indeed I have no answer.

[michalvavrik]

Strictly speaking, I could add some support for adding certificates in this PR, but I really think it would be better to keep it small even for you, it must make reviews easier.

[sberyozkin]

Hi Michal I think it should be a draft, and we discuss

[michalvavrik]

Hi Michal I think it should be a draft, and we discuss

Ok, turning it into a draft until you say it is ready. I also realized there already is io.quarkus.tls.TlsConfigurationRegistry#register which should make it possible to register TLS config programmatically. I am going to explore, write test and update docs right very soon.

Update: done, there is now a test and docs for programmatic TLS registry setup.

[sberyozkin]

Hi @michalvavrik

we want to provide a programmatic way to configure what users can do in a properties file because some users prefer it;

Sure, I totally support this idea

this allows to enable mTLS client authentication at runtime unlike the quarkus.http.ssl.client-auth build-time configuration property (and no, we can't simply move that property to runtime because it required code changes related as you can see)

Interesting, let me clarify here, do we still have a single instance of MTLS mechanism ? Or, given that the quarkus.http.ssl.client-auth is a build time property, is it possible that a user sets quarkus.http.ssl.client-auth=required and then also enables is with the API and we have 2 MTLS mechanism instances ? Sorry if the answer is in the code, I'd just like to clarify it first...

it allows flexibility in mapping certificate to SecurityIdentity roles, you documented that users can use the augmentor, but this API requires less code than definining the augmentor and is more straightforward, because it is in a one place.

Looks good indeed

I am quite sure you are wrong. Can you rephrase this so that I can understand what you mean, please? There can be something I am missing. Without this PR you need to set quarkus.http.ssl.client-auth to get the client auth. With this PR you don't need to set this property in your properties file You can also disable insecure requests in the properties file (which is done by default when the property is REQUIRED), but that is not mTLS.

Right, you need to set quarkus.http.ssl.client-auth but REQUEST also allows MTLS and the MTLS mechanism will be acttive. We even have it doc-ed at https://quarkus.io/guides/security-authentication-mechanisms

I am happy to follow-up with TLS registry.

I suppose what I'm trying to suggest is that we should think it out, we do not have to rush because the fluent API will be the most important Quarkus Security feature, so we have to think about all the variations. Adding one bit now, and then another one later might now work

[sberyozkin]

@michalvavrik Combining it with the TLS registry looks good

[michalvavrik]

Hello @cescoffier , this PR might still get some tiny changes (like docs tweaks if Sergey spots something), but I think the changes in extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerOptionsUtils.java and extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java are "final". Could you have a look and ACK that these changes are acceptable? If you would like to review this PR as well, great, any input is welcome!

[sberyozkin]

Hi @michalvavrik @cescoffier, the API is marked as experimental so we can easily update it in 3.25.CR1 or even later to support more cases, and tune it as required.
IMHO, a good start for supporting MTLS in the code only

[michalvavrik]

Right, for record, I made sure that this PR has no impact on cases when this fluent API is not used, so changes in HttpServerOptionsUtils and VertxHttpRecorder will not affect users unless they are experimenting with this API.

[cescoffier]

I will have a deeper look - there are a few refactoring in vert.x HTTP that need a bit more scrutiny (switch from build time config to runtime, moving a method to retrieve the tls config to some security classes...)

[michalvavrik]

I will have a deeper look - there are a few refactoring in vert.x HTTP that need a bit more scrutiny (switch from build time config to runtime, moving a method to retrieve the tls config to some security classes...)

It would be great if we can finalize this before you go to PTO, otherwise considering Sergey's PTO, it will be hard to finalize this for 3.26. Thanks @cescoffier

[cescoffier]

It's planned for tomorrow.

[cescoffier]

I had another look. My main concern is having the main HTTP utilities depending on security utilities.

[cescoffier]

I don't believe moving these to the HTTP security classes is correct. They are related to TLS and should be closed to the TLS handling.
Can these methods be moved here, or into a class that is TLS specific?

[michalvavrik]

It won't be without cost, because if I can't store programmatic TLS config on the security class, it means I'll have to create a new class that holds it (I mean that value that user configured), but it is not an issue for me. I'll move it.

[michalvavrik]

I have introduced HttpServerTlsConfig which now holds TLS client auth and config name for the HTTP server. It is not completely decoupled from HTTP Security configuration because if users can configure TLS client auth and config by configuring MTLS, then we still need to make sure that HTTP Security config is ready, so this class still initialize HTTP Security config if it wasn't initialized already (which can only happen in DEV mode).

Hope it's better.

[michalvavrik]

Side note, I could make it bean if you prefer that. I avoided adding HTTP Security config to CDI because Martin Kouba warned me there could be theoretical circular dependency, but this HTTP Server TLS config is independent. Anyway, I think it is fine as on reload it is cleaned by a shutdown context.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 7b09577.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 7b09577.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/micrometer-opentelemetry/deployment

✖ io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_Failed - History

• Stream has no elements - java.lang.IllegalArgumentException

java.lang.IllegalArgumentException: Stream has no elements
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReadingDataPoint(MetricDataFilter.java:236)
	at io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_Failed(MicrometerTimedInterceptorTest.java:100)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:534)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:448)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

[michalvavrik]

I think #49100 will bring merge conflicts, I'll resolve them once Clement is OK with this PR.

[sberyozkin]

@cescoffier Hi, how does it look to you now ? Note, we marked HttpSecurity Fluent API as experimental, so anything related to the Fluent API is changeable until it settles a bit and everyone is happy

[sberyozkin]

Great, thanks @cescoffier, I think this PR from @michalvavrik will make it more exciting for users to deal with (M)TLS, the HttpSecurity Fluent API is a work in progress, a good of number of incremental improvements yet to come, for CORS, CSRF, and the rest of it...