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

Remove warning on build time truststore property #22492

Merged
merged 1 commit into from
Dec 23, 2021
Merged

Remove warning on build time truststore property #22492

merged 1 commit into from
Dec 23, 2021

Conversation

vsevel
Copy link
Contributor

@vsevel vsevel commented Dec 23, 2021

GraalVM supports now truststore configured at runtime. The old warning does not make sense anymore.

@vsevel
Copy link
Contributor Author

vsevel commented Dec 23, 2021

/cc @geoand

@geoand
Copy link
Contributor

geoand commented Dec 23, 2021

Thanks for this!

Let's get an ack from @zakkak first :)

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Support for TrustStore configuration at run time was added in oracle/graal#3652 and is present in GraalVM/Mandrel 21.3

Thank you @vsevel !

@vsevel
Copy link
Contributor Author

vsevel commented Dec 23, 2021

LGTM!

Support for TrustStore configuration at run time was added in oracle/graal#3652 and is present in GraalVM/Mandrel 21.3

Thank you @vsevel !

yes. that was @matthyx actually doing all the work on our side to do the lobbying in the graalvm project. ;)
he just tested that the runtime truststore was working in native with 2.6 and micro image. very happy about this.

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 23, 2021
@vsevel
Copy link
Contributor Author

vsevel commented Dec 23, 2021

we would need to modify doc in https://quarkus.io/guides/native-and-ssl as well

@vsevel
Copy link
Contributor Author

vsevel commented Dec 23, 2021

I have made a doc change proposition. I will let you review it.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple of minor comments. Thanks for taking care of this.

I'm also wondering if we should adjust the conclusion of the guide (the one saying it needs more thinking). Not sure it's still valid.

* When a certificate expires, the application needs to be rebuilt, and redeployed into production with the new certificate, which is an inconvenience.
* Even worse, if a certificate gets revoked because of a security breach, all applications that embed this certificate need to be rebuilt and redeployed in a timely manner.
* Providing all certificates at build time complicates the CI, specifically in dynamic environments such as Kubernetes where valid certificates are provided by the platform in the `/var/run/secrets/kubernetes.io/serviceaccount/ca.crt` PEM file.
* Lastly, this requires to add into the application all certificates for all environments (e.g. DEV, TEST, PROD), which means that a certificate that is required for DEV but should not be used elsewhere, will make its way anyway in production.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true? If you rebuild the application for each environment, it's not a problem right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you do rebuild the app in each env, I agree it is not a problem.
but we don't, and most work environments in which I worked don't either: software is built once and deployed in each env unchanged. otherwise what you deploy in production has never run anywhere. configuration changes, but the binary does not. at least that is what I have been used to. but I am sure this is a topic where people can have split positions.
what I can do is something like: unless the application gets a dedicated built for each environment or in case the decision is to run the exact same build in all environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/src/main/asciidoc/native-and-ssl.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/native-and-ssl.adoc Outdated Show resolved Hide resolved
@vsevel
Copy link
Contributor Author

vsevel commented Dec 23, 2021

I have taken into account the different comments. I changed also the conclusion (any suggestion welcome).

@gsmet gsmet merged commit 41876c0 into quarkusio:main Dec 23, 2021
@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Dec 23, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 23, 2021
@gsmet
Copy link
Member

gsmet commented Dec 23, 2021

Thanks!

@vsevel vsevel deleted the warn_build_truststore branch December 23, 2021 20:07
@gsmet gsmet modified the milestones: 2.7 - main, 2.6.1.Final Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants