Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quarkus.tls.trust-all does not seem to be a build time property #23680

Closed
manofthepeace opened this issue Feb 14, 2022 · 13 comments · Fixed by #26802
Closed

quarkus.tls.trust-all does not seem to be a build time property #23680

manofthepeace opened this issue Feb 14, 2022 · 13 comments · Fixed by #26802
Assignees
Labels
Milestone

Comments

@manofthepeace
Copy link
Contributor

Describe the bug

I was trying to do a https call using a self signed cert on my laptop and as expected, I got the Failed to create SSL connection error. This is without setting quarkus.tls.trust-all so it is false by default.

Then I just tried running the app the following way;
QUARKUS_TLS_TRUST_ALL=true java -jar target/quarkus-app/quarkus-run.jar

I got the following warning;

2022-02-14 11:47:06,535 WARN  [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:
 - quarkus.tls.trust-all was 'false' at build time and is now 'true'

But it still worked, in the end the tls call worked so the property ended up true, and working. So it proves the property does work runtime and not only build time. I did test native as well with the same result.

Expected behavior

I expected not being able to change that property at runtime

Actual behavior

Property can be changed at runtime, and is working.

How to Reproduce?

Reproducer: N/A

Steps to reproduce:

  1. Make a https call to a endpoint using a self signed cert and expect a ssl failure
  2. set quarkus.tls.trust-all to true at runtime and expect previous call to work.

Output of uname -a or ver

Darwin Kernel Version 20.6.0

Output of java -version

openjdk version "17.0.2" 2022-01-18

GraalVM version (if different from Java)

21.3.1

Quarkus version or git rev

2.7.1

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.4

Additional information

No response

@manofthepeace manofthepeace added the kind/bug Something isn't working label Feb 14, 2022
@sberyozkin
Copy link
Member

CC @radcortez

@radcortez
Copy link
Member

@manofthepeace I could use some more information.

I did find a couple of usages where the config is being read directly. Can you confirm that this was using the REST Client? A reproducer is also welcomed. Thanks!

@radcortez radcortez added the triage/needs-reproducer We are waiting for a reproducer. label Feb 22, 2022
@manofthepeace
Copy link
Contributor Author

Hi @radcortez sorry indeed the information in the OP was limited. I was referring to the rest-client-reactive.

Just to be clear. I was happy that it did work, since we did not have to rebuild for somebody to test with a self signed cert, nor we did add to provide a build with that insecure settings hardcoded to true. Would probably be nice to have different config that are runtime for the places where it can work.

Added a reproducer here

To reproduce;

  1. mvn quarkus:dev (the trust_all will use the default, false)
  2. curl localhost:8080/test (this should fail with the ssl exception)
  3. mvn quarkus:dev -Dquarkus.tls.trust-all=true
  4. curl localhost:8080/test (this should work)

For some reason I dont understand why in the reproducer I do not see the following;

2022-02-14 11:47:06,535 WARN  [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:
 - quarkus.tls.trust-all was 'false' at build time and is now 'true'

and I cannot make it work native.. But I can say it does work with our real app which does not call itself like the reproducer.

@radcortez
Copy link
Member

In #18777, the configuration moved to be loaded dynamically. @michalszynkiewicz any thoughts?

@michalszynkiewicz
Copy link
Member

Are there any difficulties in having it a runtime property?
For the rest client it IMO makes more sense. You can take your binary, test it in with trust-all enabled and then go to prod where it's disabled...

@radcortez
Copy link
Member

radcortez commented Feb 28, 2022

Not at all.

Currently, this is inconsistent in REST Clients and the Config documentation, plus the other extensions that use the fixed config at runtime (Keycloack, Kubernetes, Mail Client, OIDC, and Spring Cloud Config). Are we ok to change the configuration behavior for these extensions?

@michalszynkiewicz
Copy link
Member

I don't know... @sberyozkin @cescoffier @geoand ^

@cescoffier
Copy link
Member

This is why we are improving TLS config. That work was out on standby due to the duplicated context work.

See #17038

@radcortez radcortez removed the triage/needs-reproducer We are waiting for a reproducer. label Mar 1, 2022
@radcortez
Copy link
Member

Ok, lets do the work as part of #17038

@cescoffier
Copy link
Member

BTW, ALL TLS related attributes should be runtime and not build-time. There are some nasty details here (often named security) that need runtime adjustment.

@cescoffier
Copy link
Member

Ok, lets do the work as part of #17038

Famous last word since almost a year 🤣

@glefloch
Copy link
Member

glefloch commented Apr 4, 2022

Does it still make sense to set quarkus.tls.trust-all a runtime property ?

@cescoffier
Copy link
Member

trust-all and any TLS related properties should be runtime properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment