-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
avoid allocation of SafeFreeSslCredentials and SafeDeleteSslContext on Linux #69527
Conversation
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue Details
|
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
Outdated
Show resolved
Hide resolved
|
||
using (SafeX509Handle certHandle = Interop.Crypto.X509UpRef(cert.Handle)) | ||
{ | ||
SetSslCertificate(contextPtr, certHandle, certKeyHandle); |
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.
What happens to certKeyHandle after this SetSslCertificate call?
src/libraries/Common/src/System/Net/Security/Unix/SafeDeleteContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
This is ready for another pass @stephentoub. |
src/libraries/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs
Show resolved
Hide resolved
src/libraries/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
Outdated
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.
Mostly looks good, but some SafeHandle-related issues to be addressed before it can be merged.
I made updates to address comments @stephentoub. Can you please take another look? |
src/libraries/Common/src/System/Net/Security/Unix/SafeDeleteNegoContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <stoub@microsoft.com>
SafeFreeSslCredentials
really have no meaning on Linux (unlike Schannel) and we simple drag copy of some properties fromsslAuthenticationOptions
. The only difference is that we carte copy of certificates handle and key early so it would probably work even if disposed. I moved existing code toAllocateSslContext
and we will get the handles when we carte the TLS session.SafeDeleteSslContext
is basically wrapper that only stores reference to anotherSafeHandle
. To avoid that, I madeSafeSslHandle
subclass fromSafeDeleteSslContext
so I can use single object. There may be better way to do it. For one, I was thinking about actually merging but the benefits seems same and it would be much bigger change.