Etag config TLS - #9033
Conversation
There was a problem hiding this comment.
Pull request overview
Adds ETag / HTTP 304 handling to the TLS config plugin so repeated config refreshes can avoid downloading unchanged configs, with a --config_tls_etag opt-out. This extends the remote request stack to surface HTTP status codes and response headers, and updates the test TLS HTTP server plus unit tests to exercise the new behavior.
Changes:
- Add ETag caching in
TLSConfigPlugin(sendIf-None-Match, reuse cached config on 304) guarded by--config_tls_etag. - Extend
TLSRequestHelper,Request, andTLSTransportto expose response status code and headers (ETag) to callers. - Add/expand test server controls and new unit tests covering 200→304, payload changes, disabled mode, node API behavior, and error handling.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/tests/test_http_server.py | Adds ETag/304 support and test control endpoints to the TLS test server. |
| plugins/config/tls_config.h | Adds cached ETag/config fields and a mutex to serialize genConfig(). |
| plugins/config/tls_config.cpp | Implements ETag-based conditional requests and 304 cache replay for TLS config fetches. |
| plugins/config/tests/tls_config_tests.cpp | Adds unit tests validating ETag and 304 behavior for TLS config. |
| osquery/remote/utility.h | Extends TLSRequestHelper API to optionally send ETag and return response metadata. |
| osquery/remote/transports/tls.h | Exposes response status code and header accessors on TLSTransport. |
| osquery/remote/transports/tls.cpp | Adds If-None-Match header injection when an ETag option is set. |
| osquery/remote/requests.h | Plumbs response status/header access through the Request wrapper. |
| osquery/remote/http_client.h | Adds header() accessor and makes status() const on HTTP_Response. |
| osquery/remote/http_client.cpp | Stops treating HTTP 304 as a redirect-like status in the redirect switch. |
| osquery/config/tests/config_tests.cpp | Adds a regression test ensuring rejected config updates do not alter hashes. |
| osquery/config/config.cpp | Moves hash computation to after validation and adjusts parsing guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif self.path == "/reset_config_test_state": | ||
| CONFIG_PAYLOAD_OVERRIDE = None | ||
| CONFIG_STATUS_OVERRIDE = None | ||
| CONFIG_ETAG_EVENTS.clear() | ||
| ENROLL_RESET["count"] = 0 | ||
| ENROLL_RESET["max"] = 1000000 | ||
| self._reply({}) |
| if ((!clone.empty() && !doc.fromString(clone, JSON::ParseMode::Iterative)) || | ||
| !doc.doc().IsObject()) { | ||
| return Status::failure("Error parsing the config JSON"); | ||
| } |
74867a6 to
d4dc018
Compare
|
Reworked this PR based on design feedback: the etag now travels in the JSON request/response bodies instead of HTTP |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
plugins/config/tls_config.cpp:133
- The error message for a server returning the reserved "ok" etag is misleading: the client does send an "etag" field (it’s just empty when there is no prior validator). This makes debugging server/client behavior harder and contradicts the conditional-request contract described in docs.
if (etag == kEtagNotModified) {
if (etag_.empty()) {
return Status::failure(
"Server signaled an unchanged config to a request that sent "
"no etag");
osquery/config/config.cpp:767
updateSourcenow treats an empty config string as a valid empty JSON object because parsing is skipped whencloneis empty. This means an empty/partial response could wipe the active config instead of failing the refresh (previous behavior). Also, since comments are stripped intoclone, hashingjson(the unstripped content) reintroduces unnecessary reloads for comment-only changes.
if ((!clone.empty() && !doc.fromString(clone, JSON::ParseMode::Iterative)) ||
!doc.doc().IsObject()) {
return Status::failure("Error parsing the config JSON");
lucasmrod
left a comment
There was a problem hiding this comment.
LGTM! Left some comments/questions.
| elif self.path == "/reset_config_test_state": | ||
| CONFIG_PAYLOAD_OVERRIDE = None | ||
| CONFIG_STATUS_OVERRIDE = None | ||
| CONFIG_ETAG_EVENTS.clear() | ||
| ENROLL_RESET["count"] = 0 | ||
| ENROLL_RESET["max"] = 1000000 | ||
| self._reply({}) |
Extend the Status(2) 'unchanged' convention from Config::updateSource() to the plugin boundary: genConfig() may return Status(2) when it knows the configuration has not changed since it was last applied, and Config::refresh() then skips the update entirely, restoring the normal refresh interval and leaving the installed config untouched. After a successful update, refresh() notifies the active config plugin via a new 'configApplied' action (a default no-op), so a plugin can distinguish a config it delivered from a config that took effect.
In deployments that poll for configuration frequently, config responses
can dominate traffic even though the config almost never changes between
refreshes. Implement conditional requests with the validator carried in
the JSON bodies, because the config request is a POST where HTTP
ETag/If-None-Match semantics do not apply and intermediary behavior
around them is unpredictable.
Unless --config_tls_etag=false, the plugin includes an 'etag' field in the
request body carrying the value from the last config response it received
(empty at first; the field's presence is the opt-in). A supporting server
returns the config with an 'etag' key, or the reserved minimal body
{"etag":"ok"} when the config is unchanged, in which case the plugin
reports Status(2) and the refresh skips the reload.
The value is server-assigned and opaque; the client never computes one. It
is held in memory only, so a restart fetches the full config. The etag key
is stripped before the config is applied, keeping the applied bytes
identical to a non-etag agent's. A config that fails to apply is never
acknowledged (via the configApplied hook), so the failure is logged on
every refresh without re-downloading the unchanged payload. Servers that
ignore the field, and agents that do not send it, see today's exact
behavior.
d4dc018 to
961a24f
Compare
|
@lucasmrod I believe that I have backed out the code move and stored it in a separate branch. I have a separate bug report I can file for it separately. The http rest finding by copilot was also addressed. If all automated tests pass here, I think this addresses everything you brought up. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
osquery/config/config.cpp:553
- Config::refresh treats Status code 2 as "unchanged" success unconditionally, even during the very first refresh (Config::load calls refresh()). If a misbehaving/older external config plugin returns code 2 before any config has ever been applied, osquery will mark the config as loaded/valid while skipping update, leaving the process running without a real configuration.
if (status.getCode() == 2) {
// The plugin reports that the configuration is unchanged since it was
// last applied; there is nothing to reload. Plugins may only report
// this after a configuration was applied in this process lifetime.
if (getRefresh() != FLAGS_config_refresh) {
plugins/config/tls_config.cpp:134
- The failure message says the request "sent no etag", but in this code path the client explicitly sent the etag field (it was just empty). Wording this as "empty etag" avoids confusion when debugging server/client behavior.
return Status::failure(
"Server signaled an unchanged config to a request that sent "
"no etag");
}
docs/wiki/deployment/remote.md:99
- PR description mentions "HTTP 304" support, but this implementation/documentation makes conditional config semantics part of the JSON body and does not use HTTP 304 / If-None-Match headers. Calling out 304 explicitly here would prevent confusion for server implementers reading the docs.
Configurations rarely change between refreshes, so the **tls** config plugin supports conditional requests carried in the JSON bodies (the request is a `POST`, so HTTP `ETag`/`If-None-Match` header semantics do not apply). Servers are free to ignore all of this: a response without an `"etag"` key is treated as the configuration content, byte for byte, exactly as before.
This PR adds ETag/HTTP 304 support to the config TLS feature of osquery with an opt out config option. In deployments where you want fast convergence of configuration changes (say every minute), configs can become a HUGE percentage of all bandwidth uses. This can lower that by over 99% if you don't often change configs but just want them picked up immediately.
Since we only submit an "If-None-Match" if we receive an ETag, it shouldn't break any existing implementations, but I added an opt-out just-in case.
The opt-out can be removed if it doesn't make sense.