Settings: send query parameters on http(s) settings urls - #1404
Merged
Conversation
A boot.ini entry such as [settings] 1 = http://nsclient.mydom.local/nsclient.php?RootFolder=myhost/&Filename=nsclient.ini was fetched as a bare "GET /nsclient.php": net::parse splits the query off the path into url.query, but settings_http only ever handed url.path to the downloader, so every parameter was silently dropped and a script that generates per-host configuration could never see which host was asking. Add net::url::get_request_path(), which reassembles path and query into the form that belongs on the request line, and use it for the settings download. url::to_string() now includes the query too, so the log lines and TLS warnings name the url that is actually being fetched. Proxied requests are covered as well: make_proxy_request builds its absolute URI from the same path. Caching had to follow. resolve_cache_file derived the cache file name from the url path alone, so two entries pointing at the same script with different parameters resolved to one file and overwrote each other. The query now contributes a short digest to the name - it cannot be appended verbatim since '?' and '&' are not legal in a Windows file name. A url with no file name at all ("http://host/?file=x") no longer collapses onto the cache directory itself but falls back to cached.ini. Also drop the dead http::request in cache_remote_file; it was built from url.path and never sent. Tests: new net_test.cpp covers url parsing, get_request_path and to_string round-tripping; settings_http_test now asserts on the request line the loopback server actually receives, and on cache file separation. Fixes #460 Assisted-by: Claude Code Signed-off-by: Michael Medin <michael@medin.name>
Follow-up to the http settings query fix, addressing review findings.
Cache file migration. Adding the query digest to the cache file name
renames the cache file of exactly the urls the fix is aimed at. That
matters because cache_remote_file falls back to the cached copy when the
settings server cannot be reached: an agent upgrading while its server was
down would have found nothing under the new name and booted with an empty
configuration, having booted fine off the cache the day before.
migrate_legacy_cache_file moves the old file into place once, so the
fallback keeps working across the upgrade. Query-less urls keep their name
and are left alone.
Request line encoding. The query is now concatenated onto the request
target, so characters that are illegal there reach the wire for the first
time. A space produced a malformed three-token request line; a CR or LF
split one request into two. net::encode_query percent-encodes anything
outside the RFC 3986 query grammar, passing an existing "%XX" through
untouched so an already-encoded query is not encoded twice, and escaping a
'%' that introduces no valid pair.
Log hygiene. A settings url may authenticate with its parameters
("?token=..."), and the previous commit put the whole url in the log,
including at warning level on every boot when TLS verification is off or
the CA bundle is missing. Add url::get_baseurl(), url::get_path() and
url::to_log_safe_string() composing the two, and use the latter for the
settings log lines and TLS advisories. to_string() keeps the query and
remains the faithful rendering.
Not addressed here: the cache file name still ignores host and directory,
so two urls whose paths differ only above the file name continue to share
one cache file. That predates this series and is left for its own change.
Fixes #460
Assisted-by: Claude Code
Signed-off-by: Michael Medin <michael@medin.name>
Now that a settings url can carry parameters, the obvious next thing to put
in them is which host is asking - that is what turns one boot.ini into a
fleet-wide configuration instead of a per-host file:
[settings]
1 = http://cfgsrv/nsclient.php?host=${hostname}
Run settings urls through socket_helpers::expand_hostname, the same helper
the submit clients (NRDP, Graphite, Syslog, Icinga, ...) already use for
their "hostname" setting, so the placeholders mean the same thing wherever
they appear. Expansion happens before parsing, so a placeholder may sit
anywhere in the url - query, path or host - and before the query is
percent-encoded, so a host name needing an escape gets one rather than
corrupting the request line. Attachment urls go through the same path.
The cache file name is derived from the expanded url, so each host caches
its own configuration rather than sharing one "${hostname}" bucket.
expand_hostname itself gains ${hostname}, ${hostname_lc} and ${hostname_uc}
for the full system name. It only had ${host} and ${domain}, which split on
the first '.', so a template had no way to ask for the name as reported -
"auto" gives it, but only as the entire spec, which a url cannot be. This
is additive: the token was previously left in place as literal text. Every
module using expand_hostname picks it up, which is the intent - the
placeholder set should not differ between a settings url and an NRDP
sender name.
Assisted-by: Claude Code
Signed-off-by: Michael Medin <michael@medin.name>
The docs promised that NSClient++ logs settings urls as scheme, host and path only, but only the two TLS warnings in settings_http actually went through to_log_safe_string(). boot() still echoed the raw boot.ini entry three times - once at info level - and get_info(), which `nscp settings --show` prints, embedded the raw context. An operator who put a token in a settings url on the strength of that paragraph got it written to the log file verbatim on every boot. Redact at the remaining sites: the "Activating"/"Failed to activate"/ "using that" messages and the boot order list in settings_manager_impl, the "Undefined settings protocol" exception boot() logs, and get_info(). Also correct the version markers in the docs - these changes land after 0.16.0, not in 0.14 - and note that only the agent's own output is covered, not a proxy or the settings server's access log. Signed-off-by: Michael Medin <michael@medin.name> Assisted-by: Claude Code:claude-opus-5
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
1 participant
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.
Fixes #460.
The problem
A
boot.inientry such aswas fetched as a bare
GET /nsclient.php.net::parsesplits the query off the path intourl.query(include/net/net.hpp), butsettings_httponly ever handedurl.pathto the downloader, so every parameter was silently dropped — a script that generates per-host configuration could never see which host was asking.net::url::querywas in fact read nowhere in the tree.The fix
net::url::get_request_path()reassembles path and query into the form that belongs on the request line, andsettings_http::cache_remote_fileuses it for the download. Proxied requests come along for free:make_proxy_requestbuilds its absolute URI from the samepath_.url::to_string()now includes the query, soCreating instance for: …and the TLS advisories name the url that is actually being fetched rather than a truncated one.resolve_cache_filederived the name from the url path alone, so two entries pointing at the same script with different parameters resolved to one file and clobbered each other. The query now contributes a short digest to the name — it cannot be appended verbatim, since?and&are not legal in a Windows file name. Urls without a query keep their historic plain name, so existing installs do not re-download on upgrade.http://host/?file=x) previously collapsed onto the cache directory itself; it now falls back tocached.ini.http::requestincache_remote_file— it was built fromurl.pathand never sent.Tests
New
include/net/net_test.cpp(added to thenet_testtarget) covers url parsing,get_request_pathandto_stringround-tripping, including the empty-query andini://cases.settings_http_test.cpp: the loopback server now records the request line it receives, and four new cases assert that the query reaches the wire, that a url without one is unchanged, that two queries against the same script resolve to different cache files with names legal on every platform, and that a url without a file name still resolves inside the cache folder.Both new
settings_httpcases were confirmed to fail against the old code:With the fix,
settings_http_testpasses 14/14 andnet_test497/497 (1 skipped: the IPv6 pinger case, environmental).Docs: a "Query parameters" subsection under http settings in
docs/docs/concepts/settings.md.Generated by Claude Code