Skip to content

Commit 8b13043

Browse files
halter73halter73
committed
Address PR feedback
1 parent 0403b56 commit 8b13043

File tree

6 files changed

+56
-43
lines changed

6 files changed

+56
-43
lines changed

‎src/Servers/Kestrel/Core/src/CoreStrings.resx‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -621,9 +621,12 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
621621
<value>Unknown algorithm for certificate with public key type '{0}'.</value>
622622
</data>
623623
<data name="SniNotConfiguredForServerName" xml:space="preserve">
624-
<value>Connection refused because no SNI configuration was found for '{serverName}' in '{endpointName}'. To allow all connections, add a wildcard ('*') SNI section.</value>
624+
<value>Connection refused because no SNI configuration section was found for '{serverName}' in '{endpointName}'. To allow all connections, add a wildcard ('*') SNI section.</value>
625625
</data>
626626
<data name="SniNotConfiguredToAllowNoServerName" xml:space="preserve">
627-
<value>Connection refused because the client did not specify a server name, and no wildcard ('*') SNI configuration was found in '{endpointName}'.</value>
627+
<value>Connection refused because the client did not specify a server name, and no wildcard ('*') SNI configuration section was found in '{endpointName}'.</value>
628628
</data>
629-
</root>
629+
<data name="SniNameCannotBeEmpty" xml:space="preserve">
630+
<value>The endpoint {endpointName} is invalid because an SNI configuration section has an empty string as its key. Use a wildcard ('*') SNI section to match all server names.</value>
631+
</data>
632+
</root>

‎src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ private IEnumerable<EndpointConfig> ReadEndpoints()
9999
Certificate = new CertificateConfig(endpointConfig.GetSection(CertificateKey)),
100100
SslProtocols = ParseSslProcotols(endpointConfig.GetSection(SslProtocolsKey)),
101101
ClientCertificateMode = ParseClientCertificateMode(endpointConfig[ClientCertificateModeKey]),
102-
Sni = ReadSni(endpointConfig.GetSection(SniKey)),
102+
Sni = ReadSni(endpointConfig.GetSection(SniKey), endpointConfig.Key),
103103
};
104104

105105
endpoints.Add(endpoint);
@@ -108,7 +108,7 @@ private IEnumerable<EndpointConfig> ReadEndpoints()
108108
return endpoints;
109109
}
110110

111-
private Dictionary<string, SniConfig> ReadSni(IConfigurationSection sniConfig)
111+
private Dictionary<string, SniConfig> ReadSni(IConfigurationSection sniConfig, string endpointName)
112112
{
113113
var sniDictionary = new Dictionary<string, SniConfig>(0, StringComparer.OrdinalIgnoreCase);
114114

@@ -132,6 +132,11 @@ private Dictionary<string, SniConfig> ReadSni(IConfigurationSection sniConfig)
132132
// }
133133
// }
134134

135+
if (string.IsNullOrEmpty(sniChild.Key))
136+
{
137+
throw new InvalidOperationException(CoreStrings.FormatSniNameCannotBeEmpty(endpointName));
138+
}
139+
135140
var sni = new SniConfig
136141
{
137142
Protocols = ParseProtocols(sniChild[ProtocolsKey]),
@@ -140,7 +145,7 @@ private Dictionary<string, SniConfig> ReadSni(IConfigurationSection sniConfig)
140145
ClientCertificateMode = ParseClientCertificateMode(sniChild[ClientCertificateModeKey])
141146
};
142147

143-
sniDictionary[sniChild.Key] = sni;
148+
sniDictionary.Add(sniChild.Key, sni);
144149
}
145150

146151
return sniDictionary;

‎src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs‎

Lines changed: 20 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
1818
{
1919
internal class SniOptionsSelector
2020
{
21-
private const string wildcardHost = "*";
22-
private const string wildcardPrefix = "*.";
21+
private const string WildcardHost = "*";
22+
private const string WildcardPrefix = "*.";
2323

2424
private readonly string _endpointName;
2525

2626
private readonly Func<ConnectionContext, string, X509Certificate2> _fallbackServerCertificateSelector;
2727
private readonly Action<ConnectionContext, SslServerAuthenticationOptions> _onAuthenticateCallback;
2828

29-
private readonly Dictionary<string, SniOptions> _fullNameOptions = new Dictionary<string, SniOptions>(StringComparer.OrdinalIgnoreCase);
29+
private readonly Dictionary<string, SniOptions> _exactNameOptions = new Dictionary<string, SniOptions>(StringComparer.OrdinalIgnoreCase);
3030
private readonly SortedList<string, SniOptions> _wildcardPrefixOptions = new SortedList<string, SniOptions>(LongestStringFirstComparer.Instance);
31-
private readonly SniOptions _wildcardHostOptions = null;
31+
private readonly SniOptions _wildcardOptions = null;
3232

3333
public SniOptionsSelector(
3434
ICertificateConfigLoader certifcateConfigLoader,
@@ -85,17 +85,18 @@ public SniOptionsSelector(
8585
HttpProtocols = httpProtocols,
8686
};
8787

88-
if (name.Equals(wildcardHost, StringComparison.Ordinal))
88+
if (name.Equals(WildcardHost, StringComparison.Ordinal))
8989
{
90-
_wildcardHostOptions = sniOptions;
90+
_wildcardOptions = sniOptions;
9191
}
92-
else if (name.StartsWith(wildcardPrefix, StringComparison.Ordinal))
92+
else if (name.StartsWith(WildcardPrefix, StringComparison.Ordinal))
9393
{
94-
_wildcardPrefixOptions.Add(name, sniOptions);
94+
// Only slice off 1 character, the `*`. We want to match the leading `.` also.
95+
_wildcardPrefixOptions.Add(name.Substring(1), sniOptions);
9596
}
9697
else
9798
{
98-
_fullNameOptions[name] = sniOptions;
99+
_exactNameOptions.Add(name, sniOptions);
99100
}
100101
}
101102
}
@@ -104,13 +105,20 @@ public SslServerAuthenticationOptions GetOptions(ConnectionContext connection, s
104105
{
105106
SniOptions sniOptions = null;
106107

107-
if (!string.IsNullOrEmpty(serverName) && !_fullNameOptions.TryGetValue(serverName, out sniOptions))
108+
if (!string.IsNullOrEmpty(serverName) && !_exactNameOptions.TryGetValue(serverName, out sniOptions))
108109
{
109-
TryGetWildcardPrefixedOptions(serverName, out sniOptions);
110+
foreach (var (suffix, options) in _wildcardPrefixOptions)
111+
{
112+
if (serverName.EndsWith(suffix, StringComparison.OrdinalIgnoreCase))
113+
{
114+
sniOptions = options;
115+
break;
116+
}
117+
}
110118
}
111119

112120
// Fully wildcarded ("*") options can be used even when given an empty server name.
113-
sniOptions ??= _wildcardHostOptions;
121+
sniOptions ??= _wildcardOptions;
114122

115123
if (sniOptions is null)
116124
{
@@ -149,27 +157,6 @@ public SslServerAuthenticationOptions GetOptions(ConnectionContext connection, s
149157
return sslOptions;
150158
}
151159

152-
private bool TryGetWildcardPrefixedOptions(string serverName, out SniOptions sniOptions)
153-
{
154-
sniOptions = null;
155-
156-
ReadOnlySpan<char> serverNameSpan = serverName;
157-
158-
foreach (var (nameCandidate, optionsCandidate) in _wildcardPrefixOptions)
159-
{
160-
ReadOnlySpan<char> nameCandidateSpan = nameCandidate;
161-
162-
// Only slice off 1 character, the `*`. We want to match the leading `.` also.
163-
if (serverNameSpan.EndsWith(nameCandidateSpan.Slice(1), StringComparison.OrdinalIgnoreCase))
164-
{
165-
sniOptions = optionsCandidate;
166-
return true;
167-
}
168-
}
169-
170-
return false;
171-
}
172-
173160
// TODO: Reflection based test to ensure we clone everything!
174161
// This won't catch issues related to mutable subproperties, but the existing subproperties look like they're mostly immutable.
175162
// The exception are the ApplicationProtocols list which we clone and the ServerCertificate because of methods like Import() and Reset() :(

‎src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ internal class HttpsConnectionMiddleware
3030
{
3131
private const string EnableWindows81Http2 = "Microsoft.AspNetCore.Server.Kestrel.EnableWindows81Http2";
3232

33-
private static readonly bool _isWindowsVersionIncompatible = IsWindowsVersionIncompatible();
33+
private static readonly bool _isWindowsVersionIncompatibleWithHttp2 = IsWindowsVersionIncompatibleWithHttp2();
3434

3535
private readonly ConnectionDelegate _next;
3636
private readonly TimeSpan _handshakeTimeout;
@@ -369,12 +369,12 @@ internal static HttpProtocols ValidateAndNormalizeHttpProtocols(HttpProtocols ht
369369
{
370370
throw new NotSupportedException(CoreStrings.Http2NoTlsOsx);
371371
}
372-
else if (_isWindowsVersionIncompatible)
372+
else if (_isWindowsVersionIncompatibleWithHttp2)
373373
{
374374
throw new NotSupportedException(CoreStrings.Http2NoTlsWin81);
375375
}
376376
}
377-
else if (httpProtocols == HttpProtocols.Http1AndHttp2 && _isWindowsVersionIncompatible)
377+
else if (httpProtocols == HttpProtocols.Http1AndHttp2 && _isWindowsVersionIncompatibleWithHttp2)
378378
{
379379
logger.Http2DefaultCiphersInsufficient();
380380
return HttpProtocols.Http1;
@@ -383,7 +383,7 @@ internal static HttpProtocols ValidateAndNormalizeHttpProtocols(HttpProtocols ht
383383
return httpProtocols;
384384
}
385385

386-
private static bool IsWindowsVersionIncompatible()
386+
private static bool IsWindowsVersionIncompatibleWithHttp2()
387387
{
388388
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
389389
{

‎src/Servers/Kestrel/Core/test/KestrelServerTests.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
using Microsoft.AspNetCore.Testing;
1717
using Microsoft.Extensions.Configuration;
1818
using Microsoft.Extensions.DependencyInjection;
19+
using Microsoft.Extensions.Hosting;
1920
using Microsoft.Extensions.Logging;
2021
using Microsoft.Extensions.Options;
2122
using Microsoft.Extensions.Primitives;
@@ -495,6 +496,7 @@ public async Task ReloadsOnConfigurationChangeWhenOptedIn()
495496
var serviceCollection = new ServiceCollection();
496497
serviceCollection.AddSingleton(mockLoggerFactory.Object);
497498
serviceCollection.AddSingleton(Mock.Of<ILogger<KestrelServer>>());
499+
serviceCollection.AddSingleton(Mock.Of<IHostEnvironment>());
498500

499501
var options = new KestrelServerOptions
500502
{
@@ -631,6 +633,7 @@ public async Task DoesNotReloadOnConfigurationChangeByDefault()
631633
var serviceCollection = new ServiceCollection();
632634
serviceCollection.AddSingleton(mockLoggerFactory.Object);
633635
serviceCollection.AddSingleton(Mock.Of<ILogger<KestrelServer>>());
636+
serviceCollection.AddSingleton(Mock.Of<IHostEnvironment>());
634637

635638
var options = new KestrelServerOptions
636639
{

‎src/Servers/Kestrel/Kestrel/test/ConfigurationReaderTests.cs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,21 @@ public void ReadEndpointWithoutSniConfigured_ReturnsEmptyCollection()
288288
Assert.False(endpoint.Sni.Any());
289289
}
290290

291+
[Fact]
292+
public void ReadEndpointWithEmptySniKey_Throws()
293+
{
294+
var config = new ConfigurationBuilder().AddInMemoryCollection(new[]
295+
{
296+
new KeyValuePair<string, string>("Endpoints:End1:Url", "http://*:5001"),
297+
new KeyValuePair<string, string>("Endpoints:End1:Sni::Protocols", "Http1"),
298+
}).Build();
299+
300+
var reader = new ConfigurationReader(config);
301+
var ex = Assert.Throws<InvalidOperationException>(() => reader.Endpoints);
302+
303+
Assert.Equal(CoreStrings.FormatSniNameCannotBeEmpty("End1"), ex.Message);
304+
}
305+
291306
[Fact]
292307
public void ReadEndpointWithSniConfigured_ReturnsCorrectValue()
293308
{

0 commit comments

Comments
 (0)