logging-log4j2 icon indicating copy to clipboard operation
logging-log4j2 copied to clipboard

Reload of key/trustsore when re-establishing a connection

Open MichaelMorrisEst opened this issue 1 year ago • 2 comments

https://github.com/apache/logging-log4j2/pull/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

MichaelMorrisEst avatar Oct 10 '24 09:10 MichaelMorrisEst

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 avatar Oct 10 '24 09:10 vy

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 avatar Oct 10 '24 11:10 MichaelMorrisEst

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

MichaelMorrisEst avatar Oct 23 '24 10:10 MichaelMorrisEst

@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?

ppkarwasz avatar Oct 23 '24 11:10 ppkarwasz

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.

MichaelMorrisEst avatar Oct 30 '24 09:10 MichaelMorrisEst

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:

  • The Configuration itself can be replaced and I believe that this is a mechanism that works well. There are minor hiccups (like #3043), but otherwise it is all sunshine and rainbows.
  • We allow users to modify log levels on the fly and we even provide an API to modify levels.
  • We have a RollingFile appender that regularly performs file rollovers. This is an endless source of bugs.
  • We have a recently added MutableContextMapFilter that updates its configuration on-the-fly. The mechanism is probably not bug-free.

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.

ppkarwasz avatar Oct 30 '24 12:10 ppkarwasz

Hi @ppkarwasz Only getting back to look at this now. I've created a PR to see if I'm on the right track as regards what you are proposing above for an event sending mechanism. I includes a minimal sending/receiving mechanism and an example of using it for key/trust store changes. Please let me know what you think when you get time. Thanks

MichaelMorrisEst avatar Jan 10 '25 14:01 MichaelMorrisEst

Hi @ppkarwasz Just a gentle reminder if you get time to look at this, thanks

MichaelMorrisEst avatar Feb 07 '25 11:02 MichaelMorrisEst

@MichaelMorrisEst,

We have a rather busy moment at work, but I'll try to look at it in the weekend. cc/ @vy

ppkarwasz avatar Feb 07 '25 12:02 ppkarwasz

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:

The keystore and truststore are modified. 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.

Hi @ppkarwasz

I can provide an example with Tomcat. As you might know with Tomcat it is possible to programatically configure the ssl configuration and also reload it. I have written a working example here: https://github.com/Hakky54/java-tutorials/tree/main/instant-ssl-reloading-with-spring-tomcat It uses my own library for taking care of reloading the ssl configuration, see here https://github.com/Hakky54/sslcontext-kickstart?tab=readme-ov-file#support-for-reloading-ssl-at-runtime

I hope it will be useful for this task

Hakky54 avatar Feb 09 '25 07:02 Hakky54

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).

A different angle: Accept a list of ~files~ URI sources to monitor in addition to the configuration file itself?

<Configuration monitorInterval="60">
    <MonitorUri>file:///path/to/cert</MonitorUri>
</Configuration>

vy avatar Feb 10 '25 21:02 vy

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).

A different angle: Accept a list of ~files~ URI sources to monitor in addition to the configuration file itself?

file:///path/to/cert

Yes, that could work well. The URIs could be read in XmlConfiguration/JsonConfiguration and passed to AbstractConfiguration.initializeWatchers(...) in the same way as the monitorInterval is. The URIs could then be passed to WatchManager.watch(...) in addition to the config file, and the WatchManager.isModified() method returning true if the config file has changed or if any of the files given in the URIs have changed, thus triggering a reconfig.

This could be a very tidy way to implement it, extending the existing WatchManager rather than implementing something new. @ppkarwasz what do yo think?

MichaelMorrisEst avatar Feb 19 '25 16:02 MichaelMorrisEst

This could be a very tidy way to implement it, extending the existing WatchManager rather than implementing something new. @ppkarwasz what do yo think?

It's a very nice idea. Tomcat has a similar feature with WatchedResource and it works quite well.

ppkarwasz avatar Feb 19 '25 19:02 ppkarwasz