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

Add SslStream.NegotiatedCipherSuite #34867

Closed
krwq opened this Issue Jan 25, 2019 · 13 comments

Comments

Projects
None yet
5 participants
@krwq
Copy link
Member

commented Jan 25, 2019

Spin off from: #33809 - currently we do not have agreement on how cipher suite policy should look like, this part of the feature is fairly stable and unlikely to change - regardless of how that API will look like we will need to test it and be able to provide some correct information on what exactly happened during negotiation.

Current APIs on SslStream reports MAC, key exchange and cipher but they don't work well with TLS 1.3 and with AEAD ciphers (MAC is calculated together with ciphertext) and on some platforms like OSX or Linux they do not always work correctly or report None for some of the values.

Adding this API will allow us to directly use value reported by the underlying implementation and tell correctly what the implementation did.

namespace System.Net.Security {
public partial class SslStream {
    [CLSCompliantAttribute(false)]
    public virtual System.Net.Security.TlsCipherSuite? NegotiatedCipherSuite { get { throw null; } }
}

[CLSCompliant(false)]
public enum TlsCipherSuite : ushort
{
    // reasons for using underscores and not following conventions:
    // - i.e. TLS_RSA_EXPORT_WITH_RC4_40_MD5  - 2 numbers next to each other - there must be some separator
    // - this is what IANA calls them - much easier to find considering how many values there are - having to translate between .NET <==> IANA name will be inconvenient
    TLS_NULL_WITH_NULL_NULL = 0x0000,
    TLS_RSA_WITH_NULL_MD5 = 0x0001,
    TLS_RSA_WITH_NULL_SHA = 0x0002,
    TLS_RSA_EXPORT_WITH_RC4_40_MD5 = 0x0003,
    TLS_RSA_WITH_RC4_128_MD5 = 0x0004,
    TLS_RSA_WITH_RC4_128_SHA = 0x0005,
    TLS_RSA_EXPORT_WITH_RC2_CBC_40_MD5 = 0x0006,
    TLS_RSA_WITH_IDEA_CBC_SHA = 0x0007,
    TLS_RSA_EXPORT_WITH_DES40_CBC_SHA = 0x0008,
    // ... (300 more values from IANA registry)
}
}
@davidsh

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

What is the default value of this property, SslStream.NegotiatedCipherSuite, ? I.e. when you first create the SslStream but before you do AuthenticateAsClient/AuthenticateAsServer?

Is the default value, TlsCipherSuite.TLS_NULL_WITH_NULL_NULL? Or is that value possible after a connection is made?

@krwq

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2019

@davidsh currently I've matched the behavior of the other similar fields (CipherAlgorithm) - it either throws or returns default value but I'm unsure as I can't check right now (it does CheckThrow in SslState but not sure what it currently does). I'll add a test for that. Do we have any preference? I don't have strong opinion but IMO something like InvalidOperationException feels "correct"

@davidsh

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

.NET Design guidelines frown on throwing exceptions for property getters. So, it shouldn't throw.

Since it is an enum value, it can't return null. But it should return some sentinel value that represents uninitialized.

@krwq

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2019

@davidsh, does TlsCipherSuite.TLS_NULL_WITH_NULL_NULL sound reasonable? It's hard to tell which other value to use since new values can be added in the future - other option would be to change backing type from ushort to int and use -1 for that.

@davidsh

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

other option would be to change backing type from ushort to int and use -1 for that.

Typically, .NET design guidelines don't do that usually. For non-reference types, sometimes making them nullable is the answer. I suggest getting guidance from the API design group.

@terrajobst can you comment on this?

@krwq

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2019

@davidsh I've just tested what the existing pattern is for similar fields and they throw InvalidOperationException, I'll follow that especially in this context default value will mean something else

@vcsjones

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2019

I assume that if a cipher suite is negotiated that is not in the enum, this will still work since an enum can contain any value that is of it's base type?

does TlsCipherSuite.TLS_NULL_WITH_NULL_NULL sound reasonable?

I realize that in the real world no sane client or server would negotiate this, but I feel that it is cleaner to have a distinction between uninitialized and a null cipher suite.

I've just tested what the existing pattern is for similar fields and they throw InvalidOperationException, I'll follow that

I think being consistent with the class's other members is likely the correct answer there. TlsCipherSuite? seems fine too.

@krwq

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

@vcsjones correct, values which are not in enum will work as well - AFAIR all platforms have a way of getting 16bit cipher id (Windows and Linux do, OSX almost sure it does).

@krwq

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

Hey @terrajobst what's the ETA for having this reviewed? Is there something I can do to speed this up?

@terrajobst

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Sorry for the delay; today we reviewed a bunch of back log items. We'll do the same thing next week. I'll try to make sure I'll remember this :-)

@krwq

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

Thanks :-)

@ghuntley

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

@terrajobst

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

This does NOT look good, but it seems we have issues with adjacent numbers that we'll have to to separate by underscores. Given that this means this enum will never look ".NET-ty", and that like five people in the world will care about this API, we might as well expose the IANA names.

Every time I'll be looking at this, I'll throw up in little my mouth, but hey. Approved.

We should make NegotiatedCipherSuite nullable in cases we can't determine the TLS cipher. The API might also throw if it wasn't negotiated that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.