CipherSuitePolicy implementation by krwq · Pull Request #36648 · dotnet/corefx

krwq · 2019-04-06T06:50:23Z

No description provided.

krwq · 2019-04-07T05:06:52Z

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

@@ -0,0 +1,22 @@
+// Licensed to the .NET Foundation under one or more agreements.


I'm not sure what the "remove" is for. If it's the file, OK. If it's the license header, no... it should still be around.

I meant the whole file, initially it was doing a bit more but after couple of iterations it ended up being redundant...

…s enabled

bartonjs · 2019-04-08T17:59:57Z

src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs

-                        }
+                        // user explicitly asks for TLS 1.3 but their policy is not compatible with TLS 1.3
+                        throw new SslException(
+                            SR.Format(SR.net_ssl_encryptionpolicy_notsupported, policy));


This seems like a thing that we might want to change later. If I said, TLS 1.2 or TLS 1.3 and then didn't include any TLS 1.3 ciphersuites then the "TLS 1.2" part of the or still seems to apply, just as if the remote didn't support TLS 1.3, or the TLS 1.3 ciphersuites I did.

That said, if the TLS 1.3 rules are "if both sides support TLS 1.3, but can't agree on a ciphersuite, fail", then maybe this is okay?

Exception now and no-exception-later is the happier path, so this is fine if we can't justify my thought experiment.

@bartonjs actually this happens when user wants TLS1.3 (protocols include it) but user specified ciphersuites in such a way that TLS1.3 cannot happen (none TLS 1.3 ciphersuites included in a policy) - so user contradicted themselves

so user contradicted themselves

Yeah, I know. The cases where this would come up seem vanishingly small, but the easy one is trying to build something like SslLabs, where you connect, see what ciphersuite got negotiated, remove it from the allow list, and try again. When the last TLS 1.3 ciphersuite gets removed the next call throws.

The throw just means that we're pushing the knowledge of TLS 1.3 vs prior ciphersuites onto the caller. Which, given we are encouraging people to just use system defaults (after configuring their system, if they don't like the out-of-the-box answer), seems okay for now.

I got mixed feelings for that - if user doesn't want to know about TLS 1.3 they should use None for protocols in which case we won't throw.

Perhaps let's leave as is and perhaps change it when someone complains this is not intuitive?

I don't feel strongly either way - allowing this would simplify a code a bit but feels to me that asking for TLS 1.3 and not passing any ciphers which could allow us to do so is a user bug - for scenarios like SslLabs they will have to understand the cipher suites either way...

src/Native/Unix/System.Security.Cryptography.Native/configure.cmake

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

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

bartonjs · 2019-04-08T19:23:32Z

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

@@ -0,0 +1,22 @@
+// Licensed to the .NET Foundation under one or more agreements.


I'm not sure what the "remove" is for. If it's the file, OK. If it's the license header, no... it should still be around.

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

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

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

krwq · 2019-04-09T21:21:32Z

finally green on the CI. Left:

apply remaining feedback
add tests for AllowedCipherSuites property
double check tests for covering corner cases
osx implementation

krwq · 2019-04-10T18:31:00Z

/azp run

krwq · 2019-04-10T18:33:15Z

test this please

krwq · 2019-04-10T18:33:23Z

/azp help

azure-pipelines · 2019-04-10T18:33:35Z

Supported commands
     help:
          Get descriptions, examples and documentation about supported commands
          Example: help "command_name"
     run:
          Run all pipelines or a specific pipeline for this repository using a comment. Use
          this command by itself to trigger all related pipelines, or specify a pipeline
          to run.
          Example: "run" or "run pipeline_name"

See additional documentation.

krwq · 2019-04-10T18:34:00Z

/azp run corefx-ci

krwq · 2019-04-10T18:35:16Z

@safern @dotnet/dnceng how do I get this picked up by CI? this has already been running correctly, then it got green and it doesn't get picked up by CI anymore

safern · 2019-04-10T18:39:36Z

Maybe it is related to it being a draft PR? Can you try closing and reopening?

krwq · 2019-04-10T18:42:05Z

/azp run

krwq · 2019-04-10T18:42:18Z

/azp run

krwq · 2019-04-10T18:42:26Z

/azp run

krwq · 2019-04-10T18:43:34Z

@safern probably related, but seems we shouldn't be doing anything special for draft PRs, right? people use them to test stuff on CI, everything else I can do locally without opening any PR

CipherSuitePolicy implementation (Linux)

3102766

dotnet deleted a comment from azure-pipelines bot Apr 6, 2019

krwq commented Apr 7, 2019

View reviewed changes

krwq added 4 commits April 7, 2019 01:42

SSL_CIPHER_find

4323618

do not call TLS1.3 APIs on platforms which don't support it

5e3b703

Non-TLS1.3 specific tests are skipped when not enough cipher suites i…

3031634

…s enabled

clean ups

af30465

krwq requested review from GrabYourPitchforks and bartonjs April 8, 2019 17:36

krwq added 3 commits April 8, 2019 10:46

attempt to fix OSX

ef3519d

another attempt to fix OSX

1a6f1f7

missing define

80fc8a1

bartonjs reviewed Apr 8, 2019

View reviewed changes

address some feedback, try to fix test failures

9978be7

bartonjs reviewed Apr 8, 2019

View reviewed changes

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

krwq added 2 commits April 8, 2019 14:49

portable build fix

d402d7b

do not call old set ciphers API when only TLS 1.3 is requested

6e78270

apply feedback

fab7fd1

dotnet deleted a comment from azure-pipelines bot Apr 10, 2019

krwq closed this Apr 10, 2019

krwq reopened this Apr 10, 2019

krwq closed this Apr 10, 2019

krwq reopened this Apr 10, 2019

krwq closed this Apr 10, 2019

krwq mentioned this pull request Apr 10, 2019

CipherSuitePolicy implementation #36775

Merged

karelz added this to the 3.0 milestone May 22, 2019

		@@ -0,0 +1,22 @@
		// Licensed to the .NET Foundation under one or more agreements.

Conversation

krwq commented Apr 6, 2019

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

azure-pipelines bot commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

safern commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krwq commented Apr 9, 2019 •

edited

Loading