Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[baggage] validate key and values during baggage injection/extraction #5647

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

## Unreleased

* **Breaking change:** Do not encode/decode baggage item key during injection/extraction.
Validate baggage item key meets `token` requirement as defined in
[W3C Baggage propagation format specification](https://www.w3.org/TR/baggage/).
Reject all baggage item values if any of them is invalid during extraction.
([#5647](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5647))
* The experimental APIs previously covered by `OTEL1000` (`LoggerProvider`,
`LoggerProviderBuilder`, & `IDeferredLoggerProviderBuilder`) will now be part
of the public API and supported in stable builds.
Expand Down
157 changes: 143 additions & 14 deletions src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -84,20 +84,35 @@ public override void Inject<T>(PropagationContext context, T carrier, Action<T,
if (e.MoveNext() == true)
{
int itemCount = 0;
StringBuilder baggage = new StringBuilder();
StringBuilder baggage = null;
do
{
KeyValuePair<string, string> item = e.Current;
if (string.IsNullOrEmpty(item.Value))

if (!ValidateKey(item.Key))
{
continue;
}

baggage.Append(Uri.EscapeDataString(item.Key)).Append('=').Append(Uri.EscapeDataString(item.Value)).Append(',');
if (baggage == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need to differentiate null vs empty? This would get checked in every iteration of the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a need to differentiate between these, I will modify it.

{
baggage = new StringBuilder();
}

baggage.Append(item.Key).Append('=').Append(Uri.EscapeDataString(item.Value)).Append(',');
itemCount++;
if (baggage.Length >= MaxBaggageLength)
{
break;
}
}
while (e.MoveNext() && itemCount < MaxBaggageItems);

if (baggage is not null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the expected behavior if baggage is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Remove(baggage.Length - 1, 1);
setter(carrier, BaggageHeaderName, baggage.ToString());
}
while (e.MoveNext() && ++itemCount < MaxBaggageItems && baggage.Length < MaxBaggageLength);
baggage.Remove(baggage.Length - 1, 1);
setter(carrier, BaggageHeaderName, baggage.ToString());
}
}

Expand All @@ -116,7 +131,8 @@ internal static bool TryExtractBaggage(string[] baggageCollection, out Dictionar

if (string.IsNullOrEmpty(item))
{
continue;
baggage = null;
return false;
Comment on lines +134 to +135
Copy link
Contributor

@xiang17 xiang17 Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lachmatt lachmatt Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. They are indeed different on whether allowing list-member to be OWS.

}

foreach (var pair in item.Split(CommaSignSeparator))
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List-member could contain multiple = signs. For example,

  1. in property.

list-member = key OWS "=" OWS value *( OWS ";" OWS property )
property = key OWS "=" OWS value
property =/ key OWS

  1. A value can also contain equal signs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lachmatt lachmatt Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
continue;
baggage = null;
return false;
}

var key = Uri.UnescapeDataString(parts[0]);
var value = Uri.UnescapeDataString(parts[1]);
var key = parts[0].Trim();

if (string.IsNullOrEmpty(key) || string.IsNullOrEmpty(value))
if (!ValidateKey(key))
{
continue;
baggage = null;
return false;
}

var encodedValue = parts[1].Trim();

if (!ValidateValue(encodedValue))
{
baggage = null;
return false;
}

var decodedValue = Uri.UnescapeDataString(encodedValue);

if (baggageDictionary == null)
{
baggageDictionary = new Dictionary<string, string>();
}

baggageDictionary[key] = value;
baggageDictionary[key] = decodedValue;
}
}

baggage = baggageDictionary;
return baggageDictionary != null;
}

private static bool ValidateValue(string encodedValue)
{
var index = 0;
while (index < encodedValue.Length)
{
var c = encodedValue[index];

if (c == '%')
{
if (!ValidatePercentEncoding(index, encodedValue))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{
OpenTelemetryApiEventSource.Log.BaggageItemValueIsInvalid(encodedValue);
return false;
}

index += 3;
}
else if (!ValidateValueCharInRange(c))
{
OpenTelemetryApiEventSource.Log.BaggageItemValueIsInvalid(encodedValue);
return false;
}
else
{
index++;
}
}

return true;
}

private static bool ValidatePercentEncoding(int index, string encodedValue)
{
return index < encodedValue.Length - 2 &&
ValidateHexChar(encodedValue[index + 1]) &&
ValidateHexChar(encodedValue[index + 2]);
}

private static bool ValidateHexChar(char c)
{
return c is
>= '0' and <= '9' or
>= 'A' and <= 'F' or
>= 'a' and <= 'f';
}

private static bool ValidateValueCharInRange(char c)
{
// https://w3c.github.io/baggage/#definition
// value = *baggage-octet
// baggage-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
// ; US-ASCII characters excluding CTLs,
// ; whitespace, DQUOTE, comma, semicolon,
// ; and backslash
return c is
'\u0021' or
>= '\u0023' and <= '\u002b' or
>= '\u002d' and <= '\u003a' or
>= '\u003c' and <= '\u005b' or
>= '\u005d' and <= '\u007e';
}

private static bool ValidateKey(string key)
{
if (string.IsNullOrEmpty(key))
{
return false;
}

foreach (var c in key)
{
// chars permitted in token, as defined in:
// https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6
if (!ValidateTokenChar(c) && !ValidateAsciiLetterOrDigit(c))
{
OpenTelemetryApiEventSource.Log.BaggageItemKeyIsInvalid(key);
return false;
}
}

return true;
}

private static bool ValidateTokenChar(char c)
{
// https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6
// token = 1*tchar
// tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*"
// / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the code logic is correct but tchar definition also contains / DIGIT / ALPHA which you split into another method.

return c is '!' or '#' or '$' or '%' or '&' or '\'' or '*'
or '+' or '-' or '.' or '^' or '_' or '`' or '|' or '~';
}

private static bool ValidateAsciiLetterOrDigit(char c)
{
return c is
>= '0' and <= '9' or
>= 'A' and <= 'Z' or
>= 'a' and <= 'z';
}
}
12 changes: 12 additions & 0 deletions src/OpenTelemetry.Api/Internal/OpenTelemetryApiEventSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,16 @@ public void FailedToInjectBaggage(string format, string error)
{
this.WriteEvent(11, format, error);
}

[Event(12, Message = "Baggage item key is invalid, key = '{0}'", Level = EventLevel.Warning)]
public void BaggageItemKeyIsInvalid(string key)
{
this.WriteEvent(12, key);
}

[Event(13, Message = "Baggage item value is invalid, value = '{0}'", Level = EventLevel.Warning)]
public void BaggageItemValueIsInvalid(string value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Should we inform what was the key for the given value? It might be useful for debugging.

{
this.WriteEvent(13, value);
}
}
Loading