-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 NegotiateClientCertificateAsync support on Windows #51905
Conversation
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsThis may still need some more work but this change brings basic support for I wanted to lay out some tests and restrictions as well as provide some implementation so it can be tested with Kestrel. cc: @davidfowl @Tratcher contributes to #49346
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
await TestConfiguration.WhenAllOrAnyFailedWithTimeout( | ||
client.AuthenticateAsClientAsync(clientOptions), | ||
server.AuthenticateAsServerAsync(serverOptions)); | ||
// need this to complete TLS 1.3 handshake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this is needed for 1.3? Not obvious to me...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When AuthenticateAs*
is finished with TLS 1.3, the handshake is really not done. (unfortunately)
This is clearly visible on Windows when on first message, the "renegotiation" path is triggered and underlying TLS state will change. So the next line will make sure that we are in fully operational and completed state.
Now, when I try to trigger renegotiation e.g. post-handshake-authentication in this state it fails.
In practice this should not matter as the server will receive request and that will get this completed AFAIK.
I was wondering if I could/should parametrize the test above but I decided not to as the PHA is really different beast as separate test method may be easier to update and understand. For Linux I only have prototype with TLS 1.2 so I don't know if TLS 1.3 will behave same way or not. It is certainly different set of APIs in OpenSSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand this...
With TLS 1.3, exactly what TLS state can change after AuthenticateAs*? Is it that after AuthenticateAsServer, we don't necessarily have the client cert yet?
Or are there other cases too?
I'm wondering how a user is supposed to deal with this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the underlying Schannel. it would essentially trigger the renegotiation path and calling write or another renegotiation would fail at that point. We would have certificate but the Schannel is still probably updating some crypto parameters.
Normally this would not be visible to the user. we had to rework locking quite a bit in 5.0. So I padded many tests that do care about particular behavior with the ping/pong to make sure everything is in steady state.
For TLS 1.2 and bellow we may for example receive SessionTickets. It does not matter too much in this case but it is another example when there is come extra processing after the handshake is done.
I don't know how Linux will behave at this point for TLS 1.3 since it is using different mechanism.
One option may be to remove this test and table TLS 1.3 as separate effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I understand... let me make sure...
The issue here is that with TLS 1.3, the initial handshake isn't actually guaranteed to be done on the server when AuthenticateAsServer completes. (Aside: is this only on the server, or on the client too?)
In practice this is not visible to the user except for this case, right? E.g. if the client sends a cert in the initial handshake, it will be available as soon as AuthenticateAsServer completes, right? Are there any user-visible parameters that might change after AuthenticateAsServer completes?
In this case though, I think the issue is that we can't start the "renegotiation" (or really the TLS 1.3 PHA) until the initial handshake is fully completed, is that correct?
What happens if the user calls NegotiateClientCertAsync in this case, will it fail?
I wonder if we should just wait until the initial handshake completes. Is that possible? Is it hard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was failing for me if you try renegotiation while in this weird state. I decided (at least for now) that this should not be problem in practice as the server will need receive and consume the request first so all this would actually finish.
The detection may be tricky as it works different way on Linux. The ReplyOnReAuthenticationAsync is never called but I suspect the underlying OpenSSL state is similar as we saw some strange errors before reworking the locking. I can dig through the TLS 1.3 RFC but I think some of the crypto piggybacks on first data to to cut down on roundtrips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided (at least for now) that this should not be problem in practice as the server will need receive and consume the request first so all this would actually finish.
Yeah, I guess that's reasonable. It seems like the client is always going to send something once the handshake completes, anyway.
Should we add a TLS-1.3 specific test to ensure we fail properly if they call NegotiateClientCertAsync in this weird state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can dig through the TLS 1.3 RFC but I think some of the crypto piggybacks on first data to to cut down on roundtrips.
I think it would be really good to understand this better. I feel like users could get very confused here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. I can ping the Schannel people as well.
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
Can we add more negative test cases? Specifically to test: (1) If there's already-decrypted data in the buffer when Negotiate... is called, it should fail immediately |
@@ -171,7 +171,7 @@ public async Task SslStream_NetworkStream_Renegotiation_Succeeds(bool useSync) | |||
} | |||
} | |||
|
|||
[Theory] | |||
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, the test failed on Windows7 and I did not have chance to investigate. I'm not sure how important that is but we can possibly throw PNSP if we cannot figure out how to make it work. The flow will need to probably change once we incorporate Linux so I did not want to invest too much into it at the moment.
Main goal of this PR was to provide something useable so you can experiment with Kestrel and provide feedback @Tratcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Win7 support isn't critical, so long as we get a good explanation for why it doesn't work. Http.Sys had an issue on Win7 that might be related: https://github.com/dotnet/aspnetcore/blob/8f564897f68944d5f9bf9bded45160fcf3fb0329/src/Servers/HttpSys/src/HttpSysListener.cs#L21-L29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link. That is good to know.
I can add more tests. I was somewhat hesitant as some of the restrictions comes from HTTP while SslStream is generally agnostic. But since the main use if for Kestrel, I think it is probably OK to add this for now. |
I added test for the first case. After looking at the other two, I would like to defer it until #50815 goes in as that will change the processing significantly. The Linux prototype does not use the InternalReadAsync. So when the refactoring goes in I'm inclined to use the new helper functions and process frames directly in RenegotiateAsync. That will also make it easier to check frame type in the production code and write the two remaining tests around TLS frames. Let me know if that make sense @geoffkizer |
notes added. failing MacCatalyst and iOSS are unrelated. |
Is this ready for review again? @wfurt is there any unfinished work here? |
It should be ready @geoffkizer. Next round of work depends on the ssl stream refactoring and Linux support. #49346 will stay open for a while. |
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
can you please take another look @geoffkizer @stephentoub |
@@ -612,6 +665,14 @@ private bool CompleteHandshake(ref ProtocolToken? alertToken, out SslPolicyError | |||
{ | |||
_context!.ProcessHandshakeSuccess(); | |||
|
|||
if (_nestedAuth != 1) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something worth logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small nit, otherwise looks good.
This may still need some more work but this change brings basic support for
NegotiateClientCertificateAsync
e.g. ability to ask for peer certificate late in TLS session.I wanted to lay out some tests and restrictions as well as provide some implementation so it can be tested with Kestrel.
Linux/OpenSSL will be different with similar structure. We need to trigger the renegotiation and then we simply crank the state machine until it is finished.
On Windows, Tls12 and Tls13 has same code path. Linux will be different as the feature is implemented on top of different mechanisms.
cc: @davidfowl @Tratcher
contributes to #49346