Add AllowLineTerminator option in KestrelServerOptions

[sebastienros]

Background and Motivation

New option to support LF as a request line terminator.

Proposed API

public class KestrelServerOptions
{
     /// <summary>
     /// Gets or sets a value that controls whether the request lines
     /// can end with LF only instead of CR/LF.
     /// </summary>
     /// <remarks>
     /// Defaults to false.
     /// </remarks>
+    public bool AllowLineFeedTerminator { get; set; }
}

Usage Examples

n/a

Alternative Designs

n/a

Risks

The change is non-breaking, the default is not activating the new feature. The feature has been verified in production by Azure App Service.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

Relevant PR: #41101

Does the spec take a position on whether servers ought to be tolerant of this or not? I know that at least some customers need this, but is this common? I don't want to make KestrelServerOptions any more of a mess than it already is, so an AppContext switch like the PR already has seems like it might be appropriate if this is a niche scenario.

If we do need public API, I think it needs to be clear what we're enabling the line feed terminator for. This also applies to the name of the switch.

_enableLineFeedTerminator = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.EnableLineFeedTerminator", out var enabled) && enabled;

I think EnableLineFeedTerminator should probably be something like AllowHttp1LineFeedTerminators. @dotnet/aspnet-api-review does anyone else have an opinion here?

[sebastienros]

Correct PR: #43202

[halter73]

I see. I linked to the .NET 6 backport PR where we cannot change public API. Can we just continue using the AppContext switch in main?

[sebastienros]

Using an option in 7 was a decision we made at the time of the issue, because we wanted it to be an officially supported feature. But let's see if what everyone things about it.

[halter73]

Because we wanted it to be an officially supported feature. But let's see if what everyone things about it.

I remember wanting it shipped in an official release, but I didn't know that "officially supported" meant a public API rather than an AppContext switch. I feel that we can officially support a feature either way by promising not to remove it. We could remove a public API as well, although there is more process around obsoleting those.

[adityamandaleeka]

Does the spec take a position on whether servers ought to be tolerant of this or not? I know that at least some customers need this, but is this common? I don't want to make KestrelServerOptions any more of a mess than it already is, so an AppContext switch like the PR already has seems like it might be appropriate if this is a niche scenario.

Yea, this is a case RFC 7230 acknowledges.

Although the line terminator for the start-line and header fields is
the sequence CRLF, a recipient MAY recognize a single LF as a line
terminator and ignore any preceding CR.

(https://www.rfc-editor.org/rfc/rfc7230#section-3.5)

Regarding the question of whether this should be public API or AppContext switch, I think it comes down to defining what each of those things should be used for. This is an opt-in feature that's new and we intend to keep it, so IMO it makes sense that it would be public API surface. I think the original intent of the AppContext switches was to allow people to use them for compat reasons (i.e. opt out of new behavior), though I realize they've now been used for other purposes as well.

[halter73]

I can buy this should be an option. What do we think about AllowHttp1LineFeedTerminators as the property and AppContext switch name? Do we want to support both in main?

Also, if there are going to be other HTTP parsing knobs, do we need to want to add a subclass to KestrelServerOptions along the lines of KestrelServerOptions.Limits? Maybe KestrelServerOptions.HttpParsing? Right now, I'm leaning towards no. But if we end up with more than one or two of these things, I'm going to regret not adding it.

[halter73]

Also, would we ever consider making this opt-out? Then we could avoid adding yet another option and the AppContext switch would serve its intended purpose of opting out of new behavior. Allowing linefeeds doesn't really regress perf now, does it?

[adityamandaleeka]

What do we think about AllowHttp1LineFeedTerminators as the property and AppContext switch name?

I don't have a strong opinion between the two. If you feel that adding the Http1 makes it clearer, that's fine with me.

Do we want to support both in main?

You mean both the AppContext switch and the property? That seems unnecessary, no? IMO the number of people who will actually opt into the switch on 6.0 and then upgrade to 7+ will be small. Most (of the small number of) people who use this will start with 7+ so I'm not too worried about not supporting the switch anymore.

Also, if there are going to be other HTTP parsing knobs, do we need to want to add a subclass to KestrelServerOptions along the lines of KestrelServerOptions.Limits? Maybe KestrelServerOptions.HttpParsing? Right now, I'm leaning towards no. But if we end up with more than one or two of these things, I'm going to regret not adding it.

Ooh, good question. I feel the same. I don't think we have any other parsing options planned at the moment, but I wouldn't bet against running into another case we want to add in .NET 8. It's clunky to have a single thing in KestrelServerOptions.HttpParsing but maybe it's worth it to avoid the risk? 😬

[davidfowl]

I think we should have properties for http1 2 and 3 like we have limits.

[JamesNK]

You know, if you squint hard enough this setting does look kind of like a limit 😆

[halter73]

It limits what byte sequences we will accept as a start line and header terminators after all!

[JamesNK]

I don't mind a new Http1Limits type with a AllowLineFeedTerminators property.

It's better than adding a clunky AllowHttp1LineFeedTerminators on KestrelServerOptions or KestrelServerLimits.

[halter73]

API review notes:

1. Let's try making this the default because if a request uses a LF without a subsequent CR, how else could we interpret it? The spec encourages it.
2. Let's default to turning this on if it doesn't regress perf. And then temporarily add an AppContext switch for disabling the new behavior. This switch could be removed in a future release.

_enableLineFeedTerminator = AppContext.TryGetSwitch("Microsoft.AspNetCore.Server.Kestrel.DisableHttp1LineFeedTerminators", out var disabled) && disabled;

Technically, not new API, but let's use this name.

[blowdart]

We should consider having a separate "DontObeyRFCs" type of options.

This isn't as silly as it sounds, it would highlight some things which shouldn't be used unless you really have a need due to weird clients, or being Bing. It'd make them less discoverable by accident too.

[halter73]

I'd be more okay with a single "strict mode" option for Kestrel HTTP parsing. Or maybe a "quirks mode" option. I don't wan't to add a million parsing knobs though.

I still think we should allow line fee it by default considering the RFC says servers may allow this. I think most people want their HTTP server to just work. I think using Kestrel as a way to validate the RFC compliance of HTTP clients is extremely niche and incomplete anyway.