-
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
use also SslCertificateTrust when constructing CertificateContext #103372
Conversation
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
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.
LGTM modulo comments, but I would prefer @bartonjs to confirm that trusting everything to build a chain is okay.
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs
Show resolved
Hide resolved
@wfurt the |
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.
LGTM
why don't you do that as separate PR @simonrozsival . Maybe there will be more changes or more tests to enable...? |
…tnet#103372) * use also SslCertificateTrust when constructing CertificateContext * 'build * feedback
…CertificateContext (#104541) Backport of #103372 and #104016 to release/8.0-staging ## Customer Impact - [X] Customer reported (#100602) - [ ] Found internally Customers developing Android apps are currently unable to use mutual TLS authentication in certain cases as the `SslStreamCertificateContext.Create(...)` method will fail to build an X509Chain instance if the certificate isn't trusted by the OS due to the limitations of the Android platform. ## Regression - [ ] Yes - [X] No ## Testing Unit tests and manual testing on Android emulator. ## Risk Low. The change is mostly limited to Android where this API doesn't currently work in many cases. --------- Co-authored-by: Tomas Weinfurt <tweinfurt@yahoo.com> Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
there are two parts to this change.
When #54219 added support for
SslCertificateTrust
it did impact only certificate validation but notSslStreamCertificateContext
. Back in the day it was available only for servers so it was not probably big deal. (as the primary focus was on sending trusted CA list in the handshake). #80182 added support also for client (in 8.0) and that opened more options here IMHO. First part of this change will useTrust
if provided to construct the chain for consistency.Second part is specific to Android. It seems like we have difficulties to construct the chain unless the root CA is trusted.
The part above should help, but to make it more consistent with other platforms I also added fall-back attempt to build chain if intermediate certificates are provided. This is not used for avaluating trust so I feel it should be OK to use it to get list of certificates to send.
contributes to #100602
I assume @simonrozsival will follow-up on the android and update tests as needed and he would provide more insight if needed.