Add global quarkus.tls.trust-all configuration property

[glefloch]

This branch adds a global config property quarkus.tls.trust-all. Thus, extensions which needs a property to enable/disable tls verification could use it.

There are some failed tests.
Here is the problem:
The quarkus.tls.trust-all property has a default value to false.
When configuring an other property (quarkus.vault.tls.skip-verify) as:

@ConfigItem(defaultValue = "${quarkus.tls.trust-all}")

This will end with an error resolving the property. But when setting quarkus.tls.trust-all property in application.properties, the resolution will be ok.
I think using Optional could fix it but it means handling default value in each extension.

close #8975
close #12266

[gsmet]

I think the easiest way to fix this issue is to not use it as a default value and inject the config and deal with it in the code.

@dmlloyd do you see any other way? I.e. would we be able to support default values in expansion? That's probably going to be complicated, right?

[dmlloyd]

We do support default values in expansion. If it is not working in some case, it is a bug. This should be covered in the "test extension" test.

[glefloch]

Thanks @dmlloyd, I'm going to check there how it is used.

[sberyozkin]

I'm not sure it should be deprecated. There has to be an option for each extension to choose to differ from the global value. The example I suggested during the forum discussion: Vault is internal, OIDC server is not.
So I'd rename quarkus.oidc.tls.verification to quarkus.oidc.tls.trust-all (initialized by default to quarkus.tls.trust-all as per your PR) without the deprecated tag (and similarly for the other extensions). We can then update the migration guide. And docs to recommend quarkus.tls.trust-all but the users would still have an option to override it with quarkus.oidc.tls.trust-all for OIDC or quarkus.vault.tls.trust-all for Vault, etc...

[glefloch]

After running some test, default value in expansion works when all properties are defined in the same class.
If the property used in expansion is part of a different class, then resolution failed.
@dmlloyd do you think this is a bug or a limitation?
I will follow @gsmet idea to handle the property directly in the code if this an expected behavior.

[dmlloyd]

After running some test, default value in expansion works when all properties are defined in the same class.
If the property used in expansion is part of a different class, then resolution failed.
@dmlloyd do you think this is a bug or a limitation?

Definitely a bug! The default values for properties are registered globally so they should always be available. There is one limitation: a build time property cannot default to an expression that expands to a run time property. However a run time property can default to an expression referencing a build time property.

[glefloch]

They are all runtime in my case.
Could you give any pointers to get started?

I will open an issue.

[dmlloyd]

They are all runtime in my case.
Could you give any pointers to get started?

The code which generates the class that resolves the configuration default values at run time is at line 934 of RunTimeConfigurationGenerator. It would be very strange indeed if crossing class boundaries somehow triggered a problem here though. Perhaps it's related to the recent move of property expansion from Quarkus to SmallRye Config though.

I will open an issue.

👍

[glefloch]

My bad, property expansion on default values works as expected, I haven't been able to create a reproducer. My problem seems to be related to the vault extension. I'm working on it.

[sberyozkin]

Hi All, by the way, should we also migrate all the existing quarkus.ssl.* properties to quarkus.tls.* ?
Graalvm seems to use TLS term too, example,
https://github.com/graalvm/graaljs/blob/master/graal-nodejs/doc/api/tls.md
oracle/graal#1336

It is not a big deal, but I guess the trust-all should be in the same namespace as other tls/ssl properties

[glefloch]

@sberyozkin I think we can put tls/ssl properties into the same namespace.
Regarding ssl or tls, I have no personal opinion. Maybe @maxandersen has one?

[gsmet]

I wonder if a better approach would be to produce a build item that would override the local configuration.

I'm not entirely sure having a local configuration is a bad thing because you might want to disable certificates for a specific area while still checking the certificates elsewhere.

Thus having trustAll undefined would make the local config authoritative and setting it to true would disable certificate checking everywhere.

[glefloch]

@gsmet I tried to create a TrustAllBuildItem which is optional depending on the configuration. But I don't know how to deal with it in the different extension.
For example, in the mailer extension, the configuration is directly used in the

quarkus/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MailClientProducer.java

Line 19 in 23c970a

public class MailClientProducer {

right here:

quarkus/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MailClientProducer.java

Line 109 in 23c970a

cfg.setTrustAll(config.trustAll);

Could you help me to move forward?

[gsmet]

@glefloch you would need to create a bean and inject it in the producer constructor. We have a few beans of this type called *Support. They are basically used to pass parameters.

[gastaldi]

Build failed:

2020-10-02T16:00:40.8750120Z [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1-jboss-1:compile (default-compile) on project quarkus-keycloak-authorization: Compilation failure
2020-10-02T16:00:40.8752609Z [ERROR] /home/runner/work/quarkus/quarkus/extensions/keycloak-authorization/runtime/src/main/java/io/quarkus/keycloak/pep/runtime/KeycloakPolicyEnforcerAuthorizer.java:[21,44] cannot find symbol
2020-10-02T16:00:40.8754227Z [ERROR]   symbol:   class Verification
2020-10-02T16:00:40.8755006Z [ERROR]   location: class io.quarkus.oidc.OidcTenantConfig.Tls
2020-10-02T16:00:40.8755934Z [ERROR] -> [Help 1]
2020-10-02T16:00:40.8758426Z org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1-jboss-1:compile (default-compile) on project quarkus-keycloak-authorization: Compilation failure
2020-10-02T16:00:40.8761656Z /home/runner/work/quarkus/quarkus/extensions/keycloak-authorization/runtime/src/main/java/io/quarkus/keycloak/pep/runtime/KeycloakPolicyEnforcerAuthorizer.java:[21,44] cannot find symbol
2020-10-02T16:00:40.8762943Z   symbol:   class Verification
2020-10-02T16:00:40.8763687Z   location: class io.quarkus.oidc.OidcTenantConfig.Tls
2020-10-02T16:00:40.8764269Z 

[glefloch]

I spent a bit of time on this and pushed a new version of the code.
I tried to create a buildItem as suggested but from my understanding, depending on the extension, the trust-all property is used differently, for example:

• the mailer extension uses CDI beans to build the client, creating a CDI Support bean works. But this is the only extension which uses a CDI bean for that.
• Other extensions are mainly using recorder.

Instead of creating a builditem and a CDI bean, I only kept the TlsConfig class, and I handled the configuration value in each extension instead of setting quarkus.tls.trust-all as default value in each extension configuration.

WDYT @gsmet, @gastaldi ?

[gastaldi]

Don't forget the REST Client extension. This may be related #12970

[gastaldi]

LGTM

[glefloch]

@sberyozkin would you be able to have a look at this one?

[sberyozkin]

@glefloch Sure, very sorry for a delay...

[sberyozkin]

@glefloch I can take care of rebasing OidcRecorder :-), lets finalize/agree on the priority issue

[glefloch]

@sberyozkin I just rebased master and took care of it.

Regarding the priority, actually, quarkus.tls.trust-all is false by default. Replacing the || by && means that you have to set both quarkus.tls.trust-all and the extension specific property to disable verification. I keep thinking that extension should have the priority but I don't know how I could handle it in the code as by default each extensions don't trust everything.

[sberyozkin]

@glefloch Thanks, I did not realize quarkus.tls.trust-all is false (and indeed in the extensions) by default.

[glefloch]

@sberyozkin I updated the code so that extension properties have a higher priority compared to quarkus.tls.trust-all.

[sberyozkin]

Hi @glefloch Thanks, it looks good IMHO as now we'd be able to use your single property plus as suggested somewhere earlier, if we have say Vault and Keycloak with different TLS trust requirements compared to the rest of the application, then we have it covered :-)

[sberyozkin]

@glefloch One more thing I'd like ask, maybe it can be done in a follow up PR, should we deprecate for OIDC (and other extensions) the extension specific properties, unless they are called trust-all ?
So that we can have

quarkus.tls.trust-all = false
# instead of quarkus.oidc.tls.verification = none
quarkus.oidc.tls.trust-all = true

If you agree then I guess it would indeed be better to do it in a follow up PR and merge this PR first...
thanks

[glefloch]

@sberyozkin yes I think that could be easier for users to have a same semantic for config property. Once this one is merged, I will do that.
There is a failing test in oidc, I tried to fix it but I don't understand the issue. Do you have an idea?

[sberyozkin]

Hi @glefloch Thanks, right now it is failing in the quarkus-arc deployment, can you please rebase again ?

[glefloch]

Yes I will do that. The previous error was a NPE because I was missing the default value for the Optional

[glefloch]

@sberyozkin it looks good with the rebase 🎉

[sberyozkin]

@glefloch Thanks a million for keeping this PR alive and completed :-), please consider following up with renaming the extension specific properties