feat(http-server): Add a KeyStoreScanner#135
Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom Dec 3, 2025
Merged
Conversation
|
|
Reviewer's GuideIntroduce dynamic HTTPS keystore reloading via Jetty KeyStoreScanner and add configuration, wiring, and tests to validate certificate rotation behavior on both main and admin connectors. Sequence diagram for HTTPS keystore reload via KeyStoreScannersequenceDiagram
actor Operator
participant FileSystem
participant KeyStoreScanner
participant SslContextFactory
participant JettyServer
participant HttpsConnector as HttpsConnector
participant Client
Operator->>FileSystem: Replace keystore file with new certificate
loop Periodic scan
KeyStoreScanner->>FileSystem: checkLastModified(keystorePath)
FileSystem-->>KeyStoreScanner: lastModifiedTimestamp
alt Keystore changed
KeyStoreScanner->>SslContextFactory: reloadFromKeystore()
SslContextFactory-->>KeyStoreScanner: reloadComplete
KeyStoreScanner->>JettyServer: notifyKeyStoreChanged()
JettyServer-->>KeyStoreScanner: acknowledged
else No change
KeyStoreScanner-->>KeyStoreScanner: waitForNextInterval
end
end
Client->>HttpsConnector: new TLS connection
HttpsConnector->>SslContextFactory: getCurrentCertificateChain()
SslContextFactory-->>HttpsConnector: certificateChain(with rotated cert)
HttpsConnector-->>Client: TLS handshake using new certificate
Class diagram for HttpServerConfig and HttpServer keystore scanning integrationclassDiagram
class HttpServerConfig {
- boolean httpEnabled
- boolean httpsEnabled
- String keyStorePath
- String keyStorePassword
- String keyManagerPassword
- int keyStoreScanIntervalSeconds
+ boolean isHttpEnabled()
+ boolean isHttpsEnabled()
+ String getKeyStorePath()
+ String getKeyStorePassword()
+ String getKeyManagerPassword()
+ HttpServerConfig setKeystoreScanIntervalSeconds(int keyStoreScanIntervalSeconds)
+ int getKeystoreScanIntervalSeconds()
}
class HttpServer {
- HttpServerInfo httpServerInfo
- HttpServerConfig config
- Server server
+ HttpServer(HttpServerInfo httpServerInfo, HttpServerConfig config, ...)
}
class Server {
+ void addBean(Object bean)
}
class Connector {
+ void addBean(Object bean)
}
class SslContextFactory {
+ void setKeyStorePath(String keyStorePath)
+ void setKeyStorePassword(String keyStorePassword)
+ void setKeyManagerPassword(String keyManagerPassword)
}
class KeyStoreScanner {
- SslContextFactory sslContextFactory
+ KeyStoreScanner(SslContextFactory sslContextFactory)
+ void setScanInterval(int seconds)
}
class AdminConnector extends Connector
HttpServerConfig "1" --> "1" HttpServer : provides_config
HttpServer "1" --> "1" Server : wraps
HttpServer "1" --> "1" SslContextFactory : configures_https
Server "1" --> "*" Connector : has_connectors
Server "1" --> "*" KeyStoreScanner : manages_scanners
Connector "1" --> "*" KeyStoreScanner : manages_scanners
HttpServer "1" --> "1" AdminConnector : admin_https
KeyStoreScanner "1" --> "1" SslContextFactory : scans_and_triggers_reload
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
aaneja
force-pushed
the
keyStoreScanner
branch
2 times, most recently
from
e11ceab to
ac569ad
Compare
![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:
- The new keystore scan interval config is accepted as an int without validation; consider enforcing a non-negative (or strictly positive) range and/or documenting what a value of 0 means to avoid ambiguous behavior at runtime.
- In testClientCertificateJava, the fixed Thread.sleep(2500) tied to the scan interval can make the test brittle and slow; consider polling for the expected behavior or otherwise synchronizing with the KeyStoreScanner rather than relying on a hard-coded sleep.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new keystore scan interval config is accepted as an int without validation; consider enforcing a non-negative (or strictly positive) range and/or documenting what a value of 0 means to avoid ambiguous behavior at runtime.
- In testClientCertificateJava, the fixed Thread.sleep(2500) tied to the scan interval can make the test brittle and slow; consider polling for the expected behavior or otherwise synchronizing with the KeyStoreScanner rather than relying on a hard-coded sleep.
## Individual Comments
### Comment 1
<location> `http-server/src/test/java/com/facebook/airlift/http/server/TestHttpServerProvider.java:322-331` </location>
<code_context>
- @Test
- public void testClientCertificateJava()
+ @Test(dataProvider = "adminMode")
+ public void testClientCertificateJava(boolean adminMode)
throws Exception
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid fixed Thread.sleep in test and use a more robust way to wait for keystore reload
Relying on `Thread.sleep(2500)` to wait for `KeyStoreScanner` makes this test flaky and slow, especially in CI. Instead, could you poll for the expected behavior with a timeout (e.g., repeatedly issue requests until the subject reflects the new CN or a max wait is reached)? This should improve reliability and may complete faster when the reload happens quickly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+322
to
+331
| @Test(dataProvider = "adminMode") | ||
| public void testClientCertificateJava(boolean adminMode) | ||
| throws Exception | ||
| { | ||
| tempDir = createTempDirectory("test-keystore").toFile().getCanonicalFile(); | ||
| String tempKeyStorePath = tempDir.toPath().resolve("server.keystore").toAbsolutePath().toString(); | ||
| String originalKeyStorePath = getResource("clientcert-java/server.keystore").getPath(); | ||
| String replacementKeyStorePath = getResource("clientcert-java/replacementServer.keystore").getPath(); | ||
|
|
||
| Files.copy(Path.of(originalKeyStorePath), Path.of(tempKeyStorePath), StandardCopyOption.REPLACE_EXISTING); |
There was a problem hiding this comment.
suggestion (testing): Avoid fixed Thread.sleep in test and use a more robust way to wait for keystore reload
Relying on Thread.sleep(2500) to wait for KeyStoreScanner makes this test flaky and slow, especially in CI. Instead, could you poll for the expected behavior with a timeout (e.g., repeatedly issue requests until the subject reflects the new CN or a max wait is reached)? This should improve reliability and may complete faster when the reload happens quickly.
aaneja
force-pushed
the
keyStoreScanner
branch
5 times, most recently
from
16b2102 to
c1ea7bb
Compare
aaneja
force-pushed
the
keyStoreScanner
branch
from
c1ea7bb to
e3fc214
Compare
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: Shahim Sharafudeen <33122287+shahimsharafudeen@users.noreply.github.com>
aaneja
force-pushed
the
keyStoreScanner
branch
from
e3fc214 to
c269182
Compare
Author
|
@sourcery-ai review |
aaneja
commented
Dec 3, 2025
| return new Object[][] {{false}, {true}}; | ||
| } | ||
|
|
||
| private static HttpServlet createCertTestServlet() |
Author
There was a problem hiding this comment.
Accidental move with code formatting, but good to have this private static on top
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
HttpServer, theKeyStoreScannersetup for the main connector and the admin connector is duplicated; consider extracting a small helper that conditionally creates/adds the scanner (e.g., skipping it when the scan interval is zero) to avoid repetition and keep the semantics consistent. - The new
keyStoreScanIntervalSecondsfield inHttpServerConfigappears to accept any int; if negative or very small values are not meaningful, consider adding validation (e.g., via a constraint annotation or explicit check) to prevent misconfiguration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `HttpServer`, the `KeyStoreScanner` setup for the main connector and the admin connector is duplicated; consider extracting a small helper that conditionally creates/adds the scanner (e.g., skipping it when the scan interval is zero) to avoid repetition and keep the semantics consistent.
- The new `keyStoreScanIntervalSeconds` field in `HttpServerConfig` appears to accept any int; if negative or very small values are not meaningful, consider adding validation (e.g., via a constraint annotation or explicit check) to prevent misconfiguration.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider only creating/adding the
KeyStoreScannerbean whengetKeystoreScanIntervalSeconds()is greater than zero to avoid unnecessary scanner setup when the feature is effectively disabled. - In
testKeystoreUpdate, usingPath.of(getResource(...).getPath())can break with spaces or special characters in the resource path; prefer constructing thePathfromgetResource(...).toURI()instead for robustness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider only creating/adding the `KeyStoreScanner` bean when `getKeystoreScanIntervalSeconds()` is greater than zero to avoid unnecessary scanner setup when the feature is effectively disabled.
- In `testKeystoreUpdate`, using `Path.of(getResource(...).getPath())` can break with spaces or special characters in the resource path; prefer constructing the `Path` from `getResource(...).toURI()` instead for robustness.
## Individual Comments
### Comment 1
<location> `http-server/src/main/java/com/facebook/airlift/http/server/HttpServerConfig.java:829-831` </location>
<code_context>
return httpComplianceViolations;
}
+
+ @Config("http-server.https.keystore.scan-interval-seconds")
+ @ConfigDescription("Interval (in seconds) to check the keystore file for changes")
+ public HttpServerConfig setKeystoreScanIntervalSeconds(int keyStoreScanIntervalSeconds)
+ {
+ this.keyStoreScanIntervalSeconds = keyStoreScanIntervalSeconds;
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider validating the scan-interval value to guard against negative inputs.
This setter forwards any `int` to `KeyStoreScanner#setScanInterval`. A negative value (from misconfiguration) could lead to unexpected behavior or runtime errors in the Jetty scanner. Adding a guard, e.g. `checkArgument(keyStoreScanIntervalSeconds >= 0, "...")`, would fail fast and surface misconfigurations early.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
The default behavior for the scanner when set to |
aaneja
changed the title
tdcmeehan
approved these changes
Dec 3, 2025
7 tasks
aaneja
added a commit
to prestodb/presto
that referenced
this pull request
Dec 4, 2025
Airlift release details - https://github.com/prestodb/airlift/actions/runs/19903584529/job/57053806773 ## Motivation and Context - Upgrade to Jetty 12.0.29 which includes recent CVE fixes - Add ability to hot swap the keystore, see prestodb/airlift#135 for details ## Impact Users can now hot swap their keystore files to update certs ## Test Plan CI ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * Add a new ``http-server.https.keystore.scan-interval-seconds`` configuration flag to scan the keystore file periodically for new certs * Upgrade Jetty to 12.0.29 in response to `CVE-2025-5115 <https://nvd.nist.gov/vuln/detail/CVE-2025-5115>`_. ```
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.
Rebased clone of #134, since @ShahimSharafudeen will be OOO for a while
Additionally -