Support reloading gRPC server SSL bundles#50248
Conversation
Add a `reloadOnUpdate` property to `GrpcServerProperties.Ssl` and introduce `ReloadableKeyManagerFactory` to support dynamically updating the key manager when the associated `SslBundle` changes. Signed-off-by: Ujjawal Sharma <ujjawal98kaushik@gmail.com>
ukasus
changed the title
spring-projects-issues
added
the
status: waiting-for-triage
|
Thanks very much for the PR @ukasus. I need some time to consider this approach as I'm still quite concerned that trying to make the I'd rather we took an approach similar to We realistically won't get to this until Spring Boot 4.2 now, so I'll leave the PR open whilst we consider things. |
prashantpiyush1111
left a comment
prashantpiyush1111
left a comment
There was a problem hiding this comment.
Reviewed the changes. A few inline comments added around thread-safety in setSslBundle and setDelegate — mainly suggesting synchronized to guard against concurrent reload events causing silent overwrites.
Also flagged early validation concern in ServerCredentials.java when reloadOnUpdate=true but no X509ExtendedKeyManager is present, and a config metadata suggestion for reloadOnUpdate flag in GrpcServerProperties.
Regarding @philwebb's concern — ReloadableX509ExtendedKeyManager is purely a delegating wrapper and the swap is scoped only to gRPC's TLS handshake path, so blast radius seems limited and shouldn't affect other SSL consumers. Solid foundation for 4.2.
|
|
||
| static class ReloadableKeyManagerFactorySpi extends KeyManagerFactorySpi { | ||
|
|
||
| private volatile @Nullable ReloadableX509ExtendedKeyManager keyManager; |
There was a problem hiding this comment.
volatile ensures visibility but not atomicity. If two setSslBundle calls fire concurrently, both can read null and create separate instances — second will silently overwrite first. Suggest making setSslBundle synchronized.
|
|
||
| static class ReloadableX509ExtendedKeyManager extends X509ExtendedKeyManager { | ||
|
|
||
| private volatile X509ExtendedKeyManager delegate; |
There was a problem hiding this comment.
volatile here is correct for visibility of the reference swap. However setDelegate has no synchronization — concurrent reloads could cause a torn update mid-handshake. Consider synchronized on setDelegate.
| return managers.getKeyManagerFactory(); | ||
| } | ||
| return ReloadableKeyManagerFactory.create(bundles, bundleName, bundle); | ||
| } |
There was a problem hiding this comment.
Clean conditional wiring. One question — if reloadOnUpdate is true but the bundle has no X509ExtendedKeyManager, it throws IllegalStateException at startup. Should this be validated earlier with a clearer error message in GrpcServerProperties?
| /** | ||
| * Whether to reload the key manager when the SSL bundle is updated. | ||
| */ | ||
| private boolean reloadOnUpdate; |
There was a problem hiding this comment.
Default is false which is safe and backward compatible. Should this have a @NestedConfigurationProperty or config metadata entry so IDEs can hint users about this flag?
4 participants
Add reloadable KeyManagerFactory for dynamic TLS bundle updates-
to support hot-reloading of X509ExtendedKeyManager on SslBundle updates
without server restart
for thread-safe runtime updates
reloadOnUpdateflag to toggle behavior(static vs reloadable based on config)
Enables zero-downtime TLS certificate/key rotation for gRPC servers.
Signed-off-by: Ujjawal Sharma ujjawal98kaushik@gmail.com
[resolves #49833]