dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.52k stars 10.04k forks source link

ConfigureKestrel() for certificate authentication must be placed before UseKestrel()! #15351

Closed replaysMike closed 4 years ago

replaysMike commented 5 years ago

The docs don't mention it but to configure Kestrel for certificate authentication, you must place the following before UseKestrel() is defined.

.ConfigureKestrel(options => {
    options.ConfigureHttpsDefaults(opt => {
        opt.ClientCertificateMode = ClientCertificateMode.RequireCertificate;
    });
})

Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

jkotalik commented 5 years ago

I'm confused about why I was assigned this; it seems very arbitrary. @anurse can we create a process where we assign issues to either you or to a group of engineers for visibility? For example, I believe I was OOF when this was assigned.

With regards to the actual issue. I don't believe this should be the case. If it is, it's a bug. @replaysMike can you provide a minimal repro?

analogrelay commented 5 years ago

I'm going to move this to aspnet/AspNetCore. I agree with @jkotalik that I don't think this should be the case. We should investigate this. If it turns out there's a reason this ordering is required we can update the docs.

blowdart commented 5 years ago

It's required because we need to turn the normal cert authentication off so we can handle it later on as middleware. It was in my original docs it just appears not to have made it over.

The reason you want to force it up front is suddenly demanding one would mean dropping the connection and renegotiating it, something we just don't support.

Tratcher commented 5 years ago

But ConfigureKestrel and UseKestrel shouldn't be order dependent.

analogrelay commented 5 years ago

@replaysMike can you provide a runnable sample that reproduces the problem so we can investigate? We don't believe these should be order-dependent.

replaysMike commented 5 years ago

I’ll try to mark a few minutes to do this today

analogrelay commented 5 years ago

@Tratcher was also going to take a look and double check.

Tratcher commented 5 years ago

I can reproduce this issue with the following code:

            var host = new WebHostBuilder()
                .UseKestrel(options =>
                {
                    options.ListenLocalhost(5001, options =>
                    {
                        options.UseHttps();
                    });
                })
                .ConfigureKestrel(options => {
                    options.ConfigureHttpsDefaults(opt => {
                        opt.ClientCertificateMode = ClientCertificateMode.RequireCertificate;
                    });
                })
                .UseContentRoot(Directory.GetCurrentDirectory())
                .UseStartup<Startup>()
                .Build();

            await host.RunAsync();

The problem is that ListenLocalhost and UseHttps run inline and consume the endpoint and https defaults so ConfigureHttpsDefaults must be called before any endpoints are registered.

The workaround described above of calling ConfigureKestrel before UseKestrel is valid, but not viable when using CreateDefaultBuilder.

The scenario where setting ConfigureHttpsDefaults in ConfigureKestrel works is when the endpoints are defined in config such as how CreateDefaultBuilder already does. This works because the config doesn't actually create endpoints until the server starts.

(More notes to follow)

Tratcher commented 5 years ago

Note UseKestrel and ConfigureKestrel do mostly the same thing except that ConfigureKestrel doesn't register an IServer so it avoids conflicts with other lazily activated servers like IIS in-proc.

In most applications UseKestrel is called by CreateDefaultBuilder or ConfigureWebHostDefaults so you won't be calling Listen in there. ConfigureKestrel was added so you could call it after CreateDefaultBuilder without causing conflicts like the IIS one above. In this scenario all Kestrel config should happen inside ConfigureKestrel, you shouldn't have direct calls to UseKestrel.

If you're not using CreateDefaultBuilder or ConfigureWebHostDefaults then you call UseKestrel and do all of the config there, there's no need to call ConfigureKestrel.

When I combine the UseKestrel and ConfigureKestrel code from my sample above the ordering issue still exists within the call:

                .Use/ConfigureKestrel(options =>
                {
                    options.ListenLocalhost(5001, options =>
                    {
                        options.UseHttps();
                    });
                    options.ConfigureHttpsDefaults(opt => {
                        opt.ClientCertificateMode = ClientCertificateMode.RequireCertificate;
                    });
                })

The above doesn't work because ConfigureHttpsDefaults needs to be called before ListenLocalhost in order to affect that endpoint:

                .Use/ConfigureKestrel(options =>
                {
                    options.ConfigureHttpsDefaults(opt => {
                        opt.ClientCertificateMode = ClientCertificateMode.RequireCertificate;
                    });
                    options.ListenLocalhost(5001, options =>
                    {
                        options.UseHttps();
                    });
                })

@replaysMike does this match what you're seeing?

I don't anticipate making any code fixes here, there's not much that can be changed without some dramatic redesigns. There are likely some clarifications we can make to the docs though.

jkotalik commented 4 years ago

I believe https://github.com/aspnet/AspNetCore.Docs/pull/15697 documented this issue. Closing.