fix(#5436): inject --header values into collector YAML config#5442
fix(#5436): inject --header values into collector YAML config#5442fullsend-ai-coder[bot] wants to merge 5 commits into
Conversation
The otelcol-contrib Collector reads headers from its YAML config (exporters.otlphttp.headers), not from OTEL_EXPORTER_OTLP_HEADERS which is an OpenTelemetry SDK env var. The script was setting OTEL_EXPORTER_OTLP_HEADERS, which the Collector silently ignored, so --header flags like x-mlflow-experiment-id had no effect. Generate a runtime config file that injects parsed headers into the otlphttp exporter's headers block as YAML key-value pairs. The script still merges --header flags with any pre-existing OTEL_EXPORTER_OTLP_HEADERS env var for backwards compatibility. Header values are double-quoted for YAML safety. The temp config is cleaned up on exit via trap. Note: make lint could not run in sandbox (pre-commit failed due to network restrictions). Manual verification of YAML output confirmed correct structure for: no headers, single header, multiple headers, and values containing YAML special characters. Closes #5436
|
🤖 Finished Review · ✅ Success · Started 7:25 AM UTC · Completed 7:38 AM UTC |
Site previewPreview: https://e013ecbd-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsMedium
Low
Labels: PR fixes a bug in a developer utility script (hack/upload-traces.sh) |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
hack/upload-traces.sh(file-level): Line 74 · [medium] stale-reference
The script still validates that CONFIG_TEMPLATE (the static YAML file upload-traces-otelcol-config.yaml) exists on disk, but the generated RUNTIME_CONFIG is now used instead. The CONFIG_TEMPLATE variable (line 5) and its validation are dead code that will produce a misleading error if the now-unused template file is later removed.
Suggested fix: Remove the CONFIG_TEMPLATE variable (line 5) and its existence check (lines 74-75), or remove only the validation if the template is kept for reference.
hack/upload-traces.sh:127: [medium] YAML-injection
Header keys and values are written into the generated YAML config with insufficient sanitization. Keys have no sanitization at all; values only escape double quotes but not embedded newlines (which break out of YAML double-quoted strings) or YAML backslash escape sequences. The OTEL_EXPORTER_OTLP_HEADERS env var could be set by CI automation, making this a viable injection path in shared environments.
Suggested fix: Validate keys match [a-zA-Z0-9_-]+ at parse time, strip newline characters from values, and consider single-quoted YAML scalars for values (which have no escape sequences).
hack/upload-traces.sh:123: [low] edge-case
Header values containing commas are incorrectly split due to the comma-separated join/split approach. This is a pre-existing constraint inherited from the OTEL_EXPORTER_OTLP_HEADERS comma-separated format, not a regression introduced by this PR.
fullsend-ai-review
Bot
added
requires-manual-review
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Hector Martinez <hemartin@redhat.com>
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 11:51 AM UTC · Completed 11:56 AM UTC |
- Remove unused SCRIPT_DIR, CONFIG_TEMPLATE variable and its file existence check (dead code after switch to RUNTIME_CONFIG) - Validate header keys match [a-zA-Z0-9_-]+ to prevent YAML injection - Strip newline/carriage-return characters from header values - Switch to single-quoted YAML scalars (no backslash escape sequences) Addresses review feedback on #5442
🔧 Fix agent — iteration 1 (human-triggered)Addressed 2 of 3 review findings. Removed dead CONFIG_TEMPLATE code (stale-reference) and hardened YAML config generation against injection (key validation, newline stripping, single-quoted scalars). Disagreed with 1 finding (comma-in-value edge case) as it is pre-existing and out of scope per the reviewer's own assessment. Fixed (2):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
|
/fs-fix is the committed configuration file required anymore? Maybe we can remove it. |
|
🤖 Finished Fix · ✅ Success · Started 12:00 PM UTC · Completed 12:05 PM UTC |
🔧 Fix agent — iteration 2 (human-triggered)Removed the now-unused hack/upload-traces-otelcol-config.yaml config template. The PR already removed all references to it from upload-traces.sh (CONFIG_TEMPLATE variable and file-existence check) and generates the config at runtime instead. No other files reference the template. Fixed (1):
Tests: passed Updated by fullsend fix agent |
The committed upload-traces-otelcol-config.yaml is no longer referenced after the script was updated to generate the Collector config at runtime. Remove the dead file. Addresses review feedback on #5442
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated multi-model review pass (2 independent reviewers completed; a third did not finish in time). Findings below are newly surfaced against the current head commit — the previously-flagged dead CONFIG_TEMPLATE reference and the original YAML-escaping gaps (missing key validation, raw double-quote-only escaping) are already fixed as of the latest commit and are not re-flagged here. A previously-discussed header-value comma-splitting limitation (inherited from the OTEL_EXPORTER_OTLP_HEADERS env-var convention, not a regression from this PR) is likewise not re-flagged. Posting 1 HIGH and 2 MEDIUM findings.
The committed upload-traces-otelcol-config.yaml is no longer referenced after the script was updated to generate the Collector config at runtime. Remove the dead file. Addresses review feedback on #5442 Signed-off-by: Hector Martinez <hemartin@redhat.com>
cb16a3b to
2843acf
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Recheck pass against head 2843acf6 (following up on the prior review round at an earlier commit).
Verified fixed:
- HIGH (cleartext header/secret printing to stdout): confirmed removed. The
echo "headers: $MERGED_HEADERS"/cat "$RUNTIME_CONFIG"dump is gone; the script now prints only a redacted header count. - MEDIUM ($ escaping gap): confirmed fixed.
value="${value//\$/\$\$}"was added, correctly neutralizing the Collector's${env:...}expansion syntax.
Still open in substance (threads already marked resolved on GitHub, so not re-flagged inline below, but noting for visibility):
- MEDIUM premature-decision (otelcol-contrib header-reading behavior asserted as fact): the code comment is unchanged and no end-to-end verification was added to the PR description; the thread's resolution rests on a reply rather than a code/doc change.
- MEDIUM (non-ASCII line-break escaping): only ASCII \n/\r are stripped; Unicode line-separator characters are still unhandled.
New findings surfaced during this recheck are below.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Recheck round 2
Re-reviewed at head 6848df27 (previous recheck was against 2843acf6). Dispatched a 4-agent review squad (independent Claude, Grok, and Codex-backed reviewers) against the current diff; findings below are the deduped, independently cross-verified survivors — each was confirmed by at least 3 of 4 agents and/or reproduced empirically against the real confmap library rather than inferred from reading code alone.
Confirmed resolved since the last recheck:
- Header secret values no longer persist in cleartext in the generated config file — only header keys and internal
REPLAY_HEADER_Nvariable names ever touch disk now. - Duplicate header keys are deduplicated with deterministic last-wins semantics (
--headeroverridesOTEL_EXPORTER_OTLP_HEADERS). - The
mktemptemplate no longer has a suffix afterXXXXXX, so it randomizes correctly on BSD/macOSmktemp(verified directly — the old template returned a literal, non-randomized path on the first call and hard-failed on every call after; the new one doesn't). - The original stdout cleartext leak remains fixed (prints a redacted count only).
Still open / newly found (see inline comments below) — unresolving the premature-decision thread, since the substance persists under the reworded comment:
- The otelcol-contrib header-reading behavior comment now cites a specific version and method, but is still an unreproducible, unbacked assertion — the PR's own Testing checklist wasn't updated and the tool version pin is still floor-only.
- The fix that moved header values out of the YAML file text (into per-header env-var indirection) closed the literal-embedding escaping gap but opened a more serious one: the Collector's own config resolver recursively re-expands
${env:...}/${file:...}-shaped text found inside an already-resolved value, so an unsanitized--header/--endpointvalue can pivot to exfiltrate an unrelated environment variable or local file. Verified empirically against the realconfmaplibrary. - A new bash-4-only construct in this commit hard-crashes the entire header feature on stock macOS bash.
Pass header values via env-var indirection (REPLAY_HEADER_N) instead of writing them as cleartext into the generated YAML config file. This keeps secrets out of the temp file on disk, consistent with how REPLAY_INCLUDE and REPLAY_ENDPOINT are already handled. Also: deduplicate headers by key (last-wins, so --header overrides env), drop .yaml suffix from mktemp template for BSD compatibility, redact header values from stdout, and ground the otelcol-contrib behavior comment in verified test results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
6848df2 to
afacbac
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Recheck round 3
Re-reviewed at head afacbac2 (previous recheck was against 6848df27). Dispatched a 4-agent review squad (independent Claude, Grok, and Codex-backed reviewers) against the current diff, then independently verified the security-relevant claims myself against the real go.opentelemetry.io/collector/confmap source and by direct reproduction.
Verified fixed:
- CRITICAL (confmap recursive re-expansion): genuinely closed.
escape_confmap()(line 107) doubles every literal$to$$beforeREPLAY_INCLUDE,REPLAY_ENDPOINT, and every header value are exported. Verified against confmap's actualfindURIescape-detection logic (an odd run of$immediately before${is required to trigger expansion; doubling a value's dollar signs always produces an even count, which is left as inert literal text) and against the project's own e2e fixture (confmap/internal/e2e/testdata/expand-escaped-env.yaml), which covers exactly this$${env:...}shape and asserts it resolves to the literal string, never the referenced variable. The>=0.120.0floor this script requires postdates the$$-escaping fix (landed 0.105.0, stable 0.107.0, unconditional since 0.109.0), so there's no version gap in the supported range. - HIGH (
declare -Abash 3.2 crash): genuinely closed. No associative arrays remain anywhere in the file; dedup now uses plain indexed arrays with a linear scan, which is bash 3.0+-safe. Confirmed no other bash-4-only syntax was introduced, and confirmed the script runs end-to-end under real stock bash 3.2.57.
Not re-flagged (raised twice, maintainer has explicitly declined both — noting for visibility, not re-litigating): the unverified otelcol-contrib behavior comment, and the bearer-token-shaped usage example.
New findings below — surfaced while tracing the secret data flow end-to-end hunting for a possible 4th regression, independently reproduced, not duplicates of anything raised in prior rounds.
| echo " headers:" >> "$RUNTIME_CONFIG" | ||
| HEADER_KEYS=() | ||
| HEADER_VALS=() | ||
| IFS=',' read -ra HEADER_ARRAY <<< "$MERGED_HEADERS" |
There was a problem hiding this comment.
HIGH — Header/endpoint values containing a literal comma are silently shredded, corrupting secrets and promoting value fragments into header names
File: hack/upload-traces.sh:53, 90-97, 134-152
Finding: --header flags are joined with a literal comma (line 53: HEADERS="${HEADERS},$2") and merged with OTEL_EXPORTER_OTLP_HEADERS the same way (lines 90-97), then split back apart on that same comma (line 134: IFS=',' read -ra HEADER_ARRAY <<< "$MERGED_HEADERS") with no escaping or encoding step in between. Any header value that itself contains a comma is silently torn into multiple bogus header entries with no error or warning.
Reproduced directly: --header 'Authorization=AWS4-HMAC-SHA256 Credential=AKIAEXAMPLE/20260101/us-east-1/s3/aws4_request,SignedHeaders=host,Signature=deadbeef1234' (a realistic SigV4-style value) splits into three separate headers — Authorization (truncated to just the credential fragment), SignedHeaders: host, and Signature: deadbeef1234 — while the script still prints headers: 3 configured (values redacted) as if nothing were wrong.
Each fragment still passes through escape_confmap, so this doesn't reopen the confmap exfiltration issue. But it does silently corrupt the secret the user intended to send (breaking auth against the real endpoint in a confusing way) and it promotes a chunk of what the user considers one secret value into a header name (Signature above), which some proxies/access logs record more liberally than header values — undermining the redaction work already done elsewhere in this PR. This has been present since early in this PR's header-generation logic and hasn't been exercised by any prior review round's test cases.
Suggestion: Don't reuse the same delimiter for joining CLI flags and splitting the merged string. Build --header key/value pairs into a real bash array at parse time (append on each --header occurrence) instead of a comma-joined string, and reserve comma-splitting only for the legacy OTEL_EXPORTER_OTLP_HEADERS env var. Alternatively, validate that every comma-delimited fragment contains exactly one = and die with a clear error otherwise, rather than silently fabricating headers.
| || die "invalid header key (must match [a-zA-Z0-9_-]+): $key" | ||
| found=0 | ||
| for j in "${!HEADER_KEYS[@]}"; do | ||
| if [[ "${HEADER_KEYS[$j]}" == "$key" ]]; then |
There was a problem hiding this comment.
MEDIUM premature-decision — Case-insensitive header-name collisions defeat the "last-wins" dedup guarantee
File: hack/upload-traces.sh:141-147
Finding: The dedup loop added this round compares keys case-sensitively (if [[ "${HEADER_KEYS[$j]}" == "$key" ]], line 142), but the commit introducing it states the guarantee as "deduplicate headers by key (last-wins, so --header overrides env)." HTTP header field names are case-insensitive (RFC 7230 §3.2), so a pre-existing OTEL_EXPORTER_OTLP_HEADERS=authorization=old-token combined with --header Authorization=new-token (a realistic rotate-a-credential scenario) produces two separate YAML entries (authorization and Authorization) instead of the intended override — both get sent to the Collector, including the value the user believed they had replaced. This is an untested assumption stated as a settled guarantee in the commit message, not verified against HTTP's actual case-insensitivity semantics.
Suggestion: Normalize keys for comparison (not necessarily for output) before the equality check at line 142 — e.g. a portable bash-3-safe lowercase via tr '[:upper:]' '[:lower:]', since ${key,,} requires bash 4.
| env_var="REPLAY_HEADER_${i}" | ||
| printf -v "$env_var" '%s' "$(escape_confmap "${HEADER_VALS[$i]}")" | ||
| export "${env_var?}" | ||
| printf ' %s: "${env:%s}"\n' "${HEADER_KEYS[$i]}" "$env_var" >> "$RUNTIME_CONFIG" |
There was a problem hiding this comment.
MEDIUM premature-decision — Whole-value ${env:...} substitution can silently change a header value's type
File: hack/upload-traces.sh:153-158
Finding: Each header value now reaches the Collector only via a whole-value placeholder ("${env:REPLAY_HEADER_N}", line 157) rather than embedded in a larger string. Per the actual envprovider.Retrieve implementation in go.opentelemetry.io/collector/confmap/provider/envprovider, an environment variable's value is passed through confmap.NewRetrievedFromYAML([]byte(val)) — i.e. re-parsed as YAML to determine its type — rather than kept as a plain string. Combined with confmap's documented whole-value-replacement behavior (a field whose entire value is a single ${...} reference takes on the type the provider returns, unlike a partial/embedded reference which always yields a string), a header value that happens to be valid YAML for a non-string type will change type: e.g. true/false/null, a bare integer, or anything shaped like key: value (parsed as a one-entry mapping) instead of the literal text intended for exporters.otlphttp.headers.
Notably, this script's own usage example — --header x-mlflow-experiment-id=42 — is exactly the bare-integer shape that triggers this. Whether the Collector's config decoder silently coerces it back to a string or rejects it at startup is untested either way; nothing in this PR's testing notes considers a header value that isn't already string-shaped.
Suggestion: Add a test case (or at minimum a note) confirming the actual behavior for a numeric-looking or key: value-shaped header value against a real otelcol-contrib. If it doesn't round-trip cleanly, consider a way to force string type at the YAML level so confmap can't infer a different type from the substituted content — the whole-value indirection introduced to fix the earlier disk-persistence/CRITICAL findings is exactly the change that created this exposure.
2 participants
Summary
Fix
--header key=valueflags being silently ignored byotelcol-contribinhack/upload-traces.sh. The script was settingOTEL_EXPORTER_OTLP_HEADERS(an OpenTelemetry SDK env var), but the Collector only reads headers from its YAML config underexporters.otlphttp.headers.Related Issue
Fixes #5436
Changes
--headervalues as YAML key-value pairs underexporters.otlphttp.headers--headerflags with any pre-existingOTEL_EXPORTER_OTLP_HEADERSenv var for backwards compatibilitytrapTesting
make lintcould not run in sandbox (pre-commit network restriction) — will be verified by CIChecklist
!for breaking changes)Closes #5436
Post-script verification
agent/5436-fix-header-passthrough)675086c5e606fe1ea9c09c22e1aa617aa17afeae..HEAD)