[Android] Allow using installed certificates for client authentication in SslStream by simonrozsival · Pull Request #103337 · dotnet/runtime

simonrozsival · 2024-06-12T11:50:11Z

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 either SecCertificate or SecIdentity via the IntPtr constructor. The Android IntPtr constructor will now accept not only handles of java.security.cert.X509Certificate, but also of java.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 with KeyChain.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 the dotnet/android repository. This will allow the developers to avoid using this low level IntPtr constructor.

/cc @karelz @vitek-karas @grendello @filipnavara @dotMorten

…yentry

dotnet-policy-service · 2024-06-12T11:57:48Z

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

simonrozsival · 2024-06-12T12:08:00Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-06-12T12:08:13Z

Azure Pipelines successfully started running 1 pipeline(s).

dotMorten · 2024-06-13T15:18:22Z

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

simonrozsival · 2024-06-14T11:42:00Z

@dotMorten I'm not sure if there's an easy way to do it, you'll need to jump through a few hoops:

build the code in this branch, you can follow https://github.com/dotnet/runtime/blob/main/docs/workflow/building/mono/README.md (alternatively I could send you the prebuilt nupkgs directly if you're OK with that, you can reach out to me via email or discord)
add the local nupkg source to your MAUI app via NuGet.config
follow this guide to use your locally built dev version of the runtimepack: https://github.com/xamarin/xamarin-android/blob/main/Documentation/workflow/DevelopmentTips.md?plain=1#L432
use the new ctor as it's shown in the issue or in the new unit test in this PR

src/libraries/Common/tests/TestUtilities/System/AndroidKeyStoreHelper.cs

…eHelper.cs Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>

dotMorten · 2024-06-14T19:02:35Z

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).
🚢 it

simonrozsival · 2024-06-14T20:53:09Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-06-14T20:53:21Z

Azure Pipelines successfully started running 1 pipeline(s).

…am-support-keystore-privatekeyentry

simonrozsival · 2024-06-19T13:22:52Z

The failing tests in runtime-extra-platforms are known and unrelated (#103472)

akoeplinger

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

...rity.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs

src/native/libs/System.Security.Cryptography.Native.Android/pal_x509store.h

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs

...aries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509.cs

.../Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Store.cs

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.ClientCertificates.cs

src/libraries/System.Net.Http/tests/FunctionalTests/AndroidKeyStoreHelper.cs

...rity.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs

src/native/libs/System.Security.Cryptography.Native.Android/pal_x509.c

src/native/libs/System.Security.Cryptography.Native.Android/pal_x509store.c

...rity.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs

simonrozsival · 2024-06-19T16:41:53Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-06-19T16:42:13Z

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>

simonrozsival · 2024-06-19T21:32:54Z

Failing runtime-extra-platforms tests are unrelated

wfurt

LGTM. Would X509Certificate2.HasPrivateKey return right value?

vcsjones · 2024-06-19T22:02:02Z

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

Assert.True(clientCertificate.HasPrivateKey);

And the Android X.509 PAL changes look sensible.

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs

Line 181 in 49663a6

    
           public bool HasPrivateKey => _privateKey is not null || _keyStorePrivateKeyEntry is not null;

simonrozsival · 2024-06-19T22:04:50Z

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

akoeplinger · 2024-06-21T15:48:32Z

...aries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509.cs

            return encoded;
        }
+        [LibraryImport(Libraries.AndroidCryptoNative, EntryPoint = "AndroidCryptoNative_X509IsKeyStorePrivateKeyEntry")]
+        [return: MarshalAs(UnmanagedType.U1)]


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

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.

oh, thanks for pointing that out!

simonrozsival added 2 commits June 12, 2024 13:27

Add support for KeyStore.PrivateKeyEntry

24c687f

Add test

fef9203

ghost added the area-System.Security label Jun 12, 2024

simonrozsival added os-android and removed area-System.Security labels Jun 12, 2024

dotnet-policy-service bot assigned simonrozsival Jun 12, 2024

Merge branch 'main' into android-sslstream-support-keystore-privateke…

598508c

…yentry

simonrozsival added the area-System.Security label Jun 12, 2024

simonrozsival marked this pull request as draft June 12, 2024 11:57

This was referenced Jun 12, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

Wasm test failure: System addon update list error SyntaxError: XMLHttpRequest.open #102955

Closed

[browser][ST] Wasm tests timeout #103246

Closed

akoeplinger reviewed Jun 14, 2024

View reviewed changes

src/libraries/Common/tests/TestUtilities/System/AndroidKeyStoreHelper.cs Show resolved Hide resolved

Update src/libraries/Common/tests/TestUtilities/System/AndroidKeyStor…

35690e1

…eHelper.cs Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>

simonrozsival added 2 commits June 14, 2024 22:50

Fix outdated comment

dadc2d5

Fix tests

25b2088

Merge branch 'main' of github.com:dotnet/runtime into android-sslstre…

aa7a643

…am-support-keystore-privatekeyentry

simonrozsival requested review from bartonjs, vcsjones and wfurt June 18, 2024 16:16

This was referenced Jun 18, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Build failure: Static graph-based restore failed with exit code .* but did not log an error. #103526

Closed

akoeplinger approved these changes Jun 19, 2024

View reviewed changes

vcsjones mentioned this pull request Jun 19, 2024

Keep all .NET runtime classes dotnet/android#9044

Merged

vcsjones reviewed Jun 19, 2024

View reviewed changes

simonrozsival added 6 commits June 19, 2024 17:05

Fix typo

4ccb253

Add helper method

b9e78eb

Address lifetime concerns

d93c08e

Fix bool marshalling

a62afea

Improve code quality

86327ef

Improve tests

49663a6

build-analysis bot mentioned this pull request Jun 19, 2024

STJ NullPropertyNameFail test failing in CI #103715

Closed

wfurt approved these changes Jun 19, 2024

View reviewed changes

vcsjones approved these changes Jun 19, 2024

View reviewed changes

simonrozsival merged commit 83031f1 into dotnet:main Jun 19, 2024

simonrozsival mentioned this pull request Jun 20, 2024

[Mono.Android] Add convenience KeyChain APIs dotnet/android#9047

Merged

akoeplinger reviewed Jun 21, 2024

View reviewed changes

simonrozsival mentioned this pull request Jun 21, 2024

[Android] Fix return type marshalling on IsKeyStorePrivateKeyEntry #103825

Merged

github-actions bot locked and limited conversation to collaborators Jul 23, 2024

karelz added this to the 9.0.0 milestone Jul 29, 2024

Conversation

simonrozsival commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Jun 12, 2024

Uh oh!

simonrozsival commented Jun 12, 2024

Uh oh!

azure-pipelines bot commented Jun 12, 2024

Uh oh!

dotMorten commented Jun 13, 2024

Uh oh!

simonrozsival commented Jun 14, 2024

Uh oh!

Uh oh!

dotMorten commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonrozsival commented Jun 14, 2024

Uh oh!

azure-pipelines bot commented Jun 14, 2024

Uh oh!

simonrozsival commented Jun 19, 2024

Uh oh!

akoeplinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonrozsival commented Jun 19, 2024

Uh oh!

azure-pipelines bot commented Jun 19, 2024

Uh oh!

simonrozsival commented Jun 19, 2024

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

vcsjones commented Jun 19, 2024

Uh oh!

simonrozsival commented Jun 19, 2024

Uh oh!

akoeplinger Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

vcsjones Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

simonrozsival Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

simonrozsival Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

simonrozsival commented Jun 12, 2024 •

edited

Loading

dotMorten commented Jun 14, 2024 •

edited

Loading