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.
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.
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
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? |
|
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