Harden config map value handling in custom prometheus config generation - #1766
Open
suyadav1 wants to merge 2 commits into
Open
Harden config map value handling in custom prometheus config generation#1766suyadav1 wants to merge 2 commits into
suyadav1 wants to merge 2 commits into
Conversation
Values in the prometheus_data_collection_settings section of the container-azm-ms-agentconfig config map were interpolated into the generated telegraf configuration without escaping. A value containing a quote and a newline could terminate its assignment and declare an additional telegraf plugin, which telegraf then runs inside a privileged agent. The interval value was additionally written into the telemetry file that main.sh sources, so it could also inject shell. Changes: - Validate interval as a positive telegraf duration anchored with \A and \z, falling back to the default when invalid. Line anchors would let a payload hide on a second line. The accepted unit set is the intersection of the linux and windows telegraf versions, so fractional and compound durations keep working and "d" stays unsupported. - Serialize every config map derived scalar and array element as a quoted TOML basic string with structural characters escaped. This covers interval, fieldpass, fielddrop, urls, kubernetes_services, the label and field selectors, and the generated per namespace plugins. - Validate monitor_kubernetes_pods_namespaces entries as RFC 1123 labels and skip entries that are not. - Use the block form of gsub so that escape sequences in a substituted value are not reinterpreted as regex backreferences. - Never echo a rejected value back into the logs. Rejected values fall back to the documented default rather than failing the agent, since a typo should not stop monitoring. Adds regression tests covering the replicaset, prometheus sidecar, daemonset and windows templates plus the namespace generated plugins, and registers build/common/installer/scripts with the ruby test driver. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def test_toml_string_array_escapes_structural_characters | ||
| parsed = Tomlrb.parse("urls = #{toTomlStringArray(["http://ok", BREAKOUT])}") | ||
| assert_equal ["http://ok", BREAKOUT], parsed["urls"] |
The new prometheus custom config tests are the first ruby tests that
require tomlrb, so the unit test workflow failed to load them. Install
the same version the agent ships (2.0.1).
That version cannot parse inline tables with quoted keys, which the
telegraf templates use for `tags = {"interface" = ["lo"]}`, so the test
helper now collapses inline table values before parsing. Injected
plugins declare their own table headers and are still detected, which
was confirmed by re-running the suite against the unpatched parser.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
2 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.
Summary
Values from the
prometheus_data_collection_settingssection of thecontainer-azm-ms-agentconfigconfig map were interpolated into the generated telegraf configuration without escaping. A value containing a quote and a newline could terminate its own assignment and declare an additional telegraf plugin, which telegraf then executes inside a privileged agent. Theintervalvalue additionally reached the telemetry file thatmain.shsources, so it could inject shell as well.Changes
All in
build/common/installer/scripts/tomlparser-prom-customconfig.rb, the single parser shared by the Linux and Windows agents.\A/\z. Line anchors (^/$) would let a payload hide on a second line; the existing helper intomlparser-agent-config.rbhas exactly that flaw, so it was deliberately not reused. Accepted units are the intersection of the shipped Linux (1.39.x) and Windows (1.24.x) telegraf versions:ns, us/µs, ms, s, m, h, including fractional (1.5s) and compound (1h30m) forms.dis excluded because the Windows version rejects it. Zero and negative durations are rejected.intervalalone was not sufficient:fieldpass,fielddrop,urls,kubernetes_services, the label/field selectors and the per-namespace generated plugins all used the same unescaped quote interpolation and offered equivalent breakouts.monitor_kubernetes_pods_namespacesentries are validated as RFC 1123 labels. Invalid entries are skipped while valid ones still apply.gsub. With a replacement string, Ruby reinterprets\\and\0in the escaped value as backreferences, which would have silently undone the escaping.Behaviour on invalid input
Invalid values fall back to the documented
1mdefault and log a warning rather than failing the agent — a typo in a config map should not stop monitoring on every node.kubernetes/container-azm-ms-agentconfig.yamlis updated to document the fallback.Compatibility
Valid existing configurations are unaffected. Array rendering is byte-identical (
["a","b"]) and valid intervals pass through unchanged. Tests assert both.Testing
build/common/installer/scripts/tomlparser-prom-customconfig_test.rb: 32 tests, 261 assertions, all passing. The tests run the real parser end to end against the real templates for all four sinks (replicaset, prometheus sidecar, daemonset, Windows) plus the namespace-generated plugins, parse the resulting TOML, and assert the input-plugin set is unchanged versus a benign baseline.The tests were verified to actually catch the bug. Against the pre-fix parser they fail with:
reproducing both the telegraf sink and the shell sink. With the fix applied all 32 pass.
test/unit-tests/test_driver.rbnow also globsbuild/common/installer/scripts/*_test.rb, which previously had no coverage.Follow-ups, intentionally not in this PR
telegraf --testexit code (kubernetes/windows/main.ps1:976-978) and starts the service regardless. Note that--testis not a security boundary: on Windows it runs unfiltered and can itself execute an injected input plugin.main.shsources:tomlparser.rb,tomlparser-common-agent-config.rb,tomlparser-geneva-config.rb,tomlparser-metric-collection-config.rb.tomlparser-osm-config.rbinterpolates namespaces into TOML. Config map to node RCE should not be considered fully closed until these are addressed or separately tracked.@defaultRsMonitorPodsis commented out (line 25), so an unsetmonitor_kubernetes_podson the replicaset emitsmonitor_kubernetes_pods =, which is invalid TOML. Left alone deliberately, since changing it would newly enable a currently-broken path.ReleaseNotes.mdentry: entries there are tied to an image version bump and belong with the release PR.CI
These are the first ruby tests to require
tomlrb, so.github/workflows/run_unit_tests.ymlnow installs it, pinned to 2.0.1 to match the version the agent ships. The full ruby suite passes: 34 runs, 278 assertions, 0 failures.