feat(server): Add a KeyStoreScanner#134
Open
ShahimSharafudeen wants to merge 1 commit intoprestodb:masterfrom
Open
feat(server): Add a KeyStoreScanner#134ShahimSharafudeen wants to merge 1 commit intoprestodb:masterfrom
ShahimSharafudeen wants to merge 1 commit intoprestodb:masterfrom
Conversation
|
|
Reviewer's GuideIntroduce a KeyStoreScanner to periodically monitor and reload the TLS keystore on file changes, make the scan interval configurable via HttpServerConfig, wire the scanner into the HttpServer lifecycle, and add integration tests verifying reload behavior. Sequence diagram for TLS keystore hot reload processsequenceDiagram
participant "KeyStoreScanner"
participant "SSLContextFactory"
participant "HttpServer"
loop Every keystoreScanInterval seconds
"KeyStoreScanner"->>"SSLContextFactory": Check for keystore file changes
alt If keystore changed
"KeyStoreScanner"->>"SSLContextFactory": Reload keystore
end
end
"HttpServer"->>"KeyStoreScanner": Configure scan interval and add bean
Entity relationship diagram for keystore scan interval configurationerDiagram
HTTP_SERVER_CONFIG {
int keystoreScanInterval
}
KEYSTORE_SCANNER {
int scanInterval
}
HTTP_SERVER_CONFIG ||--o| KEYSTORE_SCANNER : configures
KEYSTORE_SCANNER ||--|| SSL_CONTEXT_FACTORY : monitors
Class diagram for KeyStoreScanner integration and HttpServerConfig changesclassDiagram
class HttpServerConfig {
-int keystoreScanInterval = 1
+setKeystoreScanInterval(int)
+getKeystoreScanInterval()
}
class KeyStoreScanner {
+setScanInterval(int)
<<new>>
}
class HttpServer {
+HttpServer(...)
...
}
class SSLContextFactory
HttpServerConfig --> KeyStoreScanner : configures
HttpServer --> KeyStoreScanner : adds as bean
KeyStoreScanner --> SSLContextFactory : monitors
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In testKeyStoreReloadOnKeystoreFileChange, replace Thread.sleep with a polling loop or awaitility to avoid timing flakiness.
- Consider allowing a scan interval of zero or less to disable keystore scanning rather than always starting the scanner when a keystore is configured.
- The comment in HttpServer.java mentioning future configurability is now outdated—please remove or update it to reflect that the scan interval is already configurable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In testKeyStoreReloadOnKeystoreFileChange, replace Thread.sleep with a polling loop or awaitility to avoid timing flakiness.
- Consider allowing a scan interval of zero or less to disable keystore scanning rather than always starting the scanner when a keystore is configured.
- The comment in HttpServer.java mentioning future configurability is now outdated—please remove or update it to reflect that the scan interval is already configurable.
## Individual Comments
### Comment 1
<location> `http-server/src/main/java/com/facebook/airlift/http/server/HttpServerConfig.java:829-835` </location>
<code_context>
+
+ @Config("http-server.https.keystore.scan-interval")
+ @ConfigDescription("Interval in seconds at which the server checks for updates to the HTTPS keystore file.")
+ public HttpServerConfig setKeystoreScanInterval(int keystoreScanInterval)
+ {
+ this.keystoreScanInterval = keystoreScanInterval;
+ return this;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Validate keystoreScanInterval input to prevent misconfiguration.
Add validation to ensure the scan interval is a positive value within a reasonable range to avoid misconfiguration.
```suggestion
@Config("http-server.https.keystore.scan-interval")
@ConfigDescription("Interval in seconds at which the server checks for updates to the HTTPS keystore file.")
public HttpServerConfig setKeystoreScanInterval(int keystoreScanInterval)
{
if (keystoreScanInterval < 1 || keystoreScanInterval > 3600) {
throw new IllegalArgumentException("keystoreScanInterval must be between 1 and 3600 seconds");
}
this.keystoreScanInterval = keystoreScanInterval;
return this;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aaneja
suggested changes
Nov 17, 2025
| if (config.getKeyManagerPassword() != null) { | ||
| sslContextFactory.setKeyManagerPassword(config.getKeyManagerPassword()); | ||
| } | ||
| // Scan for keystore file changes every 1s (the default). Interval can be made configurable in the future |
There was a problem hiding this comment.
Comment can be removed. Default scan time will be config.getKeystoreScanInterval()
| } | ||
|
|
||
| @Config("http-server.https.keystore.scan-interval") | ||
| @ConfigDescription("Interval in seconds at which the server checks for updates to the HTTPS keystore file.") |
There was a problem hiding this comment.
Suggested change
| @ConfigDescription("Interval in seconds at which the server checks for updates to the HTTPS keystore file.") | |
| @ConfigDescription("Interval (in seconds) at which the server checks for updates to the HTTPS keystore file") |
| private Set<String> defaultAllowedRoles = ImmutableSet.of(); | ||
| private boolean allowUnsecureRequestsInAuthorizer; | ||
|
|
||
| private int keystoreScanInterval = 1; |
There was a problem hiding this comment.
nit: Lets rename to keyStoreScanIntervalSeconds
| sslContextFactory.setKeyManagerPassword(config.getKeyManagerPassword()); | ||
| } | ||
| // Scan for keystore file changes every 1s (the default). Interval can be made configurable in the future | ||
| KeyStoreScanner keyStoreScanner = new KeyStoreScanner(sslContextFactory); |
There was a problem hiding this comment.
Should we add a similar scanner to the while creating theadminConnector on line 329 ?
| @Test | ||
| public void testKeyStoreReloadOnKeystoreFileChange() | ||
| throws Exception | ||
| { |
There was a problem hiding this comment.
Let's test instead with a airlift HttpServer created via createAndStartServer.
We can first start a server like in the testClientCertificateJava test -
config.setHttpEnabled(false)
.setAdminEnabled(false)
.setHttpsEnabled(true)
.setHttpsPort(0)
.setKeystorePath(getResource("clientcert-java/server.keystore").getPath())
.setKeystorePassword("airlift");
createAndStartServer(createCertTestServlet());
try (JettyHttpClient httpClient = new JettyHttpClient(clientConfig)) {
StringResponse response = httpClient.execute(
prepareGet().setUri(httpServerInfo.getHttpsUri()).build(),
createStringResponseHandler());
assertEquals(response.getStatusCode(), HttpServletResponse.SC_OK);
assertEquals(response.getBody(), "CN=testing,OU=Client,O=Airlift,L=Palo Alto,ST=CA,C=US");
}This asserts that the certificate is loaded as expected
Then the test could replace the contents of clientcert-java/server.keystore with that of a (precreated) clientcert-java/replaced.keystore
See the clientcert.sh commands to pre-create this new keystore. We could add -
openssl req -new -key server.key -subj "/C=US/ST=CA/L=Palo Alto/O=Replacement/OU=Server/CN=localhost" -out replacement.csr
openssl x509 -req -days 9999 -in server.csr -CA ca.crt -CAkey ca.key -set_serial 01 -out replacement.crt
openssl pkcs12 -name server -inkey server.key -in replacement.crt -export -passout pass:airlift -out replacement.keystore
keytool -import -noprompt -alias ca -file ca.crt -storetype pkcs12 -storepass airlift -keystore replacement.keystoreto do this
The new httpClient response after the change should assert that the body changed to "CN=testing,OU=Client,O=Replacement,L=Palo Alto,ST=CA,C=US
Author
There was a problem hiding this comment.
Rewritten the testcase based on above suggestion.
ShahimSharafudeen
force-pushed
the
keyStoreScanner
branch
from
4b80fc1 to
fd6f5ae
Compare
ZacBlanco
reviewed
Nov 17, 2025
|
|
||
| @Config("http-server.https.keystore.scan-interval") | ||
| @ConfigDescription("Interval (in seconds) at which the server checks for updates to the HTTPS keystore file") | ||
| public HttpServerConfig setKeystoreScanInterval(int keyStoreScanIntervalSeconds) |
There was a problem hiding this comment.
I think it would make more sense if this was a configuration value of Duration type instead of an integer. It will be easier to configure. Especially since the current iteration of all the method names omit "seconds".
There was a problem hiding this comment.
The actual Jetty method takes seconds only unfortunatley. Using a fractional duration rounded down to seconds may be confusing to users
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks. Makes sense. I would just recommend making the config name clearer that it accepts a value in seconds (rather than just the description)
aaneja
reviewed
Nov 18, 2025
|
|
||
| Files.copy(Path.of(replacementKeyStorePath), Path.of(tempKeyStorePath), StandardCopyOption.REPLACE_EXISTING); | ||
|
|
||
| // Wait for the KeyStoreScanner to detect the file change. Default scan interval is 1 second, so sleeping for 3.5 seconds |
aaneja
suggested changes
Nov 18, 2025
aaneja
left a comment
aaneja
left a comment
There was a problem hiding this comment.
New test looks good ! Just a small comment about removing duplicate code
| Files.copy(Path.of(originalKeyStorePath), Path.of(tempKeyStorePath), StandardCopyOption.REPLACE_EXISTING); | ||
|
|
||
| config.setHttpEnabled(false) | ||
| .setAdminEnabled(true) |
There was a problem hiding this comment.
Since having admin enabled/disabled is the only difference, lets convert this into a dataprovider test that sets/unsets this flag
| keytool -import -noprompt -alias ca -file ca.crt -storetype pkcs12 -storepass airlift -keystore client.truststore | ||
|
|
||
|
|
||
| #Creating the replacement keystore files for both the server and the client |
There was a problem hiding this comment.
nit : Let's be more descriptive - A kesytore for testing the KeyStoreScanner's ability to transparently update the ssl config without needing a restart
ShahimSharafudeen
force-pushed
the
keyStoreScanner
branch
2 times, most recently
from
d0fbe04 to
c22054e
Compare
aaneja
reviewed
Nov 19, 2025
| private Set<String> defaultAllowedRoles = ImmutableSet.of(); | ||
| private boolean allowUnsecureRequestsInAuthorizer; | ||
|
|
||
| private int keyStoreScanIntervalSeconds = 1; |
There was a problem hiding this comment.
Lets keep the default as 0. This would match the current behavior of only scanning the file on first load. Ref - https://javadoc.jetty.org/jetty-9/org/eclipse/jetty/util/Scanner.html#setScanInterval(int)
aaneja
approved these changes
Nov 19, 2025
aaneja
left a comment
aaneja
left a comment
There was a problem hiding this comment.
LGTM % lets have the default scan interval as 0 (no scan after first load)
To allow for hot reloading the tls keystore See for more details : https://jetty.org/docs/jetty/12.1/programming-guide/server/http.html#connector-protocol-tls-keystore-auto-reload Co-authored-by: Anant Aneja <1797669+aaneja@users.noreply.github.com>
ShahimSharafudeen
force-pushed
the
keyStoreScanner
branch
from
c22054e to
e8dd570
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To allow for hot reloading the tls keystore
See for more details : https://jetty.org/docs/jetty/12.1/programming-guide/server/http.html#connector-protocol-tls-keystore-auto-reload
Testing
Tested in local and working as expected. Adding the screenshot as below.
Default keystoreScanInterval : 1s
keystoreScanInterval : 10s