E2E tests for different header encodings #760

ManickaP · 2021-02-18T11:55:53Z

All is available only for .NET 5, there are no equivalents for .NET Core 3.1 AFAIK.

1. Proxying request with UTF8/Latin1 header values

1.a. Proxy server needs: `kestrel.RequestHeaderEncodingSelector`:

reverse-proxy/test/ReverseProxy.FunctionalTests/Common/TestEnvironment.cs

Lines 134 to 137 in 6670731

    
                                       if (headerEncoding != null) 
        
                                       { 
        
                                           kestrel.RequestHeaderEncodingSelector = _ => headerEncoding; 
        
                                       }

This can be configured manually in ConfigureWebHost.

1.b. Proxy client needs: `RequestHeaderEncodingSelector`:

reverse-proxy/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs

Line 250 in 6670731

RequestHeaderEncodingSelector = (_, _) => _encoding,

This can be achieved in custom IProxyHttpClientFactory implementation, however the default implementation contains non-trivial amount of code and is internal, which leads to copy-pasting quite a bit of code.

We could also add configuration for request header encoding, which can be per header name, and we would ensure that receiving server and sending client are in sync and expect the same encoding. However, that would need some more effort.

2. Proxying back response with UTF8/Latin1 header values

2.a. Proxy client needs: `ResponseHeaderEncodingSelector`:

reverse-proxy/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs

Line 251 in 6670731

ResponseHeaderEncodingSelector = (_, _) => _encoding

Achievable the same way as 1.b.

2.b. Proxy server fails with: `System.InvalidOperationException: Invalid non-ASCII or control character in header`

I haven't found RequestHeaderEncodingSelector equivalent for response headers. I don't know if there's a way to do this atm.

We could use transforms to encode the header value in ASCII characters as this RFC describes, but I don't know if clients in general can handle that on its own or the consumer would need to parse it manually.

cc: @Tratcher

Contributes to #154

Tratcher · 2021-02-18T17:35:19Z

Response headers (not supported yet): dotnet/aspnetcore#26334, #440

Tratcher

Only style comments so far.

test/ReverseProxy.FunctionalTests/Common/TestEnvironment.cs

test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs

ManickaP · 2021-02-19T18:26:46Z

Few error handling issues discovered with this:

In case we receive Latin1 encoded request header and kestrel is not configured to receive it (i.e. no kestrel.RequestHeaderEncodingSelector = _ => Encoding.Latin1;) the server will return 400 and neither kestrel's IExceptionHandlerFeature nor proxy's IProxyErrorFeature are used.
In case an outgoing request construction fails, IProxyErrorFeature stays empty and only IExceptionHandlerFeature can be used to get the error details. Stack trace in this issue: Destination address is not validated. #765
In case an incoming response fails to be copied to kestrel, IProxyErrorFeature stays empty an only IExceptionHandlerFeature can be used to get the error details. Stack trace:

System.InvalidOperationException: Invalid non-ASCII or control character in header: 0x00E7
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpHeaders.ThrowInvalidHeaderCharacter(Char ch)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpHeaders.ValidateHeaderValueCharacters(StringValues headerValues)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpResponseHeaders.SetValueFast(String key, StringValues value)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpHeaders.Microsoft.AspNetCore.Http.IHeaderDictionary.set_Item(String key, StringValues value)
   at Microsoft.AspNetCore.Http.ParsingHelpers.SetHeaderUnmodified(IHeaderDictionary headers, String key, Nullable`1 values)
   at Microsoft.AspNetCore.Http.ParsingHelpers.AppendHeaderUnmodified(IHeaderDictionary headers, String key, StringValues values)
   at Microsoft.AspNetCore.Http.HeaderDictionaryExtensions.Append(IHeaderDictionary headers, String key, StringValues value)
   at Microsoft.ReverseProxy.Service.Proxy.HttpTransformer.CopyResponseHeaders(HttpContext httpContext, HttpHeaders source, IHeaderDictionary destination) in /home/manicka/Repositories/reverse-proxy.2/src/ReverseProxy/Service/Proxy/HttpTransformer.cs:line 115
   at Microsoft.ReverseProxy.Service.Proxy.HttpTransformer.TransformResponseAsync(HttpContext httpContext, HttpResponseMessage proxyResponse) in /home/manicka/Repositories/reverse-proxy.2/src/ReverseProxy/Service/Proxy/HttpTransformer.cs:line 71
   at Microsoft.ReverseProxy.Service.RuntimeModel.Transforms.StructuredTransformer.<>n__1(HttpContext httpContext, HttpResponseMessage proxyResponse)
   at Microsoft.ReverseProxy.Service.RuntimeModel.Transforms.StructuredTransformer.TransformResponseAsync(HttpContext httpContext, HttpResponseMessage proxyResponse) in /home/manicka/Repositories/reverse-proxy.2/src/ReverseProxy/Service/RuntimeModel/Transforms/StructuredTransformer.cs:line 102
   at Microsoft.ReverseProxy.Service.Proxy.HttpProxy.ProxyAsync(HttpContext context, String destinationPrefix, HttpMessageInvoker httpClient, RequestProxyOptions requestOptions, HttpTransformer transformer) in /home/manicka/Repositories/reverse-proxy.2/src/ReverseProxy/Service/Proxy/HttpProxy.cs:line 176
   at Microsoft.ReverseProxy.Middleware.ProxyInvokerMiddleware.Invoke(HttpContext context) in /home/manicka/Repositories/reverse-proxy.2/src/ReverseProxy/Middleware/ProxyInvokerMiddleware.cs:line 81
   at Microsoft.ReverseProxy.Middleware.PassiveHealthCheckMiddleware.Invoke(HttpContext context) in /home/manicka/Repositories/reverse-proxy.2/src/ReverseProxy/Middleware/PassiveHealthCheckMiddleware.cs:line 27
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.ReverseProxy.HeaderEncodingTests.<>c__DisplayClass1_0.<<ProxyAsync_ResponseWithEncodedHeaderValue>b__3>d.MoveNext() in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs:line 173
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task), Path = "/" }
   at Microsoft.ReverseProxy.HeaderEncodingTests.ProxyAsync_ResponseWithEncodedHeaderValue(String headerValue, String encodingName) in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs:line 186
   at Microsoft.ReverseProxy.HeaderEncodingTests.ProxyAsync_ResponseWithEncodedHeaderValue(String headerValue, String encodingName) in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs:line 201
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 264
--- End of stack trace from previous location ---
   at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:line 48
   at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in C:\Dev\xunit\xunit\src\xunit.core\Sdk\ExceptionAggregator.cs:line 90

The last two points stem from reverse proxy catching and translating only exception from HttpClient.SendAsync:
https://github.com/microsoft/reverse-proxy/blob/main/src/ReverseProxy/Service/Proxy/HttpProxy.cs#L130-L132
and not from all the code before and after. Thus, these errors are handled by kestrel and not available in IProxyErrorFeature.

Tratcher · 2021-02-19T19:20:07Z

In case an outgoing request construction fails, IProxyErrorFeature stays empty and only IExceptionHandlerFeature can be used to get the error details. Stack trace in this issue: Destination address is not validated. #765

The error you showed on #765 is a configuration issue with Destination Addresses. Can you show the error that would be hit if Kestrel accepted UTF-8 or Latin1 headers but then they were rejected when copying to HttpClient? I know the callstack will be similar, but I want to be clear on the specific exception thrown so we can handle it.

IExceptionHandlerFeature is only relevant if an app has app.UseExceptionHandler("/Home/Error"); in Startup.Configure. Otherwise the exception is caught by the server and logged.

ManickaP · 2021-02-23T18:15:40Z

The error you showed on #765 is a configuration issue with Destination Addresses. Can you show the error that would be hit if Kestrel accepted UTF-8 or Latin1 headers but then they were rejected when copying to HttpClient? I know the callstack will be similar, but I want to be clear on the specific exception thrown so we can handle it.

For headers it won't throw atm from HttpProxy.CreateRequestMessageAsync. It'll throw from HttpClient.SendAsync and is covered by ProxyErrorFeature:

System.Net.Http.HttpRequestException: Request headers must contain only ASCII characters.
   at System.Net.Http.HttpConnection.WriteStringAsync(String s, Boolean async)
   at System.Net.Http.HttpConnection.WriteStringAsync(String s, Boolean async, Encoding encoding)
   at System.Net.Http.HttpConnection.WriteHeadersAsync(HttpHeaders headers, String cookiesFromContainer, Boolean async)
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at Microsoft.ReverseProxy.Service.Proxy.HttpProxy.ProxyAsync(HttpContext context, String destinationPrefix, HttpMessageInvoker httpClient, RequestProxyOptions requestOptions, HttpTransformer transformer) in /home/manicka/Repositories/reverse-proxy.2/src/ReverseProxy/Service/Proxy/HttpProxy.cs:line 130 }
   at Microsoft.ReverseProxy.HeaderEncodingTests.<>c__DisplayClass0_0.<<ProxyAsync_RequestWithEncodedHeaderValue>b__3>d.MoveNext() in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs:line 98
--- End of stack trace from previous location ---
   at Microsoft.ReverseProxy.HeaderEncodingTests.<>c__DisplayClass0_0.<<ProxyAsync_RequestWithEncodedHeaderValue>b__3>d.MoveNext() in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs:line 106
--- End of stack trace from previous location ---
   at Microsoft.ReverseProxy.Common.TestEnvironment.Invoke(Func`2 clientFunc, CancellationToken cancellationToken) in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/Common/TestEnvironment.cs:line 74
   at Microsoft.ReverseProxy.Common.TestEnvironment.Invoke(Func`2 clientFunc, CancellationToken cancellationToken) in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/Common/TestEnvironment.cs:line 80
   at Microsoft.ReverseProxy.HeaderEncodingTests.ProxyAsync_RequestWithEncodedHeaderValue(String headerValue, String encodingName) in /home/manicka/Repositories/reverse-proxy.2/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs:line 71
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 264
--- End of stack trace from previous location ---
   at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:line 48
   at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in C:\Dev\xunit\xunit\src\xunit.core\Sdk\ExceptionAggregator.cs:line 90

Although a custom transform could cause an exception here since TransformRequestAsync is called from CreateRequestMessageAsync:

reverse-proxy/src/ReverseProxy/Service/Proxy/HttpProxy.cs

Line 282 in bb7ab95

    
           await transformer.TransformRequestAsync(context, destinationRequest, destinationPrefix);

ManickaP · 2021-02-23T18:17:55Z

IExceptionHandlerFeature is only relevant if an app has app.UseExceptionHandler("/Home/Error"); in Startup.Configure. Otherwise the exception is caught by the server and logged.

Yeah, I register the handler like this:

reverse-proxy/test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs

Lines 54 to 61 in 0c977e2

    
                               proxyApp.UseExceptionHandler(new ExceptionHandlerOptions 
        
                               { 
        
                                   ExceptionHandler = context => 
        
                                   { 
        
                                       kestrelError = context.Features.Get<IExceptionHandlerFeature>(); 
        
                                       return Task.CompletedTask; 
        
                                   } 
        
                               });

test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs

Tratcher · 2021-02-23T18:39:26Z

Although a custom transform could cause an exception here since TransformRequestAsync is called from CreateRequestMessageAsync:

Hmm, that implies an app code bug rather than an issue proxying the request. It's probably best to let those escape and be reported up stack. The client would see a 500 response which seems appropriate.

If any of the built in transforms could cause an exception then I think we'd address those in the transform.

Tratcher · 2021-02-23T18:43:06Z

In case an incoming response [headers] fails to be copied to kestrel, IProxyErrorFeature stays empty an only IExceptionHandlerFeature can be used to get the error details.

We should catch that one since it's specific to the request/response data.

ManickaP · 2021-02-23T19:00:28Z

.NET 3.1 HttpClient AppSwitch for Latin1 headers (nothing for UTF-8):
https://github.com/dotnet/corefx/blob/release/3.1/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/StaticHttpSettings.cs#L10

Do we want to enable process wide app switch? Or should we only document this option for .NET 3.1?

Kestrel 3.1 "Latin1RequestHeaders" config value:
https://github.com/dotnet/aspnetcore/blob/release/3.1/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs#L70-L81

Tratcher · 2021-02-24T00:49:31Z

Do we want to enable process wide app switch? Or should we only document this option for .NET 3.1?

Docs only. app switches don't test well. Just do a manual test.

docs/docfx/articles/proxyhttpclientconfig.md

Tratcher · 2021-02-25T19:53:38Z

docs/docfx/articles/proxyhttpclientconfig.md

+        .ConfigureWebHostDefaults(webBuilder =>
+        {
+            webBuilder.UseStartup<Startup>();
+        })
+        .ConfigureWebHost(webBuilder => webBuilder.UseKestrel(kestrel =>
+        {
+            kestrel.RequestHeaderEncodingSelector = _ => Encoding.Latin1;
+        }));


ConfigureWebHost and ConfigureWebHostDefaults are redundant. Also call ConfigureKestrel rather than UseKestrel here, UseKestrel was already called by ConfigureWebHostDefaults.

Suggested change

.ConfigureWebHostDefaults(webBuilder =>

{

webBuilder.UseStartup<Startup>();

})

.ConfigureWebHost(webBuilder => webBuilder.UseKestrel(kestrel =>

{

kestrel.RequestHeaderEncodingSelector = _ => Encoding.Latin1;

}));

.ConfigureWebHostDefaults(webBuilder =>

{

webBuilder.UseStartup<Startup>();

webBuilder.ConfigureKestrel(kestrelOptions =>

{

kestrelOptions.RequestHeaderEncodingSelector = _ => Encoding.Latin1;

})

});

Tratcher · 2021-02-25T19:56:27Z

docs/docfx/articles/proxyhttpclientconfig.md

+AppContext.SetSwitch("Microsoft.AspNetCore.Server.Kestrel.Latin1RequestHeaders", true);
+```
+
+At the moment, there is no solution for changing encoding for response headers, only ASCII is accepted.


... in Kestrel.

Link to dotnet/aspnetcore#26334.

Tratcher · 2021-02-25T22:05:17Z

src/ReverseProxy/Service/Proxy/HttpProxy.cs

+                    // Clear the response since status code, reason and some headers might have already been copied and we want clean 502 response.
+                    context.Response.Clear();
+                    context.Response.StatusCode = StatusCodes.Status502BadGateway;
+                    return;


Make sure to dispose the destinationResponse to avoid a leak.

Tratcher · 2021-02-25T22:12:34Z

test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs

+                using var httpClient = new HttpClient();
+                using var response = await httpClient.GetAsync(proxy.GetAddress());
+
+                Assert.NotNull(proxyError);


Check the enum and the exception type and message here so we'll know if it starts failing for another reason.

ManickaP added 2 commits February 17, 2021 18:32

Latin1 and UTF8 header value e2e tests

f3a11b5

Response test

0c977e2

ManickaP force-pushed the mapichov/154_utf8_headers branch from 6670731 to 0c977e2 Compare February 18, 2021 12:10

Tratcher requested review from MihaZupan and Tratcher and removed request for MihaZupan February 18, 2021 16:46

Tratcher reviewed Feb 18, 2021

View reviewed changes

test/ReverseProxy.FunctionalTests/Common/TestEnvironment.cs Outdated Show resolved Hide resolved

test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs Outdated Show resolved Hide resolved

Tratcher reviewed Feb 23, 2021

View reviewed changes

test/ReverseProxy.FunctionalTests/HeaderEncodingTests.cs Outdated Show resolved Hide resolved

Addressed styling issues.

3d1760e

Configuration for request header encoding.

1530881

ManickaP force-pushed the mapichov/154_utf8_headers branch from a37e772 to 1530881 Compare February 24, 2021 20:03

ManickaP added 3 commits February 25, 2021 16:19

Response headers error handling and added tests for the new option.

816f107

Docs.

256f711

Merge branch 'main' into mapichov/154_utf8_headers

7cb6e49

ManickaP force-pushed the mapichov/154_utf8_headers branch from d21d96a to 7cb6e49 Compare February 25, 2021 18:04

ManickaP commented Feb 25, 2021

View reviewed changes

docs/docfx/articles/proxyhttpclientconfig.md Show resolved Hide resolved

ManickaP marked this pull request as ready for review February 25, 2021 18:07

ManickaP requested a review from alnikola as a code owner February 25, 2021 18:07

Tratcher approved these changes Feb 25, 2021

View reviewed changes

Tratcher mentioned this pull request Feb 25, 2021

Support ProxyHttpClientOptions for ServiceFabric #787

Merged

Updated docs.

219b9f8

Addressed PR feedback.

9916bf9

ManickaP merged commit 3bcabe7 into microsoft:main Feb 26, 2021

ManickaP deleted the mapichov/154_utf8_headers branch February 26, 2021 20:06

Kahbazi mentioned this pull request Feb 26, 2021

Support RequestHeaderEncoding in ServiceFabric #794

Merged

karelz mentioned this pull request Mar 24, 2021

Test header encoding end-to-end #154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E tests for different header encodings #760

E2E tests for different header encodings #760

ManickaP commented Feb 18, 2021

Tratcher commented Feb 18, 2021 •

edited

Loading

Tratcher left a comment

ManickaP commented Feb 19, 2021

Tratcher commented Feb 19, 2021

ManickaP commented Feb 23, 2021

ManickaP commented Feb 23, 2021

Tratcher commented Feb 23, 2021

Tratcher commented Feb 23, 2021

ManickaP commented Feb 23, 2021 •

edited

Loading

Tratcher commented Feb 24, 2021

Tratcher Feb 25, 2021

Tratcher Feb 25, 2021

Tratcher Feb 25, 2021

Tratcher Feb 25, 2021

	if (headerEncoding != null)
	{
	kestrel.RequestHeaderEncodingSelector = _ => headerEncoding;
	}

E2E tests for different header encodings #760

E2E tests for different header encodings #760

Conversation

ManickaP commented Feb 18, 2021

1. Proxying request with UTF8/Latin1 header values

1.a. Proxy server needs: kestrel.RequestHeaderEncodingSelector:

1.b. Proxy client needs: RequestHeaderEncodingSelector:

2. Proxying back response with UTF8/Latin1 header values

2.a. Proxy client needs: ResponseHeaderEncodingSelector:

2.b. Proxy server fails with: System.InvalidOperationException: Invalid non-ASCII or control character in header

Tratcher commented Feb 18, 2021 • edited Loading

Tratcher left a comment

Choose a reason for hiding this comment

ManickaP commented Feb 19, 2021

Tratcher commented Feb 19, 2021

ManickaP commented Feb 23, 2021

ManickaP commented Feb 23, 2021

Tratcher commented Feb 23, 2021

Tratcher commented Feb 23, 2021

ManickaP commented Feb 23, 2021 • edited Loading

Tratcher commented Feb 24, 2021

Tratcher Feb 25, 2021

Choose a reason for hiding this comment

Tratcher Feb 25, 2021

Choose a reason for hiding this comment

Tratcher Feb 25, 2021

Choose a reason for hiding this comment

Tratcher Feb 25, 2021

Choose a reason for hiding this comment

1.a. Proxy server needs: `kestrel.RequestHeaderEncodingSelector`:

1.b. Proxy client needs: `RequestHeaderEncodingSelector`:

2.a. Proxy client needs: `ResponseHeaderEncodingSelector`:

2.b. Proxy server fails with: `System.InvalidOperationException: Invalid non-ASCII or control character in header`

Tratcher commented Feb 18, 2021 •

edited

Loading

ManickaP commented Feb 23, 2021 •

edited

Loading