Skip to content

fix: [processor/signingprocessor] sign the log body whatever its type - #50919

Merged
songy23 merged 1 commit into
open-telemetry:mainfrom
surpradhan:surabhi/sign-non-string-body
Sep 11, 2026
Merged

fix: [processor/signingprocessor] sign the log body whatever its type#50919
songy23 merged 1 commit into
open-telemetry:mainfrom
surpradhan:surabhi/sign-non-string-body

Conversation

@surpradhan

@surpradhan surpradhan commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

  • serializeLogRecord only signed string bodies; all other body types were dropped.
  • That meant a structured-body record and a no-body record could produce the same canonical bytes and verify against each other’s signature.
  • The fix uses valueToInterface for any non-empty body, matching attribute handling and inheriting the same UTF-8 and depth checks.
  • Unset bodies are still omitted, so no-body records are unchanged.
  • Canonical bytes now change for non-string bodies, so existing signatures for those records won’t reproduce. Hence breaking.

Note: bodies now inherit the same scalar ambiguity/int64 precision issue as attributes; #50892 fixes that too.

Tracking issue

Fixes #50911

Testing

covered all body types, distinct structured bodies, no-body behavior, UTF-8/depth validation, and full sign/verify for a map body. The existing int-body test only checked for a non-empty payload, which is why this slipped through.

Documentation

main already lists body without a string-only restriction, so this fix aligns the implementation with the README. The (string only) wording comes from #50892.

  • I, a human, wrote this pull request description myself.

@surpradhan
surpradhan requested review from a team and jmacd as code owners September 11, 2026 11:45
@github-actions github-actions Bot added first-time contributor PRs made by new contributors processor/signing labels Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib.

Important reminders:

  • Read our Contributing Guidelines.
  • Sign the CLA if you haven't already.
  • Follow the OpenTelemetry Generative AI policy: disclose any AI use in your contribution, and communicate (PR descriptions, review replies) in your own words rather than AI-generated text.
  • Give reviewers at least a few days before pinging them for feedback.
  • If you need help with general setup, development process, or contributor etiquette, ask in #opentelemetry-new-contributors.
  • First-time contributors should keep at most one PR marked as ready for review until their first PR is merged; once two are open for review, any further PR is automatically converted to draft.
  • If your change isn't one of our priority components, reviews may take more time.
  • Raise technical or Collector-specific questions in #otel-collector-dev or a Collector SIG meeting.

@github-actions
github-actions Bot requested a review from hilmarf September 11, 2026 11:46
@surpradhan
surpradhan force-pushed the surabhi/sign-non-string-body branch from 7375412 to 7f03a0e Compare September 11, 2026 11:55
@singhvibhanshu

singhvibhanshu commented Sep 11, 2026

Copy link
Copy Markdown
Member

/workflow-approve

@surpradhan surpradhan mentioned this pull request Sep 11, 2026
Closed
@surpradhan
fix: [processor/signingprocessor] sign the log body whatever its type
325ddf2 
serializeLogRecord included the body only when it was a string, so every
other type was dropped before signing and was not covered by the signature
at all. A record with a structured body and a record with no body produced
identical canonical bytes and each verified against the other's signature.

The body now goes through the same valueToInterface conversion as
attributes, which handles every pcommon.Value type, enforces the existing
nesting depth cap, and preserves the UTF-8 validation that guarded the
string-only path. An unset body is still omitted rather than encoded as
null, so records without one are unaffected.

verifyRecord independently re-derives the signed payload and still handled
only string bodies, so it could not verify anything this change now signs;
it gains a standalone converter rather than reusing valueToInterface, so it
stays a genuine re-derivation. TestSignVerifyStructuredBody drives a map
body through the full sign-then-verify path so the mirror cannot rot again.

TestSerializeLogRecordNonStringBody already set an int body but only
asserted the payload was non-empty, which is why this went unnoticed; it
now checks the body is actually present.

The nesting-depth and UTF-8 errors inside valueToInterface said "attribute",
which read as a contradiction once a body could reach them.

Fixes open-telemetry#50911

Assisted-by: Claude Opus 5
@surpradhan
surpradhan force-pushed the surabhi/sign-non-string-body branch from 7f03a0e to 325ddf2 Compare September 11, 2026 12:22
@singhvibhanshu

singhvibhanshu commented Sep 11, 2026

Copy link
Copy Markdown
Member

/workflow-approve

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-09-11 13:00 UTC

Review the latest changes.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@singhvibhanshu

singhvibhanshu commented Sep 11, 2026

Copy link
Copy Markdown
Member

This PR got approved by the code-owner. Could someone from @open-telemetry/collector-contrib-approvers please take a look at it?
Thanks!

@paulojmdias paulojmdias added the ready to merge Code review completed; ready to merge by maintainers label Sep 11, 2026
@singhvibhanshu

singhvibhanshu commented Sep 11, 2026

Copy link
Copy Markdown
Member

/workflow-approve

@songy23
songy23 merged commit 87f6885 into open-telemetry:main Sep 11, 2026
178 checks passed
@otelbot

otelbot Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution @surpradhan! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

@surpradhan

surpradhan commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @hilmarf @paulojmdias @singhvibhanshu and @songy23.

hilmarf added a commit to apeirora/opentelemetry-collector-contrib that referenced this pull request Sep 14, 2026
@hilmarf
adjust tests after merge of open-telemetry#50919
8322f70 
Signed-off-by: Hilmar Falkenberg <hilmar.falkenberg@sap.com>
hilmarf added a commit to apeirora/opentelemetry-collector-contrib that referenced this pull request Sep 14, 2026
@hilmarf
adjust tests after merge of open-telemetry#50919
fee6a81 
Signed-off-by: Hilmar Falkenberg <hilmar.falkenberg@sap.com>
hilmarf added a commit to apeirora/opentelemetry-collector-contrib that referenced this pull request Sep 14, 2026
@hilmarf
adjust tests after merge of open-telemetry#50919
c89e4e0 
Signed-off-by: Hilmar Falkenberg <hilmar.falkenberg@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulojmdias paulojmdias paulojmdias approved these changes
@jmacd jmacd Awaiting requested review from jmacd jmacd is a code owner
+2 more reviewers
@hilmarf hilmarf hilmarf approved these changes
@singhvibhanshu singhvibhanshu singhvibhanshu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

first-time contributor PRs made by new contributors processor/signing ready to merge Code review completed; ready to merge by maintainers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[processor/signingprocessor] a non-string log body is omitted from the signed payload entirely

5 participants

@surpradhan @singhvibhanshu @hilmarf @paulojmdias @songy23