Skip to content

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Jan 6, 2022

Description

There are known clients that send the request with the full url in the request line, but then also include a malformed or mismatched Host header. IIS/Http.Sys ignores the invalid host in this scenario, where Kestrel rejects it with a 400. This change provides an opt-in switch for customers to test this new approach in Kestrel.

Fixes #39335

Customer Impact

Some clients are not able to communicate with Kestrel, and updating the client isn't in the customer's control.

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

Opt-in validation change.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@Tratcher Tratcher added the Servicing-consider Shiproom approval is required for the issue label Jan 6, 2022
@Tratcher Tratcher added this to the 6.0.x milestone Jan 6, 2022
@Tratcher Tratcher requested a review from halter73 January 6, 2022 00:39
@Tratcher Tratcher self-assigned this Jan 6, 2022
@ghost ghost added the area-runtime label Jan 6, 2022
@ghost
Copy link

ghost commented Jan 6, 2022

Hi @Tratcher. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@ghost
Copy link

ghost commented Jan 6, 2022

Hi @Tratcher. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@Tratcher Tratcher marked this pull request as ready for review January 6, 2022 00:55
private Func<string, Encoding?> _responseHeaderEncodingSelector = DefaultHeaderEncodingSelector;

private bool? _enableInsecureAbsoluteFormHostOverride;
internal bool EnableInsecureAbsoluteFormHostOverride
Copy link
Member

Choose a reason for hiding this comment

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

Is a public version of this the API proposal for .NET 7?

Copy link
Member

Choose a reason for hiding this comment

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

Should we allow for a missing Host header if this is set and there's an absolute form request target?

Copy link
Member Author

Choose a reason for hiding this comment

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

The customer is currently asking for a short term mitigation, it's not clear we'll still need this in 7.0.

Should we allow for a missing Host header if this is set and there's an absolute form request target?

We could expand to that if needed, but it hasn't come up yet. So far the client is sending a Host header, but it's malformed.

Copy link
Member

@halter73 halter73 Jan 6, 2022

Choose a reason for hiding this comment

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

I think it's weird to require a HOST header if we're going to ignore it anyway. What does httpsys do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Http.Sys also always requires the Host header to be present (even if empty), even if it overrides it with the host from the request line.

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 6, 2022
@leecow leecow modified the milestones: 6.0.x, 6.0.2 Jan 6, 2022
@Tratcher
Copy link
Member Author

Tratcher commented Jan 7, 2022

@dotnet/aspnet-build please merge.

@dougbu dougbu merged commit 9c8ec73 into dotnet:release/6.0 Jan 7, 2022
@Tratcher Tratcher deleted the tratcher/release/6.0/kestrel-host branch January 7, 2022 21:05
amcasey pushed a commit to amcasey/aspnetcore that referenced this pull request May 24, 2023
…ost (dotnet#39334)

* Allow overriding the host header if doesn't match the absolute-form host
* Apply suggestions from code review

Co-authored-by: Stephen Halter <[email protected]>
amcasey pushed a commit to amcasey/aspnetcore that referenced this pull request May 26, 2023
…ost (dotnet#39334)

* Allow overriding the host header if doesn't match the absolute-form host
* Apply suggestions from code review

Co-authored-by: Stephen Halter <[email protected]>
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
amcasey pushed a commit to amcasey/aspnetcore that referenced this pull request Jul 31, 2023
…ost (dotnet#39334)

* Allow overriding the host header if doesn't match the absolute-form host
* Apply suggestions from code review

Co-authored-by: Stephen Halter <[email protected]>
amcasey added a commit that referenced this pull request Aug 1, 2023
* Allow overriding the host header if doesn't match the absolute-form host (#39334)

* Allow overriding the host header if doesn't match the absolute-form host
* Apply suggestions from code review

Co-authored-by: Stephen Halter <[email protected]>

* Add explanatory comment

* Replace internal API and appcontext switch with public API

The new public API is `KestrelServerOptions.AllowUnsafeHostHeaderOverride` and I've moved the explanatory comments there.  The behavior remains opt-in.

* Separate corruption and mismatch tests

* Rename property per API review

* Clarify comment.

Co-authored-by: Chris Ross <[email protected]>

---------

Co-authored-by: Chris Ross <[email protected]>
Co-authored-by: Stephen Halter <[email protected]>
Co-authored-by: Chris Ross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants