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]: Expose TLS details for QUIC connection #70184

Closed
JamesNK opened this issue Jun 2, 2022 · 13 comments
Closed

[API Proposal]: Expose TLS details for QUIC connection #70184

JamesNK opened this issue Jun 2, 2022 · 13 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Quic
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jun 2, 2022

Background and motivation

ASP.NET Core has an ITlsHandshakeFeature type that a dev can use to get information about the TLS connection. We want to support it for HTTP/3: today HTTP/3 only runs on TLS 1.3, but it'll eventually also run on whatever comes after TLS 1.3 and there needs to be a way for people to get that information.

We (asp.net core team) think that this has been discussed and brought up before but I couldn't find an issue in runtime for exposing this information. Tracking in asp.net core: dotnet/aspnetcore#35039. This doesn't block HTTP/3 for us in .NET 7 but will eventually need to be done.

API Proposal (edited by @rzikm)

The proposed API adds following two properties.

public class QuicConnection
{
    //
    // Version of TLS used for the handshake, currently expected to always return Tls13
    //
    public SslProtocols SslProtocol { get; }

    //
    // currently, QUIC allows negotiating only following TLS 1.3 ciphers
    // - TLS_AES_128_GCM_SHA256 = 0x1301, // rfc8446
    // - TLS_AES_256_GCM_SHA384 = 0x1302, // rfc8446
    // - TLS_CHACHA20_POLY1305_SHA256 = 0x1303, // rfc8446
    // - TLS_AES_128_CCM_SHA256 = 0x1304, // rfc8446
    // 
    [CLSCompliant(false)]
    public TlsCipherSuite NegotiatedCipherSuite { get; }
}

These properties contain all the relevant information.

API Usage

await using var connection = await listener.AcceptConnectionAsync();

Console.WriteLine($"connection.NegotiatedCipherSuite: {connection.NegotiatedCipherSuite}");
Console.WriteLine($"connection.SslProtocol: {connection.SslProtocol}");

Potential output:

connection.NegotiatedCipherSuite: TLS_AES_256_GCM_SHA384
connection.SslProtocol: Tls13

Alternative Designs

Put all the properties on a new class QuicConnectionInfo instead of directly on QuicConnection. This would be consistent with SslStream, but would impose an extra allocation (unless we make the new type struct). Furthermore, we already have NegotiatedApplicationProtocol on QuicConnection so it seems preferrable to have all properties directly on QuicConnection.

Decomposition of TlsCipherSuite into individual values as in SslStream

Not applicable anymore, releavant types obsoleted as of #100361

SslStream contains also additional properties which expose additional metadata about the cipher suite, however, not all of these properties make sense for QUIC, see comments inline.

public class QuicConnection
{
    // covered in the proposal at the top
    public SslProtocols SslProtocol { get; }
    public TlsCipherSuite NegotiatedCipherSuite { get; }

    // based on the above, currently expected to return
    // - Aes128
    // - Aes256
    // - None (for ChaCha20Poly1305) = based on SslConnectionInfo.Unix.cs, we probably don't have appropriate member for that. We can enhance CipherAlgorithmType enum in another proposal.
+   public CipherAlgorithmType CipherAlgorithm { get; }

    // based on the above, expected to return
    // - 128 (Aes128)
    // - 256 (Aes256 and ChaCha20Poly1305)
+   public int CipherStrength { get; }
    
    //
    // defined in SslStream but don't make that much sense as QUIC does not use the HashAlgorithm during encryption (it uses AEAD of version of the CipherAlgorithm which don't use the hash algorithm
    // from the cipher suite), and it would only be reported because it was part of the negotiated cipher suite
    //
+   public HashAlgorithmType HashAlgorithm { get; }
    // MsQuic currently returns always 0 for all ciphers, and same behavior is on SslStream as well on the cipers above (based on SslConnectionInfo.Unix.cs)
+   public int HashStrength { get; }

    //
    // based on SslConnectionInfo.Unix.cs these would return 0/None in all cases
    //
+   public ExchangeAlgorithmType KeyExchangeAlgorithm { get; }
+   public int KeyExchangeStrength { get; }
}

Risks

Possible small risk if some future version of QUIC uses different security protocol for handshake other than TLS (and we cant fit the value into SslProtocols enum)

@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http labels Jun 2, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 2, 2022
@ghost
Copy link

ghost commented Jun 2, 2022

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

Issue Details

Background and motivation

ASP.NET Core has an ITlsHandshakeFeature type that a dev can use to get information about the TLS connection. We want to support it for HTTP/3: in the future HTTP/3 will support whatever comes after TLS 1.3 and there needs to be a way for people to get that information.

We (asp.net core team) think that this has been discussed and brought up before but I couldn't find an issue in runtime for exposing this information. Tracking in asp.net core: dotnet/aspnetcore#35039

API Proposal

I don't have a concrete proposal, but I think this information should probably come from a QUIC connection. QuicConnection.NegotiatedApplicationProtocol is a similar property that exists today. Perhaps something like:

public class QuicConnection
{
    public QuicTlsDetail NegotiatedTlsDetails { get; } // new
}

public class QuicTlsDetail
{
    public SslApplicationProtocol ApplicationProtocol { get; } // if it makes sense to move
    // ...various TLS properties
}

API Usage

var connection = await listener.AcceptAsync();
var details = connection.NegotiatedTlsDetails;
// use details to populate implementation of ITlsHandshakeFeature

Alternative Designs

No response

Risks

No response

Author: JamesNK
Assignees: -
Labels:

api-suggestion, area-System.Net.Http

Milestone: -

@JamesNK
Copy link
Member Author

JamesNK commented Jun 2, 2022

@ghost
Copy link

ghost commented Jun 3, 2022

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

Issue Details

Background and motivation

ASP.NET Core has an ITlsHandshakeFeature type that a dev can use to get information about the TLS connection. We want to support it for HTTP/3: today HTTP/3 only runs on TLS 1.3, but it'll eventually also run on whatever comes after TLS 1.3 and there needs to be a way for people to get that information.

We (asp.net core team) think that this has been discussed and brought up before but I couldn't find an issue in runtime for exposing this information. Tracking in asp.net core: dotnet/aspnetcore#35039. This doesn't block HTTP/3 for us in .NET 7 but will eventually need to be done.

API Proposal

I don't have a concrete proposal, but I think this information should probably come from a QUIC connection. QuicConnection.NegotiatedApplicationProtocol is a similar property that exists today. Perhaps something like:

public class QuicConnection
{
    public QuicTlsDetail NegotiatedTlsDetails { get; } // new
}

public class QuicTlsDetail
{
    public SslApplicationProtocol ApplicationProtocol { get; } // if it makes sense to move
    // ...various TLS properties
}

API Usage

var connection = await listener.AcceptAsync();
var details = connection.NegotiatedTlsDetails;
// use details to populate implementation of ITlsHandshakeFeature

Alternative Designs

No response

Risks

No response

Author: JamesNK
Assignees: -
Labels:

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

Milestone: -

@rzikm
Copy link
Member

rzikm commented Jun 3, 2022

For comparison, for TLS, we have CipherAlgorithm and other properties directly on the SslStream class. doing the same for QuicConnection would be more consistent.

The underlying info is already exposed by MsQuic via GetParam QUIC_PARAM_TLS_HANDSHAKE_INFO, so there should not be any additional work needed from their side.

@wfurt
Copy link
Member

wfurt commented Mar 17, 2023

I think SslStream is like that for historical reasons. The properties like negotiated protocol version, and then eventually ALPN and cipher site were added over time. While having container for negotiated info looks good, I don't think it brings significant benefits over adding it to base class e.g. QuicConnection (or SslStream)
And proposed QuicTlsDetail is class so we will have to allocate if somebody just wants to know something basic.

And there are issues IMHO like #37578 where the info become useless for commonly used GCM mode. It would make most sense to me to expose SslStream.NegotiatedCipherSuite as that is true unite and accurate info about the handshake but the ITlsHandshakeFeature is missing that.

My suggestions would be to add Protocol (or SslProtocol or TlsProtocol) and NegotiatedCipherSuite to QuicConnection. It does not requite ay allocation or expensive processing. Everything else can be derived from it.

We could skip the Protocol as right now it would always be Tls13 but it may make sense to keep it for consistency.

Thoughts @JamesNK @Tratcher ?

@Tratcher
Copy link
Member

Tratcher commented Mar 18, 2023

It's better to add these fields now so they're available later when we need them. Especially since when there is a new TLS version it will likely be an internal/OS change, not something at the API level.

Is there an easy API take NegotiatedCipherSuite and break it down into parts? I asked SChannel recently for the opposite API and it doesn't exist.

@rzikm
Copy link
Member

rzikm commented Mar 29, 2023

Is there an easy API take NegotiatedCipherSuite and break it down into parts? I asked SChannel recently for the opposite API and it doesn't exist.

I don't think there is public API, but internally, we have

public static TlsCipherSuiteData GetCipherSuiteData(TlsCipherSuite cipherSuite)

It is internally used for SslStream on Unixes to populate the TLS-related properties.

@Tratcher
Copy link
Member

Ha, that's exactly the code I don't want to duplicate. Maybe I'll convince you to put it in shared source like we do for HTTP/2?

@wfurt
Copy link
Member

wfurt commented Apr 1, 2023

shared location may be good. We may have some updates so we can perhaps do it together.

@rzikm
Copy link
Member

rzikm commented Feb 28, 2024

To push this a bit more forward, I like the idea of putting out only the NegotiatedCipherSuite and SslProtocol, and moving cipher decomposition to source shared with asp.net. I will update the API proposal.

If all is good, I will put this for API Review.

@wfurt
Copy link
Member

wfurt commented Mar 15, 2024

looks good to me. I feel that would be good start and we can work out the shared component. We can always add something more later if needed or if the shard approach fails for whatever reason.

@rzikm rzikm added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Mar 18, 2024
@liveans liveans removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 18, 2024
@terrajobst
Copy link
Member

terrajobst commented Aug 13, 2024

Video

  • Looks good as proposed
namespace System.Net.Quic;

public partial class QuicConnection
{
    public SslProtocols SslProtocol { get; }

    [CLSCompliant(false)]
    public TlsCipherSuite NegotiatedCipherSuite { get; }
}

@terrajobst terrajobst 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 Aug 13, 2024
@rzikm
Copy link
Member

rzikm commented Aug 19, 2024

Implemented in #106391

@rzikm rzikm closed this as completed Aug 19, 2024
@karelz karelz modified the milestones: Future, 9.0.0 Sep 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
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

No branches or pull requests

9 participants