From 065f4d6cd804ffe063a186cfd73cf39d81f0f15d Mon Sep 17 00:00:00 2001 From: Mateusz Lach Date: Thu, 23 May 2024 10:52:37 +0200 Subject: [PATCH 1/3] validate key and values during baggage injection/extraction --- .../Context/Propagation/BaggagePropagator.cs | 157 ++++++++++++++++-- .../Internal/OpenTelemetryApiEventSource.cs | 12 ++ .../Propagation/BaggagePropagatorTest.cs | 140 ++++++++++++---- 3 files changed, 265 insertions(+), 44 deletions(-) diff --git a/src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs b/src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs index e9dc29d950d..902bd31737c 100644 --- a/src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs +++ b/src/OpenTelemetry.Api/Context/Propagation/BaggagePropagator.cs @@ -84,20 +84,35 @@ public override void Inject(PropagationContext context, T carrier, Action 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) + { + 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) + { + 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()); } } @@ -116,7 +131,8 @@ internal static bool TryExtractBaggage(string[] baggageCollection, out Dictionar if (string.IsNullOrEmpty(item)) { - continue; + baggage = null; + return false; } foreach (var pair in item.Split(CommaSignSeparator)) @@ -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) { - 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(); } - 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)) + { + 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 = "!" / "#" / "$" / "%" / "&" / "'" / "*" + // / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" + 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'; + } } diff --git a/src/OpenTelemetry.Api/Internal/OpenTelemetryApiEventSource.cs b/src/OpenTelemetry.Api/Internal/OpenTelemetryApiEventSource.cs index ea802a6ed15..1a468cfd59a 100644 --- a/src/OpenTelemetry.Api/Internal/OpenTelemetryApiEventSource.cs +++ b/src/OpenTelemetry.Api/Internal/OpenTelemetryApiEventSource.cs @@ -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) + { + this.WriteEvent(13, value); + } } diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs b/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs index 9d8a8972586..bfff6ffcd87 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs @@ -1,7 +1,6 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -using System.Net; using Xunit; namespace OpenTelemetry.Context.Propagation.Tests; @@ -118,25 +117,40 @@ public void ValidateLongBaggageExtraction() Assert.Equal(new string('x', 8186), array[0].Value); } - [Fact] - public void ValidateSpecialCharsBaggageExtraction() + [Theory] + [InlineData("key%201=value%201", "key%201", "value 1")] + [InlineData("key=val+ue", "key", "val+ue")] + [InlineData("key=val%2Bue", "key", "val+ue")] + [InlineData("key=val%20ue", "key", "val ue")] + [InlineData("key=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~", "key", " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~")] + public void ValidateSpecialCharsBaggageExtraction(string propagatedBaggage, string key, string expectedDecodedValue) { - var encodedKey = WebUtility.UrlEncode("key2"); - var encodedValue = WebUtility.UrlEncode("!x_x,x-x&x(x\");:"); - var escapedKey = Uri.EscapeDataString("key()3"); - var escapedValue = Uri.EscapeDataString("value()!&;:"); + var carrier = new List> + { + new(BaggagePropagator.BaggageHeaderName, propagatedBaggage), + }; + + var propagationContext = this.baggage.Extract(default, carrier, GetterList); + + Assert.False(propagationContext == default); + Assert.True(propagationContext.ActivityContext == default); - Assert.Equal("key2", encodedKey); - Assert.Equal("!x_x%2Cx-x%26x(x%22)%3B%3A", encodedValue); - Assert.Equal("key%28%293", escapedKey); - Assert.Equal("value%28%29%21%26%3B%3A", escapedValue); + Assert.Equal(1, propagationContext.Baggage.Count); - var initialBaggage = - $"key%201=value%201,{encodedKey}={encodedValue},{escapedKey}={escapedValue},key4=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~"; + var actualBaggage = propagationContext.Baggage.GetBaggage(); + + Assert.Single(actualBaggage); + Assert.Contains(key, actualBaggage); + Assert.Equal(expectedDecodedValue, actualBaggage[key]); + } + + [Fact] + public void ValidateBaggageMultipleItemsExtraction() + { var carrier = new List> { - new KeyValuePair(BaggagePropagator.BaggageHeaderName, initialBaggage), + new(BaggagePropagator.BaggageHeaderName, " key1 =val1,key2= val2 ,key3="), }; var propagationContext = this.baggage.Extract(default, carrier, GetterList); @@ -144,24 +158,59 @@ public void ValidateSpecialCharsBaggageExtraction() Assert.False(propagationContext == default); Assert.True(propagationContext.ActivityContext == default); - Assert.Equal(4, propagationContext.Baggage.Count); - var actualBaggage = propagationContext.Baggage.GetBaggage(); - Assert.Equal(4, actualBaggage.Count); + Assert.Equal(3, actualBaggage.Count); - Assert.True(actualBaggage.ContainsKey("key 1")); - Assert.Equal("value 1", actualBaggage["key 1"]); + Assert.True(actualBaggage.ContainsKey("key1")); + Assert.Equal("val1", actualBaggage["key1"]); Assert.True(actualBaggage.ContainsKey("key2")); - Assert.Equal("!x_x,x-x&x(x\");:", actualBaggage["key2"]); + Assert.Equal("val2", actualBaggage["key2"]); - Assert.True(actualBaggage.ContainsKey("key()3")); - Assert.Equal("value()!&;:", actualBaggage["key()3"]); + Assert.True(actualBaggage.ContainsKey("key3")); + Assert.Equal(string.Empty, actualBaggage["key3"]); + } - // x20-x7E range - Assert.True(actualBaggage.ContainsKey("key4")); - Assert.Equal(" !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~", actualBaggage["key4"]); + [Theory] + [InlineData("key(=value")] + [InlineData(@"key=\\")] + [InlineData("key=val%")] + [InlineData("key=%gg")] + [InlineData("key=val%2")] + [InlineData("key=val ue")] + [InlineData("key=val%IJK")] + public void ValidateInvalidPairsBaggageExtraction(string propagatedBaggage) + { + var carrier = new List> + { + new(BaggagePropagator.BaggageHeaderName, propagatedBaggage), + }; + + var propagationContext = this.baggage.Extract(default, carrier, GetterList); + + Assert.True(propagationContext.ActivityContext == default); + + var actualBaggage = propagationContext.Baggage.GetBaggage(); + + Assert.Empty(actualBaggage); + } + + [Fact] + public void ValidateAllValuesAreRejectedIfAnyIsInvalidDuringExtraction() + { + var carrier = new List> + { + new(BaggagePropagator.BaggageHeaderName, "key1=val1,key2=%2,key3=val3"), + }; + + var propagationContext = this.baggage.Extract(default, carrier, GetterList); + + Assert.True(propagationContext.ActivityContext == default); + + var actualBaggage = propagationContext.Baggage.GetBaggage(); + + Assert.Empty(actualBaggage); } [Fact] @@ -174,7 +223,7 @@ public void ValidateEmptyBaggageInjection() } [Fact] - public void ValidateBaggageInjection() + public void ValidateMultipleItemsBaggageInjection() { var carrier = new Dictionary(); var propagationContext = new PropagationContext( @@ -191,6 +240,40 @@ public void ValidateBaggageInjection() Assert.Equal("key1=value1,key2=value2", carrier[BaggagePropagator.BaggageHeaderName]); } + [Fact] + public void ValidateOnlyInvalidItemsAreRejectedDuringInjection() + { + var carrier = new Dictionary(); + var propagationContext = new PropagationContext( + default, + new Baggage(new Dictionary + { + { "key1", "value1" }, + { "key 2", "value2" }, + })); + + this.baggage.Inject(propagationContext, carrier, Setter); + + Assert.Single(carrier); + Assert.Equal("key1=value1", carrier[BaggagePropagator.BaggageHeaderName]); + } + + [Theory] + [InlineData("key 1", "value 1")] + [InlineData("", "value3")] + public void ValidateInvalidPairsBaggageInjection(string key, string value) + { + var carrier = new Dictionary(); + var propagationContext = new PropagationContext( + default, + new Baggage(new Dictionary + { + { key, value }, + })); + this.baggage.Inject(propagationContext, carrier, Setter); + Assert.Empty(carrier); + } + [Fact] public void ValidateSpecialCharsBaggageInjection() { @@ -199,16 +282,13 @@ public void ValidateSpecialCharsBaggageInjection() default, new Baggage(new Dictionary { - { "key 1", "value 1" }, { "key2", "!x_x,x-x&x(x\");:" }, - - // x20-x7E range { "key3", " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~" }, })); this.baggage.Inject(propagationContext, carrier, Setter); Assert.Single(carrier); - Assert.Equal("key%201=value%201,key2=%21x_x%2Cx-x%26x%28x%22%29%3B%3A,key3=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~", carrier[BaggagePropagator.BaggageHeaderName]); + Assert.Equal("key2=%21x_x%2Cx-x%26x%28x%22%29%3B%3A,key3=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~", carrier[BaggagePropagator.BaggageHeaderName]); } } From b557142e9f1ba66c80a0d4dae27d243113af0402 Mon Sep 17 00:00:00 2001 From: Mateusz Lach Date: Thu, 23 May 2024 11:30:46 +0200 Subject: [PATCH 2/3] changelog update --- src/OpenTelemetry.Api/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index 9bdc9bf5d16..8e41d38a679 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/src/OpenTelemetry.Api/CHANGELOG.md @@ -2,6 +2,12 @@ ## 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)) + ## 1.9.0-alpha.1 Released 2024-May-20 From 6b88989ce3243593c4db0beea6774f039ea4222e Mon Sep 17 00:00:00 2001 From: Mateusz Lach Date: Fri, 7 Jun 2024 12:46:05 +0200 Subject: [PATCH 3/3] additional test case for value containing equal sign --- .../Trace/Propagation/BaggagePropagatorTest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs b/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs index bfff6ffcd87..10f388e4ee7 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/Propagation/BaggagePropagatorTest.cs @@ -122,6 +122,7 @@ public void ValidateLongBaggageExtraction() [InlineData("key=val+ue", "key", "val+ue")] [InlineData("key=val%2Bue", "key", "val+ue")] [InlineData("key=val%20ue", "key", "val ue")] + [InlineData("key=value=1", "key", "value=1")] [InlineData("key=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~", "key", " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~")] public void ValidateSpecialCharsBaggageExtraction(string propagatedBaggage, string key, string expectedDecodedValue) {