Expose SNI hostname via a feature

[shirhatti]

We currently do not expose the SNI hostname via a feature interface

aspnetcore/src/Servers/Kestrel/Core/src/Internal/TlsConnectionFeature.cs

Lines 52 to 53 in 640b77f

	// Used for event source, not part of any of the feature interfaces.
	public string? HostName { get; set; }

namespace Microsoft.AspNetCore.Http.Features
{
    public interface ITlsHandshakeFeature
    {
+        string? HostName { get; }
    }
}

Given that it's already a property on the TlsConnectionFeature we can trivially expose it.

[Tratcher]

Is it available on http.sys or IIS?

[shirhatti]

@alanwest @cijothomas Y'all might be interested in this for open-telemetry/opentelemetry-dotnet#2159 if you wish to add the http.server_name attribute.

[shirhatti]

@Tratcher Is there somewhere else I should be looking?

aspnetcore/src/Shared/HttpSys/NativeInterop/HttpApiTypes.cs

Lines 261 to 272 in 640b77f

	[StructLayout(LayoutKind.Sequential)]
	internal struct HTTP_SSL_INFO
	{
	internal ushort ServerCertKeySize;
	internal ushort ConnectionKeySize;
	internal uint ServerCertIssuerSize;
	internal uint ServerCertSubjectSize;
	internal byte* pServerCertIssuer;
	internal byte* pServerCertSubject;
	internal HTTP_SSL_CLIENT_CERT_INFO* pClientCertInfo;
	internal uint SslClientCertNegotiated;
	}

[ghost]

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[shirhatti]

Microsoft.Extensions.Features targets netstandard2.0 and net461 which means I can't use a DIM 😢

aspnetcore/src/Extensions/Features/src/Microsoft.Extensions.Features.csproj

Line 10 in 23b7049

<TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>

[Tratcher]

What are you trying to modify in Microsoft.Extensions.Features? I thought the features you needed to change were in Microsoft.AspNetCore.Http.Features?

[shirhatti]

Oops. I meant

aspnetcore/src/Servers/Connections.Abstractions/src/Microsoft.AspNetCore.Connections.Abstractions.csproj

Line 5 in 23b7049

<TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0;netstandard2.1;$(DefaultNetCoreTargetFramework)</TargetFrameworks>

[Tratcher]

Ah. I guess we'll have to put it on ITlsConnectionFeature instead.

aspnetcore/src/Http/Http.Features/src/ITlsConnectionFeature.cs

Line 13 in 23b7049

public interface ITlsConnectionFeature

[pranavkm]

We think it's better to add an additional feature interface to Connection.Abstractions rather than adding this to ITlsConnectionFeature:

namespace Microsoft.AspNetCore.Connection.Abstractions
{
+    public interface ITlsServerNameFeature
+    {
+        string? ServerName { get; }
+    }
}

[shirhatti]

Related: #35123

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[shirhatti]

Blocked on dotnet/runtime#57105

[shirhatti]

Un-backlogging since we're no longer blocked on runtime

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[BrennanConroy]

Triage:
We can #if the new property on an existing interface to workaround netstandard not supporting default interface methods.

[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.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[halter73]

namespace Microsoft.AspNetCore.Connections.Features;

public interface ITlsHandshakeFeature
{
+ #if NETCOREAPP
+     /// <summary>
+     /// Gets the host name from the "sever_name" extension of the client hello if present.
+     /// See <see href="https://www.rfc-editor.org/rfc/rfc6066#section-3">RFC 6066</see>.
+     /// </summary>
+     string? HostName => null;
+ #endif
}

Note: We already do a similar #if in ITlsHandshakeFeature for NegotiatedCipherSuite.

aspnetcore/src/Servers/Connections.Abstractions/src/Features/ITlsHandshakeFeature.cs

Lines 22 to 27 in 6470682

	#if NETCOREAPP
	/// <summary>
	/// Gets the <see cref="TlsCipherSuite"/>.
	/// </summary>
	TlsCipherSuite? NegotiatedCipherSuite => null;
	#endif

[halter73]

API Review Notes:

• This was already approved as separate ITlsServerNameFeature. Considering we already have NegotiatedCipherSuite on the same interface in an #if NETCOREAPP without issue, it makes sense to do it again for HostName.

API Approved!

namespace Microsoft.AspNetCore.Connections.Features;

public interface ITlsHandshakeFeature
{
+ #if NETCOREAPP
+     /// <summary>
+     /// Gets the host name from the "sever_name" extension of the client hello if present.
+     /// See <see href="https://www.rfc-editor.org/rfc/rfc6066#section-3">RFC 6066</see>.
+     /// </summary>
+     string? HostName => null;
+ #endif
}

[halter73]

We decided that there was no reason to distinguish between the client sending an empty host name using SNI, the client not using SNI, and a server that doesn't support this feature in the PR. The reason the SNI host name is unavailable should not change how it's handled. This means it should be simpler to make HostName non-nullable and have the default implementation return the empty string to avoid unnecessary null checks.

Here's the updated API proposal:

namespace Microsoft.AspNetCore.Connections.Features;

public interface ITlsHandshakeFeature
{
+ #if NETCOREAPP
+     /// <summary>
+     /// Gets the host name from the "sever_name" extension of the client hello if present.
+     /// See <see href="https://www.rfc-editor.org/rfc/rfc6066#section-3">RFC 6066</see>.
+     /// </summary>
+     string HostName => string.Empty;
+ #endif
}

[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]

API Review Notes:

• Do we need a null value to indicate the server doesn't support this?
  • Maybe, but using a null HostName is too subtle for this
  • If it's important we can add a bool.

API Approved as merged!

namespace Microsoft.AspNetCore.Connections.Features;

public interface ITlsHandshakeFeature
{
+ #if NETCOREAPP
+     /// <summary>
+     /// Gets the host name from the "sever_name" extension of the client hello if present.
+     /// See <see href="https://www.rfc-editor.org/rfc/rfc6066#section-3">RFC 6066</see>.
+     /// </summary>
+     string HostName => string.Empty;
+ #endif
}