[baggage] validate key and values during baggage injection/extraction #5647
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5647 +/- ##
==========================================
+ Coverage 83.38% 86.01% +2.63%
==========================================
Files 297 254 -43
Lines 12531 11114 -1417
==========================================
- Hits 10449 9560 -889
+ Misses 2082 1554 -528
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| } | ||
|
|
||
| [Event(13, Message = "Baggage item value is invalid, value = '{0}'", Level = EventLevel.Warning)] | ||
| public void BaggageItemValueIsInvalid(string value) |
There was a problem hiding this comment.
Q: Should we inform what was the key for the given value? It might be useful for debugging.
|
@lachmatt - Could you please clarify where these requirements are coming from?
|
This is not a requirement, but a choice made during implementation (I'm open for suggestions how to improve it). For the injection part , Baggage API allows arbitrary strings as names. This is less restrictive than For the extraction part, current behavior is consistent with e.g. how invalid items are handled during As far as I know, invalid values are handled differently by different language implementations, e.g.:
|
@lachmatt - Shouldn't this be coming from spec? Looking at the spec
This seems to suggest it is not a MUST? Also, looking at this https://www.w3.org/TR/baggage/#mutating-baggage. Looks like there is already a suggested approach for values.
But a different approach is proposed here? |
|
@vishweshbankwar I created an issue in the spec |
I think the spec says to remove that (baggage) entry if conditions are met, rather than dropping the entire baggage.
|
| } | ||
| while (e.MoveNext() && itemCount < MaxBaggageItems); | ||
|
|
||
| if (baggage is not null) |
There was a problem hiding this comment.
What's the expected behavior if baggage is null?
There was a problem hiding this comment.
baggage is null only if there are no valid entries in a baggage, in which case nothing will be injected (i.e setter won't be called). The same happens for empty baggage.
| baggage = null; | ||
| return false; |
There was a problem hiding this comment.
It feels very strict to ignore the entire baggage collection if any entry is invalid, or in this case an entry is empty. I'm not so familiar with the baggage spec, but the tracestate spec specifically calls out that empty list members are allowed:
Empty and whitespace-only list members are allowed. Vendors MUST accept empty tracestate headers but SHOULD avoid sending them. Empty list members are allowed in tracestate because it is difficult for a vendor to recognize the empty value when multiple tracestate headers are sent. Whitespace characters are allowed for a similar reason, as some vendors automatically inject whitespace after a comma separator, even in the case of an empty header.
There was a problem hiding this comment.
According to W3C Baggage spec empty entries are invalid - entry needs to at least contain a key satisfying a token requirement (note at least one char from tchar range) followed by a =.
Also note that current implementation of tracestate extraction ignores all of the entries if any of them is invalid (i.e behaves similarly to what is proposed in this PR)
There was a problem hiding this comment.
I see. They are indeed different on whether allowing list-member to be OWS.
| { | ||
| continue; | ||
| } | ||
|
|
||
| baggage.Append(Uri.EscapeDataString(item.Key)).Append('=').Append(Uri.EscapeDataString(item.Value)).Append(','); | ||
| if (baggage == null) |
There was a problem hiding this comment.
Is there a need to differentiate null vs empty? This would get checked in every iteration of the loop.
There was a problem hiding this comment.
There isn't a need to differentiate between these, I will modify it.
| // https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6 | ||
| // token = 1*tchar | ||
| // tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" | ||
| // / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" |
There was a problem hiding this comment.
nit: the code logic is correct but tchar definition also contains / DIGIT / ALPHA which you split into another method.
|
|
||
| if (c == '%') | ||
| { | ||
| if (!ValidatePercentEncoding(index, encodedValue)) |
There was a problem hiding this comment.
What if the % character is part of the literal text, i.e. not used as the special character to indicate the following two characters are a percent-encoding?
Relevant spec:
Any code points outside of the baggage-octet range MUST be percent-encoded. The percent code point (U+0025) MUST be percent-encoded. Code points which are not required to be percent-encoded MAY be percent-encoded.
For example, %%, %OK, %3Q or % at the end of string.
There was a problem hiding this comment.
I've realized this is assuming the string is encoded value. You can ignore above comment's case.
However, the next if branch checks ValidateValueCharInRange which is assuming the string is already decoded. The two different assumptions here smells not good. i.e. Could there be a char that's invalid to be a baggage-octet but won't be detected here by ValidateValueCharInRange?
| @@ -131,33 +147,146 @@ internal static bool TryExtractBaggage(string[] baggageCollection, out Dictionar | |||
|
|
|||
| if (pair.IndexOf('=') < 0) | |||
| { | |||
| continue; | |||
| baggage = null; | |||
| return false; | |||
| } | |||
|
|
|||
| var parts = pair.Split(EqualSignSeparator, 2); | |||
| if (parts.Length != 2) | |||
There was a problem hiding this comment.
List-member could contain multiple = signs. For example,
- in property.
list-member = key OWS "=" OWS value *( OWS ";" OWS property )
property = key OWS "=" OWS value
property =/ key OWS
There was a problem hiding this comment.
Ad 1. Properties are not handled/recognized properly currently, this is something that needs to be addressed, but as it requires changes in both Baggage API and propagator, I'd prefer to address it in a separate PR.
Ad 2. This will be handled properly - see test case added in 6b88989
There was a problem hiding this comment.
I created issue to track handling of properties.
Currently, properties attached to a baggage entry are treated as a part of a value.
With baggage entry value validation added in this PR (ValidateValue), such entries will be considered invalid due to the existence of ; char. This might need to be adjusted, based on how we decide to proceed with the fixes (e.g separate PR for handling properties)
| [InlineData("key=%gg")] | ||
| [InlineData("key=val%2")] | ||
| [InlineData("key=val ue")] | ||
| [InlineData("key=val%IJK")] |
There was a problem hiding this comment.
Add a case which has valid percent coding triplets, but invalid as value in list-member once URI escaped?
Kielek
added
the
pkg:OpenTelemetry.Api
Fixes #5479
Design discussion issue #
Changes
tokenrequirement from the spec insteadNOTE:
Amount of allocations (e.g in extract part) can be reduced, I'd prefer to address it in a separate PR.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes