-
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
Initial support for SslStreamCertificateContext #38364
Conversation
Tagging subscribers to this area: @dotnet/ncl |
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. |
Nothing should ever use (e.g. the new multi-pem load methods are only on X509Certificate2Collection) |
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
Outdated
Show resolved
Hide resolved
intermediates[i] = chain.ChainElements[i + 1].Certificate; | ||
} | ||
} | ||
} |
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.
} | |
// Dispose the copy of the target cert. | |
chain.ChainElements[0].Certificate.Dispose(); | |
// Dispose the last cert, if we didn't include it. | |
for (int i = count + 1; i < chain.ChainElements.Count; i++) | |
{ | |
chain.ChainElements[i].Certificate.Dispose(); | |
} | |
} |
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.
if the chain is not complete, should se sent out as much as we can? e.g. the leave as well?
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.
The leaf is always sent 😄
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 guess bad terminology. I meant certificate closest to the root if root is missing. e.g. is certificate c
is signed by i1
-> i2
-> i3
-> root
and root
is missing, send cert + i1 + i2 + i3.
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.
Yeah, if the last cert isn't self-signed, then all the certs after the end-entity should be sent; that's why I checked for PartialChain (it's returned when the last cert that the builder could find wasn't self-issued).
throw new AuthenticationException(SR.net_ssl_io_no_server_cert); | ||
} | ||
|
||
CertificateContext = new SslStreamCertificateContext(certificateWithKey); |
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.
CertificateContext = new SslStreamCertificateContext(certificateWithKey); | |
CertificateContext = SslStreamCertificateContext.Create(certificateWithKey, 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.
I added internal Create()
to be more consistent but allowing to have SslStreamCertificateContext without building full chain to support legacy behavior on platform where we do not build chain but leave OS to do it.
This adds part of the new API surface approved in #37933. Internally, I switched SslStream to use CertificateContext instead of ServerCertificate and I did minimal fixes in OpenSSL pal to get that through. MacOS and Windows will follow.
So as more tests once #38182 is in. I will us that structure to create more test scenarios for partial chains.
contribues to #35844
SslStreamCertificateContext.Create() was approved with X509Certificate2Collection and I'm wondering if X509CertificateCollection would be sufficient @bartonjs since we do not need keys for the intermediate certificates.