-
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
[Android] Allow using installed certificates for client authentication in SslStream #103337
[Android] Allow using installed certificates for client authentication in SslStream #103337
Conversation
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
@simonrozsival Thank you for working on this! Very exciting. What's the easiest way for me to test this with our various PKI tests on Android? |
@dotMorten I'm not sure if there's an easy way to do it, you'll need to jump through a few hoops:
|
src/libraries/Common/tests/TestUtilities/System/AndroidKeyStoreHelper.cs
Outdated
Show resolved
Hide resolved
…eHelper.cs Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
I tried this change, and it indeed addresses the issue for us without our test server and certificates. Nice work @simonrozsival (and thanks for helping getting it running). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
The failing tests in |
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, a couple comments but I'm fine if we address them in a follow-up PR
src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj
Outdated
Show resolved
Hide resolved
...rity.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native.Android/pal_x509store.h
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs
Outdated
Show resolved
Hide resolved
...aries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509.cs
Outdated
Show resolved
Hide resolved
.../Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Store.cs
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/AndroidKeyStoreHelper.cs
Outdated
Show resolved
Hide resolved
...rity.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native.Android/pal_x509.c
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native.Android/pal_x509store.c
Outdated
Show resolved
Hide resolved
...rity.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Context: dotnet/runtime#103016 Context: dotnet/runtime#103337 In dotnet/runtime we are adding a few more Java classes to assist with .NET crypto. One was added in dotnet/runtime#103016, and another may be added in dotnet/runtime#103337. This PR changes ProGuard to keep all of the classes in this package rather than individually adding them. Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Failing |
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. Would X509Certificate2.HasPrivateKey return right value?
That is asserted here: runtime/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs Line 222 in 49663a6
And the Android X.509 PAL changes look sensible. Line 181 in 49663a6
|
@wfurt yes, the AndroidCertificatePal class will return the right value whether it's created the previously existing way or the new way with KeyStore.PrivateKeyEntry. |
@@ -32,6 +32,31 @@ internal static byte[] X509Encode(SafeX509Handle x) | |||
|
|||
return encoded; | |||
} | |||
[LibraryImport(Libraries.AndroidCryptoNative, EntryPoint = "AndroidCryptoNative_X509IsKeyStorePrivateKeyEntry")] | |||
[return: MarshalAs(UnmanagedType.U1)] |
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.
@simonrozsival the native code returns int32_t here so we should change this to UnmanagedType.Bool (4 bytes), or change the native code to use a C bool
which is 1 byte. We seem to be using both variants in the Android crypto imports...
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.
At least in the Linux and MacOS Desktop PALs, we try to use int32_t
and not a C bool
since there is no guarantee by the C standard that it is exactly one byte.
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.
oh, thanks for pointing that out!
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.
Closes #99874
This PR adds the possibility to use certificates with non-exportable private keys installed on an Android device for client authentication in SslStream/SocketsHttpHandler.
The inspiration for these changes comes from
AppleCertificatePal
where we allow passing in eitherSecCertificate
orSecIdentity
via theIntPtr
constructor. The Android IntPtr constructor will now accept not only handles ofjava.security.cert.X509Certificate
, but also ofjava.security.KeyStore.PrivateKeyEntry
. This will allow the developer to pass in certificates with non-exportable private keys and use them for client authentication (see the linked issue for an example usage withKeyChain.ChoosePrivateKeyAlias
).To implement this feature, it is necessary to introduce new Java class
net.dot.android.crypto.DotnetX509KeyManager
. This will require follow-up work in the .NET for Android SDK code to adjust the ProGuard configuration (https://github.com/xamarin/xamarin-android/blob/7e67d2c241e561b05065f2a57ddf59093f74de58/src/Xamarin.Android.Build.Tasks/Resources/proguard_xamarin.cfg#L29).As a follow-up to this PR, we should introduce new APIs to the
Android.Security.KeyChain
class in thedotnet/android
repository. This will allow the developers to avoid using this low levelIntPtr
constructor./cc @karelz @vitek-karas @grendello @filipnavara @dotMorten