Implement client certificate authentication #351

tmds · 2015-11-10T17:49:34Z

Implement client certificate authentication (#332)

Task.Run eventually ends up being QueueUserWorkItem. The returned task is ignored, so no added goodness. Short running item. Cut out the middleman

dnfclas · 2015-11-10T17:49:38Z

Hi @tmds, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

davidfowl · 2015-11-10T17:52:15Z

src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs

@@ -14,6 +14,8 @@
 using Microsoft.AspNet.Server.Kestrel.Infrastructure;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Primitives;
+using System.Security.Cryptography.X509Certificates;
+using Microsoft.AspNet.Http.Features.Internal;


Sort usings

davidfowl · 2015-11-10T17:55:15Z

@tmds Take a look at the appveyor failure

Tratcher · 2015-11-10T18:04:06Z

src/Microsoft.AspNet.Server.Kestrel.Https/HttpsApplicationBuilderExtensions.cs

@@ -21,7 +26,7 @@ public static IApplicationBuilder UseKestrelHttps(this IApplicationBuilder app,

            var prevFilter = serverInfo.ConnectionFilter ?? new NoOpConnectionFilter();

-            serverInfo.ConnectionFilter = new HttpsConnectionFilter(cert, prevFilter);
+            serverInfo.ConnectionFilter = new HttpsConnectionFilter(cert, mode, prevFilter);


HttpsConnectionFilter really should have an Options class like we do with most middleware.

https://github.com/aspnet/KestrelHttpServer/issues/241

Tratcher · 2015-11-10T18:22:48Z

I think we need to make the IConnectionFilter abstraction more powerful so all of the client cert work can happen in the https package through extension points. @halter73

await rather than synchronously blocking and then returning a Task.FromResult

…leanup

…otnet5 SslStream does)

Resolves #339

- This should stop the AVs we've been seeing in some of our test runs

…not provide a certificate and mode is set to Allow/RequireCertificate

…ertificate is passed to the HttpContext

tmds · 2015-11-12T23:11:01Z

@davidfowl @Tratcher can you do another review?
Can you check these things in particular:

Should I remove this overload: UseKestrelHttps(this IApplicationBuilder app, X509Certificate2 cert)?
Are the 'names' okay?
Should the HttpsConnectionFilterOptions contain a pattern which makes specifying the server certificate mandatory (e.g. constructor argument)?
Is it okay to use TlsConnectionFeature from Microsoft.AspNet.Http.Features.Internal?

halter73 · 2015-11-13T22:30:48Z

test/Microsoft.AspNet.Server.KestrelTests/HttpsConnectionFilterTests.cs

@@ -2,13 +2,18 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

 #if DNX451


This is looking good. Thanks for adding the HttpsConnectionFilterOptions.

Recently we enabled these HttpsConnectionFilterTests on CoreClr since SslStream is now available for that.

Can you rebase this PR on dev and verify that client certificates work with CoreClr? Thanks!

It doesn't work on CoreClr. The ssl validation callback gets a X509Certificate and the constructor X509Certificate2(X509Certificate) to convert it doesn't exist (yet).

I created an issue: https://github.com/dotnet/corefx/issues/4510

And another one: https://github.com/dotnet/corefx/issues/4512

…tionFeature in HttpsConnectionFilter

…leanup

…otnet5 SslStream does)

…not provide a certificate and mode is set to Allow/RequireCertificate

…ertificate is passed to the HttpContext

…tionFeature in HttpsConnectionFilter

tmds · 2015-11-14T04:23:10Z

ok, rebasing did not work as expected...

tmds · 2015-11-14T04:40:10Z

I guess I did something wrong...
How can I make this show only the changes that are part of the pull request? And not those that I got by rebasing on dev?
What should I have done to rebase to not get in this situation?

shahid-pk · 2015-11-14T08:53:32Z

@tmnds do this
git checkout client_certificate
git rebase HEAD~37 -i

you will get a prompt squash all 36 comments only leave the one that you want and then resolve merge conflicts if their are any, and you are ready for the last step ... Note: if origin refers to your kestrel fork otherwise replace origin accordingly.

git push origin client_certificate --force

tmds · 2015-11-14T10:12:14Z

I made another pull request to replace this one #385

@shahid-pk thanks for your suggestion. I was unsure how to apply it. Executing the rebase command gave me a list of all commits. About half of these commits were part of the pull request, the other half came from 'rebasing'. It isn't clear to me what I should should select for those type of commits: pick, squash, .... I've read on stackoverflow I should have done a 'force push' to avoid combining the two histories.

benaadams and others added 3 commits November 10, 2015 07:28

Task.Run -> ThreadPool.QueueUserWorkItem

a2c4e3a

Task.Run eventually ends up being QueueUserWorkItem. The returned task is ignored, so no added goodness. Short running item. Cut out the middleman

Move logging to new style

c0f8276

Implement client certificate authentication

fb74658

dnfclas added the cla-already-signed label Nov 10, 2015

davidfowl reviewed Nov 10, 2015
View reviewed changes

Tratcher reviewed Nov 10, 2015
View reviewed changes

benaadams and others added 11 commits November 10, 2015 18:23

Remove sync block->task

344c821

await rather than synchronously blocking and then returning a Task.FromResult

Pass around ITlsConnectionFeature instead of X509Certificate2, code c…

48b7b79

…leanup

SslProtocols.Defaults not available on all platforms, use Tls (like d…

26ff03d

…otnet5 SslStream does)

Add HttpsConnectionFilterOptions

449b303

.Result -> .GetAwaiter().GetResult()

0500043

Slow date wait for CI

ade0dba

Resolves #339

Remove UvAsyncHandle.DangerousClose

e5ff33e

- This should stop the AVs we've been seeing in some of our test runs

Remove System beta tag in project.json for coreclr packages.

86e1924

Rewrite certificate validation callback, add tests where client does …

358e94f

…not provide a certificate and mode is set to Allow/RequireCertificate

Add ClientCertificateValidationCallback option, add test to see the c…

9e62d65

…ertificate is passed to the HttpContext

Code cleanup

cee178f

halter73 added 3 commits November 12, 2015 15:51

Merge branch 'benaadams/Task.Run-QueueUserWorkItem' into dev

fdfd7dc

Merge branch 'benaadams/reduce-log-boxing' into dev

0ead246

Merge branch 'benaadams/result-getawaiter' into dev

9958427

halter73 and others added 8 commits November 12, 2015 16:15

Merge branch 'nickcraver/cancellation' into dev

983997a

Merge branch 'benaadams/slower-ci-date' into dev

8ad7b71

Merge branch 'benaadams/odd-sync' into dev

6b99536

Don't pre-complete too many writes

59cdd60

Avoid some closure allocations in SocketOutput

fc346f7

Update project.json

52fe469

Skip some tests on Mono to prevent CI hangs.

dd1ffa5

Remove unneeded dependency System.Net.NameResolution.

2ac5e4c

halter73 reviewed Nov 13, 2015
View reviewed changes

Cesar Blum Silveira and others added 12 commits November 13, 2015 16:05

Reset Frame.Scheme on each request (#366).

a3a49d2

Add IConnectionFilter.PrepareRequest and use it to set the ITlsConnec…

5c52b8a

…tionFeature in HttpsConnectionFilter

Implement client certificate authentication

bed31a3

Pass around ITlsConnectionFeature instead of X509Certificate2, code c…

8e56ef9

…leanup

SslProtocols.Defaults not available on all platforms, use Tls (like d…

f6ca379

…otnet5 SslStream does)

Add HttpsConnectionFilterOptions

6e5ea33

Rewrite certificate validation callback, add tests where client does …

35ef29f

…not provide a certificate and mode is set to Allow/RequireCertificate

Add ClientCertificateValidationCallback option, add test to see the c…

ea039ac

…ertificate is passed to the HttpContext

Code cleanup

d489e11

Add IConnectionFilter.PrepareRequest and use it to set the ITlsConnec…

1940c3a

…tionFeature in HttpsConnectionFilter

Merge with dev, check client certificate on CoreCLR

4a940bd

Merge

060c1e8

tmds mentioned this pull request Nov 14, 2015

Implement client certificate authentication #385

Merged

tmds closed this Nov 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement client certificate authentication #351

Implement client certificate authentication #351

tmds commented Nov 10, 2015

dnfclas commented Nov 10, 2015

davidfowl Nov 10, 2015

davidfowl commented Nov 10, 2015

Tratcher Nov 10, 2015

Tratcher Nov 10, 2015

Tratcher commented Nov 10, 2015

tmds commented Nov 12, 2015

halter73 Nov 13, 2015

tmds Nov 14, 2015

tmds Nov 14, 2015

tmds commented Nov 14, 2015

tmds commented Nov 14, 2015

shahid-pk commented Nov 14, 2015

tmds commented Nov 14, 2015

		@@ -2,13 +2,18 @@
		// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

		#if DNX451

Implement client certificate authentication #351

Implement client certificate authentication #351

Conversation

tmds commented Nov 10, 2015

dnfclas commented Nov 10, 2015

davidfowl Nov 10, 2015

Choose a reason for hiding this comment

davidfowl commented Nov 10, 2015

Tratcher Nov 10, 2015

Choose a reason for hiding this comment

Tratcher Nov 10, 2015

Choose a reason for hiding this comment

Tratcher commented Nov 10, 2015

tmds commented Nov 12, 2015

halter73 Nov 13, 2015

Choose a reason for hiding this comment

tmds Nov 14, 2015

Choose a reason for hiding this comment

tmds Nov 14, 2015

Choose a reason for hiding this comment

tmds commented Nov 14, 2015

tmds commented Nov 14, 2015

shahid-pk commented Nov 14, 2015

tmds commented Nov 14, 2015