fix(sinks): confinement follow-ups from #25820 codex review#25830
Draft
pront wants to merge 8 commits into
Draft
fix(sinks): confinement follow-ups from #25820 codex review#25830pront wants to merge 8 commits into
pront wants to merge 8 commits into
Conversation
github-actions
Bot
added
docs review on hold
|
pront
added
the
no-changelog
Previously only tenant_id was confined; label key/value and structured_metadata key/value templates were rendered directly, so an event whose data controlled a templated label could steer to any Loki stream even after this PR's confinement work. Both maps now have confine() applied to each key and value template at build time, and the render paths return an Err(()) that encode_event translates into an intentional security drop when either key or value render fails with Confined.
Under `auto_routing: true` (the default), DataStreamConfig::index reads
data_stream.{type,dataset,namespace} directly off the event, completely
bypassing the confined templates. A log producer that controls those
event fields could steer to arbitrary data streams even after this PR's
confinement work.
Add Template::check_confinement(rendered) so callers that skip normal
template rendering (like the auto-routing path) can still run the
attached checker against a raw string. Elasticsearch's auto-routing
branch now runs the pulled event value through the corresponding
template's confinement check and drops the event on Confined.
When the operator-authored template for data_stream.{type,dataset,namespace}
is static (e.g. the default type = "logs"), no ConfinementChecker gets
attached and Template::check_confinement returns Ok for any raw event
value. So under auto_routing: true (the default), an attacker-controlled
event field like data_stream.type: "../evil" bypassed the check even
though the goal of the previous auto_routing fix was to plug exactly that
hole.
Add a baseline is_valid_data_stream_component check that rejects any
value containing path separators, .. or . segments, NUL bytes, control
characters, or exceeding Elasticsearch's 255-byte name limit. Applied to
every auto-routed value regardless of whether the corresponding template
has a checker attached.
UriChecker had no query handling. Two attack surfaces:
1. Build: a template like ?tenant={{ tenant }} lets an attacker
supply a value containing & to append arbitrary key=value pairs.
Every attempt to reason about & boundaries and multi-field
patterns was either bypassable or false-positive on operator-
authored static suffixes. Reject any URI template whose literal
prefix crosses into the query at build time; operators can put
dynamic routing in the path instead.
2. Render: with the build check in place, a URI checker only exists
for pathful templates. So any rendered URI with a query means a
field value smuggled ?... into the path. Example: template
.../ingest/{{ tenant }} with tenant = 'ok?tenant=evil' renders
to .../ingest/ok?tenant=evil — same authority, same path prefix,
attacker-controlled query. Reject any rendered URI with a
non-empty query.
pront
force-pushed
the
pront-confinement-followups
branch
from
da2f05d to
c3ba20a
Compare
github-actions
Bot
removed
domain: sources
…fig.labels templates PR #25820 confined `log_id` only. `resource.labels` and `label_config.labels` templates were cloned into the encoder unchanged, so an event-controlled label value (e.g. `resource.labels.zone: "{{ zone }}"`) could still steer the destination monitored resource. Confine every label value template at build time. Render-time drops already fall through the encoder's existing `.ok()?` path with `drop_event: true`, so `Confined` renders map to intentional drops via the parent PR's error_type wiring.
PR #25820 confined the URI but left `template_headers` (dynamic values from `RequestConfig::split_headers`) unchecked. Header-based routing like `X-Scope-OrgID: "{{ tenant }}"` is as steerable as URI routing — an event that controls the header field picks the destination tenant. Run `.confine()` on each templated header value at build time.
…utates event
process_log extracts and removes fields (like `timestamp`) from the
event body. Running the confinement pre-check *after* process_log
turned a Confined render into MissingKeys when the confinement error
came from a field process_log had just removed — the security drop
was silently downgraded to 'field missing', and the attack event
shipped without an index.
Example under `index: "idx-{{ timestamp }}"`: an event with
`timestamp = "../../evil"` used to render as Confined inside
process_log, then process_log removed the timestamp field, then the
pre-check re-rendered and saw MissingKeys instead of Confined.
Move the pre-check to run on the original Event before process_log,
and change has_confined_partition_error to take an `&Event` rather
than an `&HecProcessedEvent`. Adds a regression test that reproduces
the bypass shape.
Closes #25829.
TemplateRenderingError previously hard-coded error_type=template_failed
for every render failure. Sinks that surface confinement via this event
(Kafka, Redis, HTTP URI, S3 KMS key, Stackdriver labels, HTTP headers,
Splunk HEC pre-check) therefore all emitted 'template_failed' for
confinement drops. Operators alerting on error_type='condition_failed'
per the parent PR's changelog missed every one of them.
Branch on the underlying error variant: Confined maps to
error_type='condition_failed' with a message wording that identifies
the class ('Templated routing value was outside the configured
confinement; dropping event.'), everything else keeps the previous
'template_failed'/'Failed to render template.' pair. Ordinary render
failures (MissingKeys, NotNumeric) are legitimate pre-existing drops
and stay tagged as template_failed.
Also amends the parent PR's still-unreleased security changelog
fragment to make the observability contract accurate: condition_failed
is the canonical filter for confinement drops specifically, not every
drop. Message wording is per-sink but the label is consistent.
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.
Summary
Addresses the 13 items surfaced in the Codex review of #25820 (
.claude/investigations/pr25820-confinement-followups.mdfrom that branch). This PR is stacked onpront-sink-template-confinement— do not merge until #25820 lands.Twelve items are addressed in code; one (Splunk HEC
process_logbypass) is tracked as an issue: #25829.Landed
Attacker-controlled routing gaps (P1):
labels+structured_metadatatemplates now confined (build-time + drop onConfined).auto_routing: event-sidedata_stream.{type,dataset,namespace}values run through the corresponding template's confinement check.UriCheckernow records and enforces the static query prefix, so?tenant=team-{{ tenant }}+a&tenant=otherno longer slips through.Correctness gaps (P2):
TemplateRenderingError::Confinednow emitserror_type=condition_failedon log + counter (matches the security changelog).https://logs-{{ region }}.example.com/...) rejected at build time rather than dropping 100% of rendered events.Template::confine_uri) rather than inferring from whether the literal prefix starts withhttp. Object-store keys that happen to start withhttp://no longer get URI-checked.resource.labelsandlabel_config.labelstemplates confined.base_dirnow enforced even whenpathis static.Usability regressions (P2):
HumioLogsConfig/HumioMetricsConfignow exposedangerously_allow_unconfined_template_resolutionand forward it to the wrapped HEC build.rootcomposed into the partitioner key template soroot: /logs+prefix: "{{ tenant }}/"is properly confined by/logs/.bulk.indexno longer confined whenmode: data_stream, and vice versa.Deferred
process_logmutation defeats the confinement pre-check — filed as Splunk HEC logs: process_log mutation defeats confinement check #25829. Fix direction is uncertain (run on original event vs threadConfinedout ofprocess_log); the operator asked to file this rather than land a specific approach.Change Type
Is this a breaking change?
Config changes are strictly additive (new
dangerously_allow_unconfined_template_resolutionon Humio sinks, no other externally-visible surface changes).Does this PR include user facing changes?
no-changeloglabel to this PR.The user-facing security story is covered by #25820's
.securityfragment; these are follow-up hardening on the same landing.References