Adding documentation for new CipherSuitesPolicy, SslStream.NegotiatedCipherSuite and TlsCipherSuites #2822

jozkee · 2019-07-24T21:18:33Z

This completes System.Net.Security namespace by documenting the following:

CipherSuitesPolicy
- .ctor
- AllowedCipherSuites
SslStream
- NegotiatedCipherSuite
TlsCipherSuites
- ~340 enums representing cipher suites

…CipherSuite and TlsCipherSuite.

jozkee · 2019-07-24T21:19:53Z

@dotnet/ncl @krwq @bartonjs
@mairaw @rpetrusha @carlossanlop

xml/System.Net.Security/CipherSuitesPolicy.xml

krwq · 2019-07-24T21:32:11Z

xml/System.Net.Security/CipherSuitesPolicy.xml

+        <remarks>
+          <format type="text/markdown"><![CDATA[  
+> [!NOTE]
+> Defining a chipher suite policy on <xref:System.Net.Security.SslStream> authentication will prevent the OS of the ability to decide which are the best cipher suites to negotiate with and will need of your attention to manually check and update this code. It is strongly recommended to hold off the use of this property and rely on your constantly updated OS policy.


typo: chipher -> cipher

krwq · 2019-07-24T21:36:22Z

xml/System.Net.Security/CipherSuitesPolicy.xml

+        <remarks>
+          <format type="text/markdown"><![CDATA[  
+> [!NOTE]
+> Defining a chipher suite policy on <xref:System.Net.Security.SslStream> authentication will prevent the OS of the ability to decide which are the best cipher suites to negotiate with and will need of your attention to manually check and update this code. It is strongly recommended to hold off the use of this property and rely on your constantly updated OS policy.


OS is allowed to further restrict the list so it doesn't strictly speaking prevent OS from deciding (implementation will make the last call) but if new cipher suite shows up in the updated version of the underlying implementation a user won't get it by default (which in some cases might or might not be expected).

Also OS should be replaced with underlying implementation

krwq · 2019-07-24T21:37:03Z

xml/System.Net.Security/CipherSuitesPolicy.xml

+> Defining a chipher suite policy on <xref:System.Net.Security.SslStream> authentication will prevent the OS of the ability to decide which are the best cipher suites to negotiate with and will need of your attention to manually check and update this code. It is strongly recommended to hold off the use of this property and rely on your constantly updated OS policy.
+
+## Remarks
+Unlike the OS underlying SSL/TLS implementation, the order of the <xref:System.Net.Security.TlsCipherSuite> elements in `allowedCipherSuites` does not determine their priority in the client-server negotiation. It is required to use OSX or Unix-based systems with OpenSSL 1.1.1 in order to initialize an instance of this class.


I'd drop Unlike the OS underlying SSL/TLS implementation - not sure what that part means

does not determine their priority => does not guarantee their priority

xml/System.Net.Security/CipherSuitesPolicy.xml

bartonjs · 2019-07-24T21:27:00Z

xml/System.Net.Security/CipherSuitesPolicy.xml

+> Defining a chipher suite policy on <xref:System.Net.Security.SslStream> authentication will prevent the OS of the ability to decide which are the best cipher suites to negotiate with and will need of your attention to manually check and update this code. It is strongly recommended to hold off the use of this property and rely on your constantly updated OS policy.
+
+## Remarks
+Unlike the OS underlying SSL/TLS implementation, the order of the <xref:System.Net.Security.TlsCipherSuite> elements in `allowedCipherSuites` does not determine their priority in the client-server negotiation. It is required to use OSX or Unix-based systems with OpenSSL 1.1.1 in order to initialize an instance of this class.


I think the "or" here is lexically ambiguous. IIRC it requires ((macOS) or (Linux with OpenSSL 1.1.1)), but it's possible to parse as ((macOS or Linux) with OpenSSL 1.1.1).

It probably needs some sort of style bubble for OS-specific requirements. Something like

[!IMPORTANT] This feature is dependent on platform support. * Microsoft Windows: This feature is not available. * macOS: This feature is available. * Linux: This feature is available on systems with OpenSSL 1.1.1 or higher.

But I'd wait for a writer before changing the format.

I left a suggestion above. See what you think. I've changed from Unix-based to Linux since macOS it's also Unix-based.

xml/System.Net.Security/CipherSuitesPolicy.xml

bartonjs · 2019-07-24T21:28:44Z

xml/System.Net.Security/CipherSuitesPolicy.xml

+> Defining a chipher suite policy on <xref:System.Net.Security.SslStream> authentication will prevent the OS of the ability to decide which are the best cipher suites to negotiate with and will need of your attention to manually check and update this code. It is strongly recommended to hold off the use of this property and rely on your constantly updated OS policy.
+
+## Remarks
+Unlike the OS underlying SSL/TLS implementation, the order of the <xref:System.Net.Security.TlsCipherSuite> elements in `allowedCipherSuites` does not determine their priority in the client-server negotiation. It is required to use OSX or Unix-based systems with OpenSSL 1.1.1 in order to initialize an instance of this class.


Does it actually not determine the order? I thought it did on macOS and Linux (but on Windows it won't).

on OSX I think it does, on Linux it will do some re-ordering (TLS1.3 cipher suites will go first, possibly some null ciphers will go to the back - can't remember the details) but mostly preserves the order. I'd just keep the sentence to order is not guaranteed (or we will do best effort or something along these lines)

xml/System.Net.Security/SslStream.xml

xml/System.Net.Security/TlsCipherSuite.xml

xml/System.Net.Security/CipherSuitesPolicy.xml

krwq · 2019-07-24T21:44:48Z

xml/System.Net.Security/SslStream.xml

-        <summary>To be added.</summary>
-        <value>To be added.</value>
-        <remarks>To be added.</remarks>
+        <summary>Gets the cipher suite resolved to use by this <see cref="T:System.Net.Security.SslStream"/>.</summary>


This is a cipher suite which was already negotiated and is already being used. Comment could be something like:

Cipher suite picked by the server from the client supplied list of allowed cipher suites

with Gets prefix per @bartonjs comment

xml/System.Net.Security/SslStream.xml

krwq · 2019-07-24T21:48:57Z

xml/System.Net.Security/SslStream.xml

+          <format type="text/markdown"><![CDATA[  
+  
+## Remarks
+This property gets the cipher suite that is going to be used in the communication between a client and server by the underlying SSL/TLS implementation. This property is only available after a successful call to <xref:System.Net.Security.SslStream.AuthenticateAsServer%2A> or <xref:System.Net.Security.SslStream.AuthenticateAsClient%2A>.


what's the %2A for in the xref?

URL-encoded star (*), I think is used to refer to the method's name without referring to a specific signature.

what's the %2A for in the xref?

I think is used to refer to the method's name without referring to a specific signature.

Yeah, targets the method group (full overload set) rather than a specific method.

It's when you want to link to a method in general and not a specific overload. More info at https://github.com/dotnet/docs/blob/master/styleguide/template.md#links-to-apis

xml/System.Net.Security/SslStream.xml

krwq · 2019-07-24T21:52:02Z

xml/System.Net.Security/TlsCipherSuite.xml

@@ -39,7 +46,7 @@
      </ReturnValue>
      <MemberValue>4869</MemberValue>
      <Docs>
-        <summary>To be added.</summary>
+        <summary>Represents the TLS_AES_128_CCM_8_SHA256 cipher suite.</summary>


not checking anything after here, let me know if there is something there to check

jozkee · 2019-07-24T22:08:21Z

xml/System.Net.Security/TlsCipherSuite.xml

@@ -5019,7 +5026,7 @@
      </ReturnValue>
      <MemberValue>0</MemberValue>
      <Docs>
-        <summary>To be added.</summary>
+        <summary>Represents the TLS_NULL_WITH_NULL_NULL cipher suite; this is the default value.</summary>


@krwq this might worth reviewing. In this field I am stating TLS_NULL_WITH_NULL_NULL is enum's default value; not sure if it should really be included.

it is default when you do default(TlsCipherSuite) solely because enums always default to 0 which happens to map to this cipher suite. For the AllowedCipherSuites you will get whatever you pass in and there is only custom or OS default options

mairaw

A few comments

xml/System.Net.Security/CipherSuitesPolicy.xml

mairaw · 2019-07-24T22:12:53Z

xml/System.Net.Security/CipherSuitesPolicy.xml

+> Defining a chipher suite policy on <xref:System.Net.Security.SslStream> authentication will prevent the OS of the ability to decide which are the best cipher suites to negotiate with and will need of your attention to manually check and update this code. It is strongly recommended to hold off the use of this property and rely on your constantly updated OS policy.
+
+## Remarks
+Unlike the OS underlying SSL/TLS implementation, the order of the <xref:System.Net.Security.TlsCipherSuite> elements in `allowedCipherSuites` does not determine their priority in the client-server negotiation. It is required to use OSX or Unix-based systems with OpenSSL 1.1.1 in order to initialize an instance of this class.


I left a suggestion above. See what you think. I've changed from Unix-based to Linux since macOS it's also Unix-based.

xml/System.Net.Security/CipherSuitesPolicy.xml

xml/System.Net.Security/SslStream.xml

mairaw · 2019-07-24T22:26:37Z

xml/System.Net.Security/SslStream.xml

+          <format type="text/markdown"><![CDATA[  
+  
+## Remarks
+This property gets the cipher suite that is going to be used in the communication between a client and server by the underlying SSL/TLS implementation. This property is only available after a successful call to <xref:System.Net.Security.SslStream.AuthenticateAsServer%2A> or <xref:System.Net.Security.SslStream.AuthenticateAsClient%2A>.


It's when you want to link to a method in general and not a specific overload. More info at https://github.com/dotnet/docs/blob/master/styleguide/template.md#links-to-apis

xml/System.Net.Security/TlsCipherSuite.xml

* Adding some changes of my own as well. Co-Authored-By: Maira Wenzel <[email protected]> Co-Authored-By: Jeremy Barton <[email protected]>

jozkee · 2019-07-30T22:54:48Z

@mairaw @rpetrusha @bartonjs @krwq @carlossanlop
I committed the suggestions a few days ago, this should be ready for re-review or probably merge.

krwq · 2019-07-30T23:49:57Z

@jozkee I haven't seen corefx PR updating xml doc comments

jozkee · 2019-07-31T22:16:38Z

@krwq I will do it soon, but that shouldn't stop this PR from merging.
@mairaw @rpetrusha can we merge this?

krwq

LGTM content wise (please get additional sign-off for grammar from some native speaker)

carlossanlop · 2019-08-06T19:23:31Z

@mairaw @rpetrusha can we get this PR merged?

rpetrusha · 2019-08-06T21:02:36Z

I'll review it quickly now.

rpetrusha

I've left some comments, suggestions, and a question for you to consider, @jozkee.

xml/System.Net.Security/CipherSuitesPolicy.xml

xml/System.Net.Security/SslStream.xml

xml/System.Net.Security/TlsCipherSuite.xml

jozkee

Addressing suggestions for AllowedCipherSuites remarks.

xml/System.Net.Security/CipherSuitesPolicy.xml

Co-Authored-By: Ron Petrusha <[email protected]>

jozkee · 2019-08-06T23:21:37Z

@rpetrusha I just addressed your latest suggestions. I think this should be ready to merge once the build passes.

carlossanlop · 2019-08-07T17:57:26Z

@rpetrusha the build passed. Is this good to merge?

rpetrusha · 2019-08-07T18:06:25Z

Yes, this is ready to merge, @jozkee and @carlossanlop. I'll do it now.

Adding documentation for new CipherSuitesPolicy, SslStream.Negotiated…

1efb3db

…CipherSuite and TlsCipherSuite.

jozkee requested a review from karelz as a code owner July 24, 2019 21:18

krwq reviewed Jul 24, 2019

View reviewed changes

xml/System.Net.Security/CipherSuitesPolicy.xml Show resolved Hide resolved

krwq reviewed Jul 24, 2019

View reviewed changes

xml/System.Net.Security/CipherSuitesPolicy.xml Show resolved Hide resolved

krwq reviewed Jul 24, 2019

View reviewed changes

carlossanlop requested review from bartonjs, carlossanlop, mairaw, rpetrusha and scalablecory July 24, 2019 21:37

carlossanlop assigned jozkee Jul 24, 2019

carlossanlop added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles labels Jul 24, 2019

krwq reviewed Jul 24, 2019

View reviewed changes

xml/System.Net.Security/CipherSuitesPolicy.xml Outdated Show resolved Hide resolved

bartonjs reviewed Jul 24, 2019

View reviewed changes

krwq reviewed Jul 24, 2019

View reviewed changes

xml/System.Net.Security/CipherSuitesPolicy.xml Outdated Show resolved Hide resolved

krwq reviewed Jul 24, 2019

View reviewed changes

xml/System.Net.Security/SslStream.xml Outdated Show resolved Hide resolved

krwq reviewed Jul 24, 2019

View reviewed changes

xml/System.Net.Security/SslStream.xml Show resolved Hide resolved

krwq reviewed Jul 24, 2019

View reviewed changes

mairaw added this to the July 2019 milestone Jul 24, 2019

jozkee commented Jul 24, 2019

View reviewed changes

mairaw reviewed Jul 24, 2019

View reviewed changes

mairaw removed the request for review from rpetrusha July 24, 2019 22:28

bartonjs reviewed Jul 25, 2019

View reviewed changes

xml/System.Net.Security/TlsCipherSuite.xml Outdated Show resolved Hide resolved

Apply suggestions from code review

5f283d5

* Adding some changes of my own as well. Co-Authored-By: Maira Wenzel <[email protected]> Co-Authored-By: Jeremy Barton <[email protected]>

carlossanlop requested review from davidsh and wfurt July 29, 2019 16:58

krwq approved these changes Aug 1, 2019

View reviewed changes

jozkee mentioned this pull request Aug 3, 2019

Added triple-slash summary to TlsCipherSuite.tt template. dotnet/corefx#39990

Merged

mairaw added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Aug 6, 2019

carlossanlop added the verify-build-before-merge label Aug 6, 2019

rpetrusha suggested changes Aug 6, 2019

View reviewed changes

mairaw removed changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review verify-build-before-merge labels Aug 6, 2019

jozkee commented Aug 6, 2019

View reviewed changes

xml/System.Net.Security/CipherSuitesPolicy.xml Outdated Show resolved Hide resolved

xml/System.Net.Security/CipherSuitesPolicy.xml Outdated Show resolved Hide resolved

jozkee commented Aug 6, 2019

View reviewed changes

xml/System.Net.Security/CipherSuitesPolicy.xml Outdated Show resolved Hide resolved

Apply suggestions from code review

4b4276c

Co-Authored-By: Ron Petrusha <[email protected]>

mairaw added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Aug 6, 2019

carlossanlop added verify-build-before-merge and removed changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review labels Aug 7, 2019

rpetrusha approved these changes Aug 7, 2019

View reviewed changes

rpetrusha merged commit dc0d883 into dotnet:master Aug 7, 2019

mairaw removed the verify-build-before-merge label Aug 9, 2019

Adding documentation for new CipherSuitesPolicy, SslStream.NegotiatedCipherSuite and TlsCipherSuites #2822

Adding documentation for new CipherSuitesPolicy, SslStream.NegotiatedCipherSuite and TlsCipherSuites #2822

Uh oh!

Conversation

jozkee commented Jul 24, 2019

Uh oh!

jozkee commented Jul 24, 2019

Uh oh!

Uh oh!

Uh oh!

krwq Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mairaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jozkee commented Jul 30, 2019

Uh oh!

krwq commented Jul 30, 2019

Uh oh!

jozkee commented Jul 31, 2019

Uh oh!

krwq Jul 24, 2019 •

edited

Loading