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

Added option to use custom SSLContext #673

Closed
wants to merge 1 commit into from
Closed

Added option to use custom SSLContext #673

wants to merge 1 commit into from

Conversation

Hakky54
Copy link

@Hakky54 Hakky54 commented Oct 19, 2023

Tomcat supports configuring a custom SSLContext when configuring the SSLHostConfigCertificate. However the custom SSLContext gets ignored during the server startup and ssl initialization. A new SSLContext will be created and it will fail if not all properties are provided. A fix would be to use the custom SSLContext if it is present or fallback to the original behaviour of still creating it.

I noticed this issue when I tried to configure the embedded tomcat server within Spring Boot. Although it is possible to configure it, it was just ignoring it which I found strange. I asked for help at stackoverflow here: https://stackoverflow.com/questions/77322685/configuring-ssl-programatically-of-a-spring-boot-server-with-tomcat-is-failing

However after debugging the source code I found out the custom ssl is being ignored. I made the code adjustment and created local artificats to test with my project and it worked.

My motivation to have this feature is to use tomcat with a custom ssl configuration with different kind of options for constructing a trustmanager/keymanager, such as hot reloadable option, existing truststore alongside jdk trusted certificates and OS trusted certificates etc, or a trustmanager which can just grow over time adding trusted certificates whenever needed to a running instance without restarting, encrypted private keys as pem files etc. Basically allowing all kind of advanced ssl options with the following library: https://github.com/Hakky54/sslcontext-kickstart

Spring boot 3.x.x is using tomcat 10.1.x however it might be possible to backport it to earlier versions such as 9.x wich is being used in Spring boot 2.x

@rmaucher
Copy link
Contributor

It doesn't work like that because it was not supposed to. The item that is/was supposed to be configured is the SSLImplementation, which then provides the SSLUtil which will create the SSLContext. Now allowing your hack is not very complicated, so maybe, but we're never going to give any guarantees that it will never change in the future ...

@Hakky54
Copy link
Author

Hakky54 commented Oct 20, 2023

I see indeed clearly that it was not the intention of the you/ other developers/maintainers to expose a setter method to set a custom sslcontext which is later on ignored. As it is clearly not a bug it must be that you guys didn't wanted to support a custom configuration like that.

Netty and Jetty support it, so I was always curious why Tomcat does not have something like this. It must be a good reason I think. So what is your overall opinion regarding this topic, what do you think of enabling users to allow to configure ssl configuration of their server programatically? And if positive what would be the downside of supporting something like this and if negative why would you not allow the user the configure it like that?

@Hakky54
Copy link
Author

Hakky54 commented Oct 24, 2023

I am not sure whether you would consider to have this option at all. Would you prefer me to close the issue?

@KoteswararaoGundapaneni

This comment was marked as spam.

@Hakky54
Copy link
Author

Hakky54 commented Feb 6, 2024

Hi team, is there any progress on this topic. Is there something what I can do on my side? Like backporting to other branches or anything else maybe?

@markt-asf
Copy link
Contributor

I was tempted to merge this but having reviewed the Tomcat code I think this is going to create problems - the main one being that Tomcat clears the SSLContext on Connector.stop() when bindOnInit is false. I think allowing this is going to create lifecycle management issues.

I'm not against a mechanism to allow an SSLContext to be provided directly rather than via the current SSLImplementation but it needs to be one that addresses the Lifecycle issues.

@markt-asf markt-asf closed this Feb 7, 2024
@Hakky54
Copy link
Author

Hakky54 commented Feb 7, 2024

Ah that is pitty, I was looking forward to it. You have a better overview of the issues which it can cause to other functionalities. I was not aware of the lifecycle management and only focused on injecting the sslcontext programmatically. If you consider to work on this topic in the future feel free to ping, I still hope it will be available. Thank you for reviewing it, have a nice day.

@markt-asf
Copy link
Contributor

I have some ideas on how to address this. I might have a fix for this soon that takes account of the lifecycle issues.

markt-asf added a commit that referenced this pull request Feb 7, 2024
Based on pull request #673 provided by Hakan Altındağ
#673
@markt-asf
Copy link
Contributor

OK, it is in main. I'll back-port as well.

markt-asf added a commit that referenced this pull request Feb 7, 2024
Based on pull request #673 provided by Hakan Altındağ
#673
@Hakky54
Copy link
Author

Hakky54 commented Feb 7, 2024

Thank you mate, I really appreciate this! Big kudos! 🥳
I am looking forward to the new release!

markt-asf added a commit that referenced this pull request Feb 7, 2024
Based on pull request #673 provided by Hakan Altındağ
#673
markt-asf added a commit that referenced this pull request Feb 7, 2024
Based on pull request #673 provided by Hakan Altındağ
#673
@Hakky54
Copy link
Author

Hakky54 commented Feb 20, 2024

Hi @markt-asf I just wanted to thank you. I noticed the latest version 10.1.19 is available since today. I tried it out and it works, amazing work!

@markt-asf
Copy link
Contributor

You're welcome. I didn't do much here. I just tweaked the original PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants