Update Proxy-Support check to be case insensitive #61446
Conversation
RFC4559 does not specify that the Proxy-Support header value used to determine if the proxy server will honour client server authentication integrity is case sensitive. Updating the check to be case insensitive to prevent failures when value is supplied using differences in case. Fix dotnet#61414
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsRFC4559 does not specify that the Proxy-Support header value used to Fix #61414
|
| @@ -61,7 +61,7 @@ private static bool ProxySupportsConnectionAuth(HttpResponseMessage response) | |||
|
|
|||
| foreach (string v in values) | |||
| { | |||
| if (v == "Session-Based-Authentication") | |||
There was a problem hiding this comment.
Separately, @geoffkizer, we might want to audit use of Headers and update appropriate sites to use NonValidated.
There was a problem hiding this comment.
Is the goal here to improve performance by avoiding the added processing/memory use that happens when you go through TryGetValues etc?
There are semantic differences with NonValidated. In particular if the server sent something like
Proxy-Support: Session-Based-Authentication, Some-Other-Magic-Token
Then the current code would enumerate this as two header values and successfully match the first. In general for header processing, this is what you want (e.g. consider Connection: close handling).
If we just use NonValidated here then we would need to parse these tokens ourselves. Which we could certainly do, and we could certainly do it in a more efficient way than the current header parsing logic does (e.g. deal with spans instead of allocating strings). But I'm not sure the perf here matters much.
There could be (and almost certainly are) scenarios where this perf does matter, e.g. Connection: close. But again, it's not as simple as just using NonValidated.
In short: Yes, there are things we should look into as far as improving header access perf, but NonValidated by itself doesn't really help all that much, and we need to investigate further to determine if/how to actually do better here.
There was a problem hiding this comment.
Is the goal here to improve performance by avoiding the added processing/memory use that happens when you go through TryGetValues etc?
Yes, that would be the goal.
|
Is a test feasible? |
Sadly, I cannot find a test for the existing behavior here. Given how small this change is, I think we should just take it despite the lack of tests. Authentication tests in general are a pain because you need to have a certain machine setup to run them. But we do have some, and we should certainly add one that covers this case as well in the future. @wfurt did I miss anything here? Thoughts? |
Fair question, I couldn't see anything testing that area to update, and I'm not that familiar with workings of authentication via proxy. In theory I could use reflection to test the private static method modified, rather than the whole authentication via proxy piece. |
ghost
locked as resolved and limited conversation to collaborators
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/3688296891 |
RFC4559 does not specify that the Proxy-Support header value used to
determine if the proxy server will honour client server authentication
integrity is case sensitive. Updating the check to be case insensitive
to prevent failures when value is supplied using differences in case.
Fix #61414