Skip to content

fix: [processor/signingprocessor] scalar type collision - #50892

Open
hilmarf wants to merge 4 commits into
open-telemetry:mainfrom
apeirora:scalar_type_collision
Open

fix: [processor/signingprocessor] scalar type collision#50892
hilmarf wants to merge 4 commits into
open-telemetry:mainfrom
apeirora:scalar_type_collision

Conversation

@hilmarf

@hilmarf hilmarf commented Sep 10, 2026

Copy link
Copy Markdown
Member

Description

PutInt("k", 123) and PutStr("k", "123") produced identical canonical JSON, so one signature covered both values. Each scalar type is now wrapped in a single-key object keyed by type name (matching OTLP/JSON conventions), making every type distinct.

Existing signatures are invalidated by this change, but the component is in development state.

SeverityNumber and SeverityText are no longer included in the signed payload; the audit logging spec marks them as SHOULD NOT.

Link to tracking issue

follow up on: #50884

Testing

added new TestScalarTypeCollision

Documentation

README.md updated

Authorship

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

@hilmarf
hilmarf requested review from a team and jmacd as code owners September 10, 2026 13:25
@hilmarf

hilmarf commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

@surpradhan what do you think? That should close the scalar collision gap, right? It's a follower of #50884

@surpradhan

surpradhan commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Yes @hilmarf, that closes it. I ran the pairs against the branch and they all separate now:

int 123 / str "123"        distinct
int -5  / str "-5"         distinct
bytes 0xDEAD / str "3q0="  distinct
int 123 / double 123.0     distinct
[int 7] / [str "7"]        distinct

One non-blocking note : TestScalarTypeCollision covers the four scalar pairs but not negatives or containers. PutInt(-5) vs PutStr("-5"), and a slice of int 7 vs a slice of str "7", would pin the recursion path, which is where I would expect a future refactor to regress it.

@opentelemetry-pr-dashboard

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-09-11 07:01 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.

Signed-off-by: Hilmar Falkenberg <hilmar.falkenberg@sap.com>
Signed-off-by: Hilmar Falkenberg <hilmar.falkenberg@sap.com>
Signed-off-by: Hilmar Falkenberg <hilmar.falkenberg@sap.com>
Signed-off-by: Hilmar Falkenberg <hilmar.falkenberg@sap.com>
@hilmarf
hilmarf force-pushed the scalar_type_collision branch from 80e4be1 to 7408cf7 Compare September 11, 2026 07:38
@hilmarf

hilmarf commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

One non-blocking note : TestScalarTypeCollision covers the four scalar pairs but not negatives or containers. PutInt(-5) vs PutStr("-5"), and a slice of int 7 vs a slice of str "7", would pin the recursion path, which is where I would expect a future refactor to regress it.

addressed in 7408cf7

@hilmarf

hilmarf commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

@surpradhan other edge cases I should cover? If not, please approve the PR. Thanks a lot for your valuable inputs. It's good to have someone else, being familiar with the pitfalls of canonization.

@surpradhan

Copy link
Copy Markdown
Contributor

@hilmarf, I just ran them plus a few more, it looks right.

One pair left: -0.0 and +0.0 both come out as {"doubleValue":0}, so they share a signature. That's RFC 8785 doing what Appendix B says rather than the library misbehaving, so you may well decide if it isn't worth chasing. It's just the last colliding pair I can find.

Approving. Enjoyed poking at this one.

songy23 pushed a commit that referenced this pull request Sep 11, 2026
…#50919)

**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.

- [x] I, a human, wrote this pull request description myself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants