Make Kestrel config case-insensitive for certificates #23268
Conversation
wtgodbe
requested review from
halter73,
Tratcher,
javiercn,
BrennanConroy and
jkotalik
ghost
added
the
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This test fails without the change to ConfigurationReader.cs
| @@ -253,6 +253,38 @@ public void ConfigureEndpointDevelopmentCertificateGetsLoadedWhenPresent() | |||
| } | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void ConfigureEndpointDevelopmentCertificateIsCaseInsensitive() | |||
There was a problem hiding this comment.
Since the change was in ConfigurationReader, you can probobly write a more targeted test in ConfigurationReaderTests. That would let you avoid some of this overhead like writing a cert to file.
https://github.com/dotnet/aspnetcore/blob/fa20111e47774b6c08f82a1f0c6373cc30e99a83/src/Servers/Kestrel/Kestrel/test/ConfigurationReaderTests.cs
There was a problem hiding this comment.
Good idea. I try to avoid file I/O (or really any I/O) in Kestrel tests if at all possible.
| new KeyValuePair<string, string>("Certificates:FILECERT:Password", "certpassword"), | ||
| }).Build()); | ||
|
|
||
| Assert.Contains("An item with the same key has already been added", exception.Message); |
There was a problem hiding this comment.
This string is in Kestrel/Core/src/CoreStrings.resx - would it better to add a reference to that resource file to the .csproj?
There was a problem hiding this comment.
CoreStrings is already InternalsVisibleTo to this project you just need to add a using for Microsoft.AspNetCore.Server.Kestrel.Core.
|
Updated |
| var config = new ConfigurationBuilder().AddInMemoryCollection(new[] | ||
| { | ||
| new KeyValuePair<string, string>("Certificates:filecert:Path", "/path/cert.pfx"), | ||
| new KeyValuePair<string, string>("Certificates:FILECERT:Password", "certpassword"), |
There was a problem hiding this comment.
This should still pass if the entire key is UPPERCASE, right? I know this PR only changes the dictionary comparer, but I see no reason not to verify the rest of it case insensitive while we're at it.
|
Hello @davidfowl! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
amcasey
added
area-networking
Attempts to resolve #21494