From 48e809ccb67eddff11b5fa5290c55e7ea101d8a7 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 25 Mar 2021 14:11:50 -0700 Subject: [PATCH 01/13] Disable tests on Android that use unsupported features (Brainpool curve, PSS padding in cert signatures) --- .../CertificateRequestChainTests.cs | 7 +++++++ .../tests/PublicKeyTests.cs | 17 ++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestChainTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestChainTests.cs index 16c41b6a68dca..10c33cacca14c 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestChainTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestChainTests.cs @@ -526,6 +526,13 @@ public static void CreateChain_RSAPSS() private static bool DetectPssSupport() { + if (PlatformDetection.IsAndroid) + { + // Android supports PSS at the algorithms layer, but does not support it + // being used in cert chains. + return false; + } + using (X509Certificate2 cert = new X509Certificate2(TestData.PfxData, TestData.PfxDataPassword)) using (RSA rsa = cert.GetRSAPrivateKey()) { diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs index 411ca75782958..c721a224269bb 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs @@ -12,6 +12,21 @@ namespace System.Security.Cryptography.X509Certificates.Tests { public static class PublicKeyTests { + public static bool SupportsBrainpool { get; } = IsCurveSupported(ECCurve.NamedCurves.brainpoolP160r1.Oid); + + private static bool IsCurveSupported(Oid oid) + { + try + { + using ECDsa ecdsa = ECDsa.Create(ECCurve.CreateFromOid(oid)); + } + catch (Exception ex) when (ex is CryptographicException || ex is PlatformNotSupportedException) + { + return false; + } + return true; + } + private static PublicKey GetTestRsaKey() { using (var cert = new X509Certificate2(TestData.MsCertificate)) @@ -472,7 +487,7 @@ public static void TestECDHPublicKey_DeriveSecret() } } - [Theory, MemberData(nameof(BrainpoolCurves))] + [ConditionalTheory(nameof(SupportsBrainpool)), MemberData(nameof(BrainpoolCurves))] public static void TestECDsaPublicKey_BrainpoolP160r1_ValidatesSignature(byte[] curveData, byte[] existingSignature) { byte[] helloBytes = Encoding.ASCII.GetBytes("Hello"); From 5f2b5b6fb8454aa151c84466e14943ecc539c020 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 25 Mar 2021 16:46:54 -0700 Subject: [PATCH 02/13] Handle JNI thread shutdown --- .../pal_jni.c | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c index f5f6819e9afbf..d166c23dd8645 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_jni.c @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. #include "pal_jni.h" +#include JavaVM* gJvm; @@ -476,6 +477,8 @@ static jclass GetOptionalClassGRef(JNIEnv *env, const char* name) if (!TryGetClassGRef(env, name, &klass)) { LOG_DEBUG("optional class %s was not found", name); + // Failing to find an optional class causes an exception state, which we need to clear. + TryClearJNIExceptions(env); } return klass; @@ -551,6 +554,8 @@ jmethodID GetOptionalMethod(JNIEnv *env, bool isStatic, jclass klass, const char jmethodID mid = isStatic ? (*env)->GetStaticMethodID(env, klass, name, sig) : (*env)->GetMethodID(env, klass, name, sig); if (!mid) { LOG_INFO("optional method %s %s was not found", name, sig); + // Failing to find an optional method causes an exception state, which we need to clear. + TryClearJNIExceptions(env); } return mid; } @@ -566,6 +571,22 @@ jfieldID GetField(JNIEnv *env, bool isStatic, jclass klass, const char* name, co return fid; } +static void DetatchThreadFromJNI(void* unused) +{ + LOG_DEBUG("Detaching thread from JNI"); + (void)unused; + (*gJvm)->DetachCurrentThread(gJvm); +} + +static pthread_key_t threadLocalEnvKey; +static pthread_once_t threadLocalEnvInitKey = PTHREAD_ONCE_INIT; + +static void +make_key() +{ + (void) pthread_key_create(&threadLocalEnvKey, &DetatchThreadFromJNI); +} + JNIEnv* GetJNIEnv() { JNIEnv *env; @@ -573,6 +594,11 @@ JNIEnv* GetJNIEnv() if (env) return env; jint ret = (*gJvm)->AttachCurrentThreadAsDaemon(gJvm, &env, NULL); + + (void) pthread_once(&threadLocalEnvInitKey, make_key); + LOG_DEBUG("Registering JNI thread detach. env ptr %p. Key: %ld", (void*)env, (long)threadLocalEnvKey); + pthread_setspecific(threadLocalEnvKey, env); + assert(ret == JNI_OK && "Unable to attach thread to JVM"); (void)ret; return env; From 080a566f6f4c3c5d99fd09e1edfc0ae925640880 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 25 Mar 2021 16:47:16 -0700 Subject: [PATCH 03/13] On older Android API levels, ECPrivateKey does not implement Destroyable --- .../pal_ecc_import_export.c | 2 +- .../System.Security.Cryptography.Native.Android/pal_eckey.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c index 7018137fc70d9..b448e6155c29e 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_ecc_import_export.c @@ -315,7 +315,7 @@ static jobject AndroidCryptoNative_CreateKeyPairFromCurveParameters( goto cleanup; error: - if (loc[privateKey]) + if (loc[privateKey] && (*env)->IsInstanceOf(env, loc[privateKey], g_DestroyableClass)) { // Destroy the private key data. (*env)->CallVoidMethod(env, loc[privateKey], g_destroy); diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c index 73fb254839f2f..f14bfc9f640cf 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_eckey.c @@ -45,7 +45,7 @@ void AndroidCryptoNative_EcKeyDestroy(EC_KEY* r) { // Destroy the private key data. jobject privateKey = (*env)->CallObjectMethod(env, r->keyPair, g_keyPairGetPrivateMethod); - if (privateKey) + if (privateKey && (*env)->IsInstanceOf(env, privateKey, g_DestroyableClass)) { (*env)->CallVoidMethod(env, privateKey, g_destroy); ReleaseLRef(env, privateKey); From 8f347ef1c1e06636055f566b62d9b29c0d1c741e Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 25 Mar 2021 16:47:56 -0700 Subject: [PATCH 04/13] On older API levels, we can end up getting a certificate with a null public key. Return early with null instead of crashing with an assert. --- .../System.Security.Cryptography.Native.Android/pal_x509.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c index 9cd92dadecdab..82729e5592692 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c @@ -250,6 +250,10 @@ void* AndroidCryptoNative_X509PublicKey(jobject /*X509Certificate*/ cert, PAL_Ke void* keyHandle; jobject key = (*env)->CallObjectMethod(env, cert, g_X509CertGetPublicKey); + if (!key) + { + return NULL; + } switch (algorithm) { case PAL_EC: From a2a9c9064cb6c3db9145f4af00e5ee01f92100e9 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 25 Mar 2021 16:48:15 -0700 Subject: [PATCH 05/13] Condition another test on SupportsBrainpool for clarity. --- .../tests/PublicKeyTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs index c721a224269bb..77a2d6c68bb28 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs @@ -370,7 +370,7 @@ public static void TestKey_RSA384_ValidatesSignature() } } - [Theory, MemberData(nameof(BrainpoolCurves))] + [ConditionalTheory(nameof(SupportsBrainpool)), MemberData(nameof(BrainpoolCurves))] public static void TestKey_ECDsabrainpool_PublicKey(byte[] curveData, byte[] notUsed) { _ = notUsed; From 26d925126c4587fc25614af7f56140dc67a12718 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 26 Mar 2021 11:54:43 -0700 Subject: [PATCH 06/13] Fix X509 test failures found when running against an API Level 21 Android simulator. --- .../Interop.X509Chain.cs | 18 +++++++++++++++--- .../pal_x509chain.c | 17 +++++++++++++---- .../pal_x509chain.h | 9 +++++++-- .../pal_x509store.c | 5 ++++- .../tests/ChainTests.cs | 5 ++++- .../DynamicRevocationTests.Android.cs | 16 ++++++++++++++++ .../DynamicRevocationTests.Default.cs | 16 ++++++++++++++++ .../RevocationTests/DynamicRevocationTests.cs | 3 ++- ....Cryptography.X509Certificates.Tests.csproj | 16 ++++++++++++++++ 9 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Android.cs create mode 100644 src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Default.cs diff --git a/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs b/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs index 70b3f60bf3512..2590f256925e2 100644 --- a/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs +++ b/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Runtime.InteropServices; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; @@ -41,16 +42,23 @@ internal static X509Certificate2[] X509ChainGetCertificates(SafeX509ChainContext var certPtrs = new IntPtr[count]; int res = Interop.AndroidCrypto.X509ChainGetCertificates(ctx, certPtrs, certPtrs.Length); - if (res != SUCCESS) + if (res == 0) throw new CryptographicException(); + Debug.Assert(res <= certPtrs.Length); + var certs = new X509Certificate2[certPtrs.Length]; - for (int i = 0; i < certs.Length; i++) + for (int i = 0; i < res; i++) { certs[i] = new X509Certificate2(certPtrs[i]); } - return certs; + if (res == certPtrs.Length) + { + return certs; + } + + return certs[0..res]; } [StructLayout(LayoutKind.Sequential)] @@ -90,6 +98,10 @@ internal static extern int X509ChainSetCustomTrustStore( IntPtr[] customTrustStore, int customTrustStoreLen); + [DllImport(Libraries.CryptoNative, EntryPoint = "AndroidCryptoNative_X509ChainSupportsRevocationOptions")] + [return:MarshalAs(UnmanagedType.U1)] + internal static extern bool X509ChainSupportsRevocationOptions(); + [DllImport(Libraries.CryptoNative, EntryPoint = "AndroidCryptoNative_X509ChainValidate")] internal static extern int X509ChainValidate( SafeX509ChainContextHandle ctx, diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c index f57753fa799a8..9a7e0a7622e06 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c @@ -192,7 +192,16 @@ int32_t AndroidCryptoNative_X509ChainGetCertificates(X509ChainContext* ctx, // Certificate trustedCert = trustAnchor.getTrustedCert(); // certs[i] = trustedCert; jobject trustedCert = (*env)->CallObjectMethod(env, ctx->trustAnchor, g_TrustAnchorGetTrustedCert); - certs[i] = ToGRef(env, trustedCert); + if (!(*env)->IsSameObject(env, certs[i-1], certs[i])) + { + certs[i] = ToGRef(env, trustedCert); + ret = i + 1; + } + else + { + ret = i; + certs[i] = NULL; + } ret = SUCCESS; @@ -404,7 +413,7 @@ int32_t AndroidCryptoNative_X509ChainSetCustomTrustStore(X509ChainContext* ctx, return CheckJNIExceptions(env) ? FAIL : SUCCESS; } -static bool X509ChainSupportsRevocationOptions(void) +bool AndroidCryptoNative_X509ChainSupportsRevocationOptions(void) { return g_CertPathValidatorGetRevocationChecker != NULL && g_PKIXRevocationCheckerClass != NULL; } @@ -483,7 +492,7 @@ static int32_t ValidateWithRevocation(JNIEnv* env, else { certPathToUse = ctx->certPath; - if (X509ChainSupportsRevocationOptions()) + if (AndroidCryptoNative_X509ChainSupportsRevocationOptions()) { // Only add the ONLY_END_ENTITY if we are not just checking the trust anchor. If ONLY_END_ENTITY is // specified, revocation checking will skip the trust anchor even if it is the only certificate. @@ -512,7 +521,7 @@ static int32_t ValidateWithRevocation(JNIEnv* env, } jobject params = ctx->params; - if (X509ChainSupportsRevocationOptions()) + if (AndroidCryptoNative_X509ChainSupportsRevocationOptions()) { // PKIXRevocationChecker checker = validator.getRevocationChecker(); loc[checker] = (*env)->CallObjectMethod(env, validator, g_CertPathValidatorGetRevocationChecker); diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.h index 3e2dce658711c..4c92fa74c5659 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.h +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.h @@ -33,9 +33,9 @@ Return the number of certificates in the path PALEXPORT int32_t AndroidCryptoNative_X509ChainGetCertificateCount(X509ChainContext* ctx); /* -Get the certificates in the path +Get the certificates in the path. -Returns 1 on success, 0 otherwise +Returns the number of certificates exported. */ PALEXPORT int32_t AndroidCryptoNative_X509ChainGetCertificates(X509ChainContext* ctx, jobject* /*X509Certificate[]*/ certs, @@ -62,6 +62,11 @@ PALEXPORT int32_t AndroidCryptoNative_X509ChainSetCustomTrustStore(X509ChainCont jobject* /*X509Certificate[]*/ customTrustStore, int32_t customTrustStoreLen); +/* +Returns true if revocation checking is supported. Returns false otherwise. +*/ +PALEXPORT bool AndroidCryptoNative_X509ChainSupportsRevocationOptions(void); + // Matches managed X509RevocationMode enum enum { diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509store.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509store.c index 5fec76e64f319..64b839d2018d3 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509store.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509store.c @@ -98,6 +98,7 @@ int32_t AndroidCryptoNative_X509StoreAddCertificate(jobject /*KeyStore*/ store, EntryFlags flags; if (ContainsEntryForAlias(env, store, cert, alias, &flags)) { + ReleaseLRef(env, alias); EntryFlags matchesFlags = EntryFlags_HasCertificate & EntryFlags_MatchesCertificate; if ((flags & matchesFlags) != matchesFlags) { @@ -141,12 +142,14 @@ int32_t AndroidCryptoNative_X509StoreAddCertificateWithPrivateKey(jobject /*KeyS EntryFlags matchesFlags = EntryFlags_HasCertificate & EntryFlags_MatchesCertificate; if ((flags & matchesFlags) != matchesFlags) { + RELEASE_LOCALS(loc, env); LOG_ERROR("Store already contains alias with entry that does not match the expected certificate"); return FAIL; } if ((flags & EntryFlags_HasPrivateKey) == EntryFlags_HasPrivateKey) { + RELEASE_LOCALS(loc, env); // Certificate with private key is already in store - nothing to do LOG_DEBUG("Store already contains certificate with private key"); return SUCCESS; @@ -154,7 +157,7 @@ int32_t AndroidCryptoNative_X509StoreAddCertificateWithPrivateKey(jobject /*KeyS // Delete existing entry. We will replace the existing cert with the cert + private key. // store.deleteEntry(alias); - (*env)->CallVoidMethod(env, store, g_KeyStoreDeleteEntry, alias); + (*env)->CallVoidMethod(env, store, g_KeyStoreDeleteEntry, loc[alias]); } bool releasePrivateKey = true; diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs index 9e5dbb03606e6..5744a52030e7a 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs @@ -210,10 +210,13 @@ public static void TestResetMethod() /// /// Tests that when a certificate chain has a root certification which is not trusted by the trust provider, - /// Build returns false and a ChainStatus returns UntrustedRoot + /// Build returns false and a ChainStatus returns UntrustedRoot. + /// Android does not support the detailed status in this test. It always validates time + /// and trusted root. It will fail to build any chain if those are not valid. /// [Fact] [OuterLoop] + [PlatformSpecific(~TestPlatforms.Android)] public static void BuildChainExtraStoreUntrustedRoot() { using (var testCert = new X509Certificate2(TestFiles.ChainPfxFile, TestData.ChainPfxPassword)) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Android.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Android.cs new file mode 100644 index 0000000000000..fd733341851fc --- /dev/null +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Android.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Security.Cryptography.X509Certificates.Tests.Common; +using Xunit; + +namespace System.Security.Cryptography.X509Certificates.Tests.RevocationTests +{ + public static partial class DynamicRevocationTests + { + public static bool SupportsDynamicRevocation { get; } = Interop.AndroidCrypto.X509ChainSupportsRevocationOptions(); + } +} diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Default.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Default.cs new file mode 100644 index 0000000000000..34384d913b4f0 --- /dev/null +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.Default.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Security.Cryptography.X509Certificates.Tests.Common; +using Xunit; + +namespace System.Security.Cryptography.X509Certificates.Tests.RevocationTests +{ + public static partial class DynamicRevocationTests + { + public static bool SupportsDynamicRevocation => true; + } +} diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs index 7b311230b0d5d..ebf8910855825 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs @@ -10,7 +10,8 @@ namespace System.Security.Cryptography.X509Certificates.Tests.RevocationTests { [OuterLoop("These tests run serially at about 1 second each, and the code shouldn't change that often.")] - public static class DynamicRevocationTests + [ConditionalClass(typeof(DynamicRevocationTests), nameof(SupportsDynamicRevocation))] + public static partial class DynamicRevocationTests { // The CI machines are doing an awful lot of things at once, be generous with the timeout; internal static readonly TimeSpan s_urlRetrievalLimit = TimeSpan.FromSeconds(15); diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj b/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj index 53d0dd75a0baa..4c8120823f20f 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj @@ -102,6 +102,22 @@ + + + + + + + + + + + Date: Fri, 26 Mar 2021 15:07:52 -0700 Subject: [PATCH 07/13] Reuse existing item groups when possible. --- ...System.Security.Cryptography.X509Certificates.Tests.csproj | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj b/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj index 4c8120823f20f..d704ec3b6cbdf 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj @@ -99,13 +99,11 @@ - - - + From ea006f483cf51052bbc45249c95dbdb07e8554f3 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 26 Mar 2021 15:20:21 -0700 Subject: [PATCH 08/13] Remove extraneous test suppression and handle exceptions. --- .../Unix/System.Security.Cryptography.Native.Android/pal_x509.c | 2 +- .../tests/PublicKeyTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c index 82729e5592692..dfa72406a6c5e 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509.c @@ -250,7 +250,7 @@ void* AndroidCryptoNative_X509PublicKey(jobject /*X509Certificate*/ cert, PAL_Ke void* keyHandle; jobject key = (*env)->CallObjectMethod(env, cert, g_X509CertGetPublicKey); - if (!key) + if (CheckJNIExceptions(env) || !key) { return NULL; } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs index 77a2d6c68bb28..c721a224269bb 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs @@ -370,7 +370,7 @@ public static void TestKey_RSA384_ValidatesSignature() } } - [ConditionalTheory(nameof(SupportsBrainpool)), MemberData(nameof(BrainpoolCurves))] + [Theory, MemberData(nameof(BrainpoolCurves))] public static void TestKey_ECDsabrainpool_PublicKey(byte[] curveData, byte[] notUsed) { _ = notUsed; From 37e649cf9c1806516f9904e88865a3b2c7f5e0e5 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 26 Mar 2021 15:33:19 -0700 Subject: [PATCH 09/13] Fix bug in x509 get certs. --- .../System.Security.Cryptography.Native.Android/pal_x509chain.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c index 9a7e0a7622e06..97653ec606d22 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c @@ -203,8 +203,6 @@ int32_t AndroidCryptoNative_X509ChainGetCertificates(X509ChainContext* ctx, certs[i] = NULL; } - ret = SUCCESS; - cleanup: (*env)->DeleteLocalRef(env, certPathList); return ret; From 306b4dc3a8d692eb41ad48d0fe21983ac973b134 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Apr 2021 09:46:40 -0700 Subject: [PATCH 10/13] Disable X509 tests on brower with ProjectExclusions. --- .../tests/AssemblyInfo.cs | 7 ------- ...tem.Security.Cryptography.X509Certificates.Tests.csproj | 1 - src/libraries/tests.proj | 3 +++ 3 files changed, 3 insertions(+), 8 deletions(-) delete mode 100644 src/libraries/System.Security.Cryptography.X509Certificates/tests/AssemblyInfo.cs diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/AssemblyInfo.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/AssemblyInfo.cs deleted file mode 100644 index 3a74f33d7aa32..0000000000000 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/AssemblyInfo.cs +++ /dev/null @@ -1,7 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using Xunit; - -[assembly: ActiveIssue("https://github.com/dotnet/runtime/issues/37669", TestPlatforms.Browser)] diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj b/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj index d704ec3b6cbdf..af574bfff240d 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj @@ -12,7 +12,6 @@ - + + + From 32036cb596d5e65bb65de35aff465a6bf7eff624 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Apr 2021 11:12:07 -0700 Subject: [PATCH 11/13] Fix ChainTests.TestResetMethod test fix. --- .../System.Security.Cryptography.Native.Android/pal_x509chain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c index 97653ec606d22..adc736b0af11f 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_x509chain.c @@ -192,7 +192,7 @@ int32_t AndroidCryptoNative_X509ChainGetCertificates(X509ChainContext* ctx, // Certificate trustedCert = trustAnchor.getTrustedCert(); // certs[i] = trustedCert; jobject trustedCert = (*env)->CallObjectMethod(env, ctx->trustAnchor, g_TrustAnchorGetTrustedCert); - if (!(*env)->IsSameObject(env, certs[i-1], certs[i])) + if (i == 0 || !(*env)->IsSameObject(env, certs[i-1], trustedCert)) { certs[i] = ToGRef(env, trustedCert); ret = i + 1; From e1ecfe85bec8c1d521ed1322dfa0fc7c67d79787 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Apr 2021 13:36:38 -0700 Subject: [PATCH 12/13] PR feedback. --- .../tests/PublicKeyTests.cs | 24 ++++++------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs index c721a224269bb..346cf59002698 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/PublicKeyTests.cs @@ -12,21 +12,6 @@ namespace System.Security.Cryptography.X509Certificates.Tests { public static class PublicKeyTests { - public static bool SupportsBrainpool { get; } = IsCurveSupported(ECCurve.NamedCurves.brainpoolP160r1.Oid); - - private static bool IsCurveSupported(Oid oid) - { - try - { - using ECDsa ecdsa = ECDsa.Create(ECCurve.CreateFromOid(oid)); - } - catch (Exception ex) when (ex is CryptographicException || ex is PlatformNotSupportedException) - { - return false; - } - return true; - } - private static PublicKey GetTestRsaKey() { using (var cert = new X509Certificate2(TestData.MsCertificate)) @@ -487,7 +472,7 @@ public static void TestECDHPublicKey_DeriveSecret() } } - [ConditionalTheory(nameof(SupportsBrainpool)), MemberData(nameof(BrainpoolCurves))] + [Theory, MemberData(nameof(BrainpoolCurves))] public static void TestECDsaPublicKey_BrainpoolP160r1_ValidatesSignature(byte[] curveData, byte[] existingSignature) { byte[] helloBytes = Encoding.ASCII.GetBytes("Hello"); @@ -504,7 +489,12 @@ public static void TestECDsaPublicKey_BrainpoolP160r1_ValidatesSignature(byte[] Assert.Equal("1.2.840.10045.2.1", cert.PublicKey.Oid.Value); bool isSignatureValid = publicKey.VerifyData(helloBytes, existingSignature, HashAlgorithmName.SHA256); - Assert.True(isSignatureValid, "isSignatureValid"); + + if (!isSignatureValid) + { + Assert.True(PlatformDetection.IsAndroid, "signature invalid on Android only"); + return; + } unchecked { From 46a3d5b6c990b0816f21e06a4a8f4805d6fad4a1 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 5 Apr 2021 16:14:43 -0700 Subject: [PATCH 13/13] Fix test project path. --- src/libraries/tests.proj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/tests.proj b/src/libraries/tests.proj index 5ac7c286abce6..2d5924c7ca45e 100644 --- a/src/libraries/tests.proj +++ b/src/libraries/tests.proj @@ -167,7 +167,7 @@ - +