Negotiated ciphersuite by krwq · Pull Request #35029 · dotnet/corefx

krwq · 2019-02-01T19:23:34Z

Fixes: https://github.com/dotnet/corefx/issues/34867

Motivation:
Needed for https://github.com/dotnet/corefx/issues/33809

This fixes two issues:

adds negotiated ciphersuite API
improves current reporting (gives more accurate answers or starts giving answers in some cases)
- not included: in some cases providing the answer is not possible because we do not have some enum values yet: i.e. we do not have ciphers like: Idea, Seed so we cannot provide it - in such cases None will be returned - this will need to be addressed separately but I made it as easy as possible in the code: https://github.com/dotnet/corefx/pull/35029/files#diff-2a2a56d1ddc1b36c5a8082e54a6323beR75 and also defined how the enums should look like so this can be used an API proposal: https://github.com/dotnet/corefx/pull/35029/files#diff-2a2a56d1ddc1b36c5a8082e54a6323beR11 (all 3 are in a row)

support for:

auto update (manual trigger: /p:AllowTlsCipherSuiteGeneration=true) TlsCipherSuite enum values (data downloaded from IANA registry)
parses the name to figure out what the cipher suite means (i.e. https://github.com/dotnet/corefx/pull/35029/files#diff-4ad8f316400043567d02d7755f8f191eR24 this should be audited as one time thing and then audit the diff caused by new cipher suites in the future - the template attempts to error whenever anything out of ordinary shows up so that we can audit what to do with the new thing - also error in here are not usually much harmful since we already are not too accurate with giving answer)
- when IANA responds with any RFC we don't know about the template generator will fail, RFC must be added here: https://github.com/dotnet/corefx/pull/35029/files#diff-050df69b76efda2f05cf9e1a79c5ce5cR93 - this should be audited

src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c

src/System.Net.Security/ref/System.Net.Security.csproj

src/System.Net.Security/ref/TlsCipherSuite.cs

src/System.Net.Security/ref/TlsCipherSuite.tt

karelz · 2019-03-04T05:43:42Z

Still blocked on API review + validation that Windows is implementable

krwq · 2019-03-06T21:06:19Z

@karelz to clarify - Windows is implementable (negotiated cipher suite - not policy - it was already included in the previous version) - what's missing right now is just API review (previously this was also missing fixes for OSX and merge conflict which creeped in much more stuff than I initially wanted here)

src/System.Net.Security/ref/TlsCipherSuite.tt

karelz · 2019-03-18T16:22:42Z

~~@krwq do we have final API shape yet? The associated issue is NOT marked as ready for API review ... (sorry there may have been emails about it I forgot about)~~

krwq · 2019-03-18T16:43:06Z

@karelz this is is marked as ready for review for a while (since Jan 25):
https://github.com/dotnet/corefx/issues/34867

cipher suite policy issue (https://github.com/dotnet/corefx/issues/33809) is not yet (this issue adds internals needed for cipher suite policy feature and has only APIs which are very unlikely to get changed during API review) but that APIs depend on this so cannot finalize them before this one is finalized (although I got a changeset somewhere with most likely shape on my fork)

karelz · 2019-03-18T20:55:28Z

Ah, mix up on my side, thanks for clarification!

davidsh · 2019-03-19T20:02:03Z

Given the recent API review and approval https://github.com/dotnet/corefx/issues/34867#issuecomment-474516862, will this property be changed to nullable, or just throw when not available yet?

krwq · 2019-03-21T18:43:43Z

@davidsh all similar APIs currently throw so I have followed that pattern and I think it makes sense to throw from consistency point of view (also this value doesn't make any sense before negotiation is successful so it is likely a user bug). I do not feel strongly either way though so please let me know if you prefer nullable.

khellang · 2019-03-21T19:00:23Z

I think the API review landed on throwing and nullable? Throw for invalid state, return null if the platform is unable to provide the value.

krwq · 2019-03-21T19:07:53Z

@khellang I think currently all platforms are able to provide a value - at least AFAIK - I'm not sure if the API review feedback was throw OR nullable or throw AND nullable - I don't mind either way - just feels like nullable will never be null. I will check with @terrajobst whenever I see him

khellang · 2019-03-21T19:11:08Z

I think it was @vcsjones that suggested it might be a good idea to make it nullable in case a future platform can't provide the value, instead of returning a bogus value or throwing.

krwq · 2019-03-21T19:20:11Z

makes sense - I'll double check with @terrajobst if this is what he meant as well and update accordingly (first need to fix merge conflicts and failures anyway, that will be just cosmetic change)

vcsjones · 2019-03-21T19:21:25Z

Yes I think the conversation leaned toward nullable and throw.

Throw like other properties on this class do if the stream is not in a state to provide this information.
Return null if there is any reason we can't actually return the information later.

Really my concern was, we don't own the values of the enum, IANA does, so we can never add an "Unknown" value to the enum. Making it nullable was trying to be a bit forward thinking.

src/System.Net.Security/ref/System.Net.Security.csproj

src/System.Net.Security/ref/TlsCipherSuite.cs

src/System.Net.Security/src/System.Net.Security.csproj

src/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Linux.cs

stephentoub · 2019-03-24T02:25:46Z

src/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Linux.cs

+        {
+            Protocol = (int)MapProtocolVersion(Interop.Ssl.SslGetVersion(sslContext));
+
+            MapCipherSuite(SslGetCurrentCipherSuite(sslContext));


We should be consistent: it's strange to have one of these Map... methods update state and the other not.

this was existing code I moved to different place so that we can use it on both Linux and OSX - also this maps 1 value into 6 so don't see any clean way to write that except for copy & paste

src/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Unix.cs

src/System.Net.Security/src/System/Net/Security/SslStream.cs

src/System.Net.Security/src/System/Net/Security/TlsCipherSuiteData.Lookup.cs

src/System.Net.Security/src/System/Net/Security/TlsCipherSuiteData.cs

src/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj

src/Native/Unix/System.Security.Cryptography.Native/opensslshim.h

src/System.Net.Security/ref/System.Net.Security.csproj

src/System.Net.Security/ref/System.Net.Security.cs

src/System.Net.Security/src/System/Net/Security/TlsCipherSuiteData.Lookup.tt

This reverts commit c8bffcc.

krwq · 2019-04-01T19:11:57Z

this was pure rebase to pick up new yaml files and fix the build

src/System.Net.Security/src/System.Net.Security.csproj

src/System.Net.Security/src/System/Net/Security/TlsCipherSuiteNameParser.ttinclude

bartonjs · 2019-04-02T22:27:40Z

src/System.Net.Security/src/System/Net/Security/TlsCipherSuiteNameParser.ttinclude

+
+    public static string ToFrameworkName(HashAlgorithmType val)
+    {
+        return val == HashAlgorithmType.Aead ? "None" : val.ToString();


Consider expanding this, and doing the open-drain-throw default.

currently we have 1:1 mapping except Aead so will leave it as is for simplicity

I'm not suggesting changing it to be anything complicated, just explicit-case on the 1:1 mapping (returning val.ToString() still) and letting Aead be None, and anything future throw so whoever is making the change figures out what to do about it.

The reason I brought it up is I think everything else currently throws on new kinds of data, so someone might hit a deceptive "I'm done" state instead of being really done.

will add it it in my coming ciphersuite policy PR

src/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs

* NegotiatedCiphersuite implementation * fix after merge conflict * fix netfx * add more expected Tls 1.0 ciphersuites (rfc5289) * change TlsCipherSuite API to nullable * apply self-feedback * apply feedback * Revert "change TlsCipherSuite API to nullable" This reverts commit dotnet/corefx@c8bffcc. * simplification around nullables * msbuild /t:GenerateReferenceSource instead of copy & paste * revert badly generated APIs * fix codegen after moving tt files * apply feedback Commit migrated from dotnet/corefx@f9fbe70

krwq added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 1, 2019

davidsh added the area-System.Net.Security label Feb 1, 2019

davidsh reviewed Feb 11, 2019

View reviewed changes

src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c Outdated Show resolved Hide resolved

davidsh reviewed Feb 11, 2019

View reviewed changes

src/System.Net.Security/ref/System.Net.Security.csproj Outdated Show resolved Hide resolved

davidsh reviewed Feb 11, 2019

View reviewed changes

src/System.Net.Security/ref/TlsCipherSuite.cs Outdated Show resolved Hide resolved

davidsh reviewed Feb 11, 2019

View reviewed changes

src/System.Net.Security/ref/TlsCipherSuite.tt Outdated Show resolved Hide resolved

karelz assigned krwq Mar 4, 2019

karelz added the blocked Issue/PR is blocked on something - see comments label Mar 4, 2019

krwq force-pushed the negotiated_ciphersuite branch from 6a3b296 to 4958145 Compare March 6, 2019 21:04

krwq commented Mar 6, 2019

View reviewed changes

src/System.Net.Security/ref/TlsCipherSuite.tt Outdated Show resolved Hide resolved

krwq removed the blocked Issue/PR is blocked on something - see comments label Mar 21, 2019

krwq force-pushed the negotiated_ciphersuite branch from 4958145 to 8063575 Compare March 21, 2019 22:53

krwq changed the title ~~[DO NOT MERGE] Negotiated ciphersuite~~ Negotiated ciphersuite Mar 22, 2019

krwq removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 22, 2019

krwq commented Mar 22, 2019

View reviewed changes

src/System.Net.Security/ref/System.Net.Security.csproj Outdated Show resolved Hide resolved

krwq commented Mar 22, 2019

View reviewed changes

src/System.Net.Security/ref/TlsCipherSuite.cs Outdated Show resolved Hide resolved

krwq commented Mar 22, 2019

View reviewed changes

src/System.Net.Security/src/System.Net.Security.csproj Outdated Show resolved Hide resolved

krwq commented Mar 22, 2019

View reviewed changes

src/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Linux.cs Show resolved Hide resolved