-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Delayed client certificate #54692
Delayed client certificate #54692
Conversation
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. |
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue Details
|
I see lot of test failures when running locally on Windows 10 box. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
ca25e2c
to
ca00e71
Compare
cc @geoffkizer |
src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c
Outdated
Show resolved
Hide resolved
// Issue empty read to get renegotiation going. | ||
await ReadAsyncInternal(adapter, Memory<byte>.Empty, renegotiation: true).ConfigureAwait(false); | ||
_handshakeBuffer = new ArrayBuffer(InitialHandshakeBufferSize); | ||
ProtocolToken message = null!; |
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 similar to the existing logic in ForceAuthenticationAsync. Can we share the logic so it isn't duplicated?
I also wonder about some cases that ForceAuthenticationAsync is handling which aren't handled here, like transferring any additional buffered read data to the _internalBuffer.
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 added the missing handling of _internalBuffer. I agree about the similarity but I would like to let re-factoring for and consolidation for follow up work so we can get the base functionality in.
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs
Show resolved
Hide resolved
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.
Feedback above
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Show resolved
Hide resolved
contributes to #49346 I addressed all the functional feedback and this is ready for another pass @geoffkizer. Currently this will work only with OpenSSL 1.1.1 and older versions have some problems. |
@wfurt thanks for hand over, the changes LGTM |
@wfurt we should create new tracking issue/bug for lower OpenSSL versions ... @geoffkizer will you be able to finish code review to hit the checkin date tomorrow? |
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.
Core parts looks ok to.
We still have #49346 open so we can use it track the progress. I think we can investigate older version little bit more and either fix it if easy, leave the issue open or create new one or throw PNSP. |
Tested following configuration:
SslStream authenticated as client was without change, throwing PNSE (when using openssl version smaller than 1.1.1) would
|
Ads support for retrieve client certificate on secure connection.
Contributes to #49346