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

Reload of key/trustsore when re-establishing a connection #3074

Open
MichaelMorrisEst opened this issue Oct 10, 2024 · 6 comments
Open

Reload of key/trustsore when re-establishing a connection #3074

MichaelMorrisEst opened this issue Oct 10, 2024 · 6 comments
Labels
appenders Affects one or more Appender plugins configuration Affects the configuration system in a general way enhancement Additions or updates to features

Comments

@MichaelMorrisEst
Copy link
Contributor

#2767 introduces functionality to enable reloading key/trustore when the certs are renewed. However a manual step of triggering a reconfiguration (e.g. by touching the config file) is needed for the key/trust store to be reloaded. While this is a big improvement on having no reload, it is still not ideal to have to trigger a reconfiguration.

The cert renewal has no impact on existing established connections (as the handshake is done when the connection is established) so there is no need for the key/trust store to be reloaded for existing connections to continue working.
However, when an error occurs in writing to the socket a retry is attempted which includes the creation of a new socket and connection. Using a no longer valid cert here will prohibit the connection being re-established. If, during the retry, the key/truststore are reloaded, then the latest certs would always be used in re-establishing the connection and would effectively remove the need to trigger the reconfiguration.

Is this something the community would be open accepting a PR on? If so I can work on it and submit

@vy
Copy link
Member

vy commented Oct 10, 2024

If, during the retry, the key/truststore are reloaded, then the latest certs would always be used in re-establishing the connection and would effectively remove the need to trigger the reconfiguration.

@MichaelMorrisEst, this is something crossed my mind while working on #2767, but I am not sure about the implications of trying to reload key/trust store on every new socket creation attempt. [Thinking out loud here...] If the server has indeed gained a new identity, reloading key/trust stores will solve the problem swiftly. But what if the failure is due to something else? If we decide to implement this, shall this be the default, or opt-in via a new attribute?

Another concern I have regarding this approach is we will be introducing an exception to the reconfiguration logic. That is, when a Log4j configuration is loaded, it is treated more or less immutable – obviously, except LoggerConfigs. Now we will introduce a "please reload only this little configuration element" exception. I am not saying this is something bad per se. But exceptions generally bite us in the long run.

@ppkarwasz, any thoughts?

@vy vy added enhancement Additions or updates to features appenders Affects one or more Appender plugins configuration Affects the configuration system in a general way labels Oct 10, 2024
@MichaelMorrisEst
Copy link
Contributor Author

Hi @vy, thanks for the feedback

@MichaelMorrisEst, this is something crossed my mind while working on #2767, but I am not sure about the implications of trying to reload key/trust store on every new socket creation attempt. [Thinking out loud here...] If the server has indeed gained a new identity, reloading key/trust stores will solve the problem swiftly. But what if the failure is due to something else? If we decide to implement this, shall this be the default, or opt-in via a new attribute?

If reloading at every re-connection is a concern then a slightly different approach could be to do the re-connection as today (i.e. don't reload the stores) but catch SSLHandshakeException and, when encountered then do the reload and try again. This would limit the reloading to only the relevant scenario of a handshake problem.
I have no strong opinion on whether it should be default behaviour or opt in, and am happy to go with whichever the community prefers (if there is agreement on proceeding)

Another concern I have regarding this approach is we will be introducing an exception to the reconfiguration logic. That is, when a Log4j configuration is loaded, it is treated more or less immutable – obviously, except LoggerConfigs. Now we will introduce a "please reload only this little configuration element" exception. I am not saying this is something bad per se. But exceptions generally bite us in the long run.

The key/truststore are already not handled the same as other configuration elements though (as changes to them do not trigger a reconfig). As they are excluded from the normal reconfiguration logic, then the only way to support changes to them is through something other than the normal reconfiguration logic.

@MichaelMorrisEst
Copy link
Contributor Author

Hi @vy, @ppkarwasz
Just a gentle reminder on this issue when you get a chance to consider, thanks

@ppkarwasz
Copy link
Contributor

@MichaelMorrisEst,

Thanks for the reminder, this issue was buried deep down on my TODO list.

The cert renewal has no impact on existing established connections (as the handshake is done when the connection is established) so there is no need for the key/trust store to be reloaded for existing connections to continue working.
However, when an error occurs in writing to the socket a retry is attempted which includes the creation of a new socket and connection. Using a no longer valid cert here will prohibit the connection being re-established. If, during the retry, the key/truststore are reloaded, then the latest certs would always be used in re-establishing the connection and would effectively remove the need to trigger the reconfiguration.

Could you explain more your use case? I looked at Tomcat's SSL implementation (SSLUtilBase) and some HTTP client libraries for Java and I couldn't find an example of the opportunistic keystore reloading behavior you are proposing.
How do you deal with modification of the key/trust store in other Java components?

Certificate expiration events are usually very rare, so I would assume that whenever such an event occurs:

  1. The keystore and truststore are modified.
  2. All the applications that use them are notified. IIRC UNIX for servers like NGINX you just need to send a SIGUSR signal. Since Java applications do not support that, they need to be restarted.

Spring Boot did introduce an SSL reloading mechanim in 3.1.0. Maybe your application needs to do the same?

@MichaelMorrisEst
Copy link
Contributor Author

Hi @ppkarwasz
The use case is that when a certificate expires and is replaced that the SocketAppender can continue without the need for manual intervention (currently need to touch the config file in order to reload the cert or restart).
The spring boot implementation is exactly the type of functionality we are looking for. We already use this for SSL communication in our code in our spring boot based applications and implement a similar mechanism in our application code where we are not using spring boot. Our application code has no access to the SSL communication layer in the SocketAppender though so it does not help there. For the SocketAppender to follow a similar pattern, the key/truststore would be reloaded when they change (for example as part of the config monitoring). I would see this as the ideal solution (and would be happy to implement), however if that is not agreeable then I am suggesting this reloading when a handshake error occurs as an alternative way to solve the problem.

@ppkarwasz
Copy link
Contributor

Hi @MichaelMorrisEst ,

The spring boot implementation is exactly the type of functionality we are looking for. We already use this for SSL communication in our code in our spring boot based applications and implement a similar mechanism in our application code where we are not using spring boot.

Thanks for the details.
I should probably note that I am not strongly opposed to allowing on-the-fly updates to the "SSL subsystem" of Log4j Core, I am a little bit concerned about the complexity of the task.
Theoretically support for SSLContext rotation seems like a great feature, but the devil is in the details.
We already have some mutable parts in Log4j Core:

All these systems (of course) use an ad-hoc or partially shared update mechanism. I think what we need is:

  • a minimal system to send events to a LoggerContext, like Spring events. You could use that system to inform Log4j Core that the SSL configuration has changed.
  • a way to configure which event to send, when a particular file has changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appenders Affects one or more Appender plugins configuration Affects the configuration system in a general way enhancement Additions or updates to features
Projects
None yet
Development

No branches or pull requests

3 participants