-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Simplify usage of custom ssl configuration #706
base: main
Are you sure you want to change the base?
Simplify usage of custom ssl configuration #706
Conversation
…e as an easy way to create a custom tomcat sslcontext
Hi @markt-asf What do you think of this PR, would it make sense to have this kind of wrapper, or does it needs to be adjusted or would you like me to close it and disregard it? Looking forward to get your feedback on this |
Your previous PR, which was integrated, unfortunately caused a very high number of regressions that had to be fixed over multiple Tomcat releases. This PR similarly seems uninteresting to me, so as a result I will not integrate it. |
Ah so your feeling is that this also might cause some regression while this wrapper does not add that much value to the project itself. I can understand that. Okay, thank you for your time for reviewing this PR. Let's close it then. |
I had left the PR open since others could have been willing to go through with it (or not, I don't know). |
Hi guys, any news regarding this topic? |
This wrapper class appears to specifically discard the key managers and trust managers. The constructor captures its arguments and then ignores them forever after that. Why bother capturing them in the first place? If the goal is to allow "instant" reloading of the SSL configuration... that capability already exists in Tomcat. |
I used my own example from my own project to demonstrate the usage of it within the PR description, however the code changes are slightly different. It has a constructor with a SSLContext, KeyManager and TrustManager.
Well actually it is using the KeyManager and TrustManager, see here: https://github.com/apache/tomcat/pull/706/files#diff-8ed2a43a8b2f354b707c0fdb8cd5b794e5a476ecbf603b2ba69af5eea18b3cc4R73-R81 So it is using all of the objects from the constructor even the SSLContext itself. It just acts as a wrapper to simplifies the usage of a custom
In my pull request description I mentioned the usage of ssl reloading, but this was an example to demonstrate how I used it with the resulting wrapper which I needed to add to provide a custom ssl configuration. So I wanted to point out there that the developer who want's to have a custom ssl configurationn always needs to create a wrapper on their side or else it won't work. So the reloading of tomcat was just an example but I use it also for different use cases, such as:
It might be that I am the only developer which is working in this kind of edge cases... So all of these scenario's are working for me already actually while using the wrapper class, I thought it would be nice to move it to apache tomcat to simplify the usage of it. |
Oops, I seem to have totally missed that part of the code. My apologies.
Really, using any custom SSLContext for whatever reason is a valid use case. It's not reasonable for Tomcat to provide all of these various combinations of features, so extensibility is certainly useful. I think the only question is whether this wrapper is really useful to ship with Tomcat. It's certainly not useful outside of Tomcat since it uses Tomcat's internal interface. But it does bridge the gap between Java-provided APIs and Tomcat's APIs. |
The |
This PR is a followup of the following earlier PR #673 Although that pull request didn't get merged, the code changes has been comitted to the main branch by the main developer, see here for the specific commit: b0df981
Context
With the earlier commit it is now possible to programatically configure the ssl configuration of tomcat instead of using properties and delegating to tomcat to construct the ssl configuration. This opens the possibility of reloading the ssl configuration or other customizations as shown also here: sslcontext-kickstart
I want to provide an example with my own code to give a better understanding, the project is also available here: Instant SSL Reloading with Tomcat
SSLContext wrapper
Problem statement
Boilerplate code is needed by the end-user to provide a custom ssl configuration. Tomcat takes a custom SSLContext, the full name is
org.apache.tomcat.util.net.SSLContext
while the end-user hasjavax.net.ssl.SSLContext
. So the end-user is required to create an implementation oforg.apache.tomcat.util.net.SSLContext
which acts as a wrapper. This sslcontext needs to be passed toSSLHostConfigCertificate
to further configure the server.Solution
Provide a helper class which acts as a wrapper to reduce the boilerplate code. The utility interface is able to provide a method to wrap the required objects, in this case
javax.net.ssl.SSLContext
, KeyManager, TrustManager in aorg.apache.tomcat.util.net.SSLContext
What do you think, would it be a nice addition to the code changes we did previously? It will atleast make it more developer friendlier. Looking forward to your feedback.