Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Add TargetHostName to QuicConnection #80508

Closed
EatonZ opened this issue Jan 11, 2023 · 9 comments · Fixed by #84976
Closed

[API Proposal]: Add TargetHostName to QuicConnection #80508

EatonZ opened this issue Jan 11, 2023 · 9 comments · Fixed by #84976
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Quic
Milestone

Comments

@EatonZ
Copy link
Contributor

EatonZ commented Jan 11, 2023

Background and motivation

Consider the following code:

var handler = new SocketsHttpHandler
{
    SslOptions = new SslClientAuthenticationOptions { RemoteCertificateValidationCallback = HandlePinningPolicy }
};
var httpClient = new HttpClient(handler);
private static bool HandlePinningPolicy(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
    if (sslPolicyErrors != SslPolicyErrors.None) return false;

    if (PinList != null && PinList.TryGetValue(((SslStream)sender).TargetHostName, out var pin)) return new Span<byte>(pin).SequenceEqual(SHA256.HashData(certificate.GetPublicKey()));

    return true;
}

On HTTP 1 and 2 connections, that all works perfectly fine. With .NET 7 and HTTP 3, I noticed HandlePinningPolicy was throwing an exception:

Unable to cast object of type 'System.Net.Quic.QuicConnection' to type 'System.Net.Security.SslStream'.

The problem is obvious, so I decided to add a check for when the sender is QuicConnection. Just one problem though.. QuicConnection doesn't provide the host name of the connection, and I do not see any way to get it:

QuicConnection

As a result, pinning HTTP 3 connections seems impossible.

Any temporary workaround would be appreciated.🙂

API Proposal

namespace System.Net.Quic;

public sealed partial class QuicConnection : IAsyncDisposable
{
+    public string TargetHostName { ... }
}

API Usage

if (PinList != null && PinList.TryGetValue(sender is QuicConnection qc ? qc.TargetHostName : ((SslStream)sender).TargetHostName, out var pin)) return new Span<byte>(pin).SequenceEqual(SHA256.HashData(certificate.GetPublicKey()));

Alternative Designs

No response

Risks

None

@EatonZ EatonZ added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 11, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 11, 2023
@ghost
Copy link

ghost commented Jan 11, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Consider the following code:

var handler = new SocketsHttpHandler
{
    SslOptions = new SslClientAuthenticationOptions { RemoteCertificateValidationCallback = HandlePinningPolicy }
};
var httpClient = new HttpClient(handler);
private static bool HandlePinningPolicy(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
    if (sslPolicyErrors != SslPolicyErrors.None) return false;

    if (PinList != null && PinList.TryGetValue(((SslStream)sender).TargetHostName, out var pin)) return new Span<byte>(pin).SequenceEqual(SHA256.HashData(certificate.GetPublicKey()));

    return true;
}

On HTTP 1 and 2 connections, that all works perfectly fine. With .NET 7 and HTTP 3, I noticed HandlePinningPolicy was throwing an exception:

Unable to cast object of type 'System.Net.Quic.QuicConnection' to type 'System.Net.Security.SslStream'.

The problem is obvious, so I decided to add a check for when the sender is QuicConnection. Just one problem though.. QuicConnection doesn't provide the host name of the connection, and I do not see any way to get it:

QuicConnection

As a result, pinning HTTP 3 connections seems impossible.

Any temporary workaround would be appreciated.🙂

API Proposal

namespace System.Net.Quic;

public sealed partial class QuicConnection : IAsyncDisposable
{
+    public string TargetHostName { ... }
}

API Usage

if (PinList != null && PinList.TryGetValue(sender is QuicConnection qc ? qc.TargetHostName : ((SslStream)sender).TargetHostName, out var pin)) return new Span<byte>(pin).SequenceEqual(SHA256.HashData(certificate.GetPublicKey()));

Alternative Designs

No response

Risks

None

Author: EatonZ
Assignees: -
Labels:

api-suggestion, area-System.Net, untriaged

Milestone: -

@wfurt
Copy link
Member

wfurt commented Jan 11, 2023

This would make sense to me. It seems like we already have _targetHost inside of private SslConnectionOptions.
You may be able to get to it with little bit of Reflection magic @EatonZ until there is better way.

@EatonZ
Copy link
Contributor Author

EatonZ commented Jan 11, 2023

@wfurt Thank you for the tip. I came up with this, which is an acceptable workaround for now. Hopefully you will consider my proposal so this can be cleaned up later.🙂

string targetHost;
if (sender is QuicConnection qc)
{
    var _sslConnectionOptions = qc.GetType().GetField("_sslConnectionOptions", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(qc);
    targetHost = (string)_sslConnectionOptions.GetType().GetField("_targetHost", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(_sslConnectionOptions);
}
else targetHost = ((SslStream)sender).TargetHostName;

@wfurt
Copy link
Member

wfurt commented Jan 12, 2023

For the record, here is #27619 original ask for SslStream.

@ghost
Copy link

ghost commented Jan 12, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Consider the following code:

var handler = new SocketsHttpHandler
{
    SslOptions = new SslClientAuthenticationOptions { RemoteCertificateValidationCallback = HandlePinningPolicy }
};
var httpClient = new HttpClient(handler);
private static bool HandlePinningPolicy(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
{
    if (sslPolicyErrors != SslPolicyErrors.None) return false;

    if (PinList != null && PinList.TryGetValue(((SslStream)sender).TargetHostName, out var pin)) return new Span<byte>(pin).SequenceEqual(SHA256.HashData(certificate.GetPublicKey()));

    return true;
}

On HTTP 1 and 2 connections, that all works perfectly fine. With .NET 7 and HTTP 3, I noticed HandlePinningPolicy was throwing an exception:

Unable to cast object of type 'System.Net.Quic.QuicConnection' to type 'System.Net.Security.SslStream'.

The problem is obvious, so I decided to add a check for when the sender is QuicConnection. Just one problem though.. QuicConnection doesn't provide the host name of the connection, and I do not see any way to get it:

QuicConnection

As a result, pinning HTTP 3 connections seems impossible.

Any temporary workaround would be appreciated.🙂

API Proposal

namespace System.Net.Quic;

public sealed partial class QuicConnection : IAsyncDisposable
{
+    public string TargetHostName { ... }
}

API Usage

if (PinList != null && PinList.TryGetValue(sender is QuicConnection qc ? qc.TargetHostName : ((SslStream)sender).TargetHostName, out var pin)) return new Span<byte>(pin).SequenceEqual(SHA256.HashData(certificate.GetPublicKey()));

Alternative Designs

No response

Risks

None

Author: EatonZ
Assignees: -
Labels:

api-suggestion, untriaged, area-System.Net.Quic

Milestone: -

@ManickaP
Copy link
Member

Triage: this seems fairly easy so we could do it in 8.0. And we have precedent in SslStream.

@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Jan 12, 2023
@ManickaP ManickaP added this to the 8.0.0 milestone Jan 12, 2023
@ManickaP
Copy link
Member

Related issue: #70184

@rzikm
Copy link
Member

rzikm commented Feb 6, 2023

Related: We might want to not pass IP literals as ServerName to MsQuicConnectionStart, On Linux, we may end up diverging from RFC. see #81590 (comment).

IntPtr targetHostPtr = Marshal.StringToCoTaskMemUTF8(options.ClientAuthenticationOptions.TargetHost ?? host ?? address?.ToString());

@wfurt wfurt self-assigned this Mar 16, 2023
@wfurt wfurt added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 17, 2023
@bartonjs
Copy link
Member

bartonjs commented Apr 13, 2023

Video

Looks good as proposed (assuming "..." was "get;")

namespace System.Net.Quic;

public sealed partial class QuicConnection : IAsyncDisposable
{
     public string TargetHostName { get; }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 13, 2023
@rzikm rzikm assigned rzikm and unassigned wfurt Apr 18, 2023
rzikm added a commit to rzikm/dotnet-runtime that referenced this issue Apr 18, 2023
rzikm added a commit that referenced this issue Apr 25, 2023
* Add TargetHostName to QuicConnection
Fixes #80508

* Make TargetHostName not nullable

* Fix build

* Fix build of tests

* Fix failing tests

* Code review feedback

* Use unencoded hostname in user-facing properties/params

* Fix failing tests

* Revert unwanted changes

* Add test for IDN cert validation

* Fix test again

* Fix trailing dot in hostname
@ghost ghost locked as resolved and limited conversation to collaborators May 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Quic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants