Skip to content

[release/8.0-staging] Fix performance of System.Security. Cryptography.Tests #95310

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 12, 2024
Merged

[release/8.0-staging] Fix performance of System.Security. Cryptography.Tests #95310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...es/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/ImportExport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ namespace System.Security.Cryptography.Rsa.Tests
[SkipOnPlatform(TestPlatforms.Browser, "Not supported on Browser")]
public partial class ImportExport
{
public static bool Supports16384 { get; } = TestRsa16384();
private static readonly Lazy<bool> s_supports16384 = new Lazy<bool>(TestRsa16384);
public static bool Supports16384 => s_supports16384.Value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestRsa16384 itself takes a lot of time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~30 seconds.

Copy link
Member

@stephentoub stephentoub Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woh. Ok. That's not something we have any control over, right? That's all time spent in the OS implementation? Why does testing for even the support for this take so long?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not something we have any control over, right? That's all time spent in the OS implementation?
Yeah.

Why does testing for even the support for this take so long?

It basically boils down to: This is the largest possible RSA key. No one uses them (or should), but it is a theoretical max, so we want to test our upper boundary condition. Using an RSA key that large is a guaranteed way to hit performance problems.

The performance hit is in OpenSSL's EVP_PKEY_check. We use this method on key inputs so we can validate the key's consistency (making sure public parameters match private ones, etc). OpenSSL does a rather "thorough" job of this by running Miller-Rabin on the key input. We have no control over that or any flags, and we can't really avoid the called to EVP_PKEY_check.

Woh. Ok.

This PR cut the total execution time of System.Security.Cryptography on Linux by more than half on my Linux box.


[Fact]
public static void ExportAutoKey()
Expand Down
42 changes: 38 additions & 4 deletions ...Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAKeyFileTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Security.Cryptography.Encryption.RC2.Tests;
using System.Text;
using Microsoft.DotNet.XUnitExtensions;
using Test.Cryptography;
using Xunit;

Expand Down Expand Up @@ -122,9 +123,17 @@ public static void ReadWriteDiminishedDPPrivatePkcs1()
TestData.DiminishedDPParameters);
}

[ConditionalFact(typeof(ImportExport), nameof(ImportExport.Supports16384))]
[ConditionalFact]
[OuterLoop("RSA 16384 takes considerable time.")]
public static void ReadWritePublicPkcs1()
{
// Do not move this to the [ConditionalFact], otherwise the platform will check if RSA 16384 is supported
// during test discovery for innerloop, and the check itself is expensive.
if (!ImportExport.Supports16384)
{
throw new SkipTestException("Platform does not support RSA 16384.");
}

ReadWriteBase64PublicPkcs1(
@"
MIIICgKCCAEAmyxwX6kQNx+LSMao1StC1p5rKCEwcBjzI136An3B/BjthgezAOuu
Expand Down Expand Up @@ -198,9 +207,18 @@ public static void ReadWriteSubjectPublicKeyInfo_DiminishedDPKey()
TestData.DiminishedDPParameters);
}

[ConditionalFact(typeof(ImportExport), nameof(ImportExport.Supports16384))]

[ConditionalFact]
[OuterLoop("RSA 16384 takes considerable time.")]
public static void ReadWriteRsa16384SubjectPublicKeyInfo()
{
// Do not move this to the [ConditionalFact], otherwise the platform will check if RSA 16384 is supported
// during test discovery for innerloop, and the check itself is expensive.
if (!ImportExport.Supports16384)
{
throw new SkipTestException("Platform does not support RSA 16384.");
}

ReadWriteBase64SubjectPublicKeyInfo(
@"
MIIIIjANBgkqhkiG9w0BAQEFAAOCCA8AMIIICgKCCAEAmyxwX6kQNx+LSMao1StC
Expand Down Expand Up @@ -250,9 +268,17 @@ public static void ReadWriteRsa16384SubjectPublicKeyInfo()
TestData.RSA16384Params);
}

[ConditionalFact(typeof(ImportExport), nameof(ImportExport.Supports16384))]
[ConditionalFact]
[OuterLoop("RSA 16384 takes considerable time.")]
public static void ReadWrite16384Pkcs8()
{
// Do not move this to the [ConditionalFact], otherwise the platform will check if RSA 16384 is supported
// during test discovery for innerloop, and the check itself is expensive.
if (!ImportExport.Supports16384)
{
throw new SkipTestException("Platform does not support RSA 16384");
}

ReadWriteBase64Pkcs8(
@"
MIIkQgIBADANBgkqhkiG9w0BAQEFAASCJCwwgiQoAgEAAoIIAQCbLHBfqRA3H4tI
Expand Down Expand Up @@ -525,9 +551,17 @@ public static void ReadEncryptedRsa1032()
TestData.RSA1032Parameters);
}

[ConditionalFact(typeof(ImportExport), nameof(ImportExport.Supports16384))]
[ConditionalFact]
[OuterLoop("RSA 16384 takes considerable time.")]
public static void ReadEncryptedRsa16384()
{
// Do not move this to the [ConditionalFact], otherwise the platform will check if RSA 16384 is supported
// during test discovery for innerloop, and the check itself is expensive.
if (!ImportExport.Supports16384)
{
throw new SkipTestException("Platform does not support RSA 16384");
}

// PBES2: PBKDF2 + des (single DES, not 3DES).
const string base64 = @"
MIIkizA9BgkqhkiG9w0BBQ0wMDAbBgkqhkiG9w0BBQwwDgQI63upT8JPNNcCAggA
Expand Down
30 changes: 27 additions & 3 deletions ...ibraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Xml.Linq;
using Microsoft.DotNet.XUnitExtensions;
using Xunit;

namespace System.Security.Cryptography.Rsa.Tests
Expand Down Expand Up @@ -76,9 +77,17 @@ public static void TestRead1032Parameters_Private()
TestData.RSA1032Parameters);
}

[ConditionalFact(typeof(ImportExport), nameof(ImportExport.Supports16384))]
[ConditionalFact]
[OuterLoop("RSA 16384 takes considerable time.")]
public static void TestRead16384Parameters_Public()
{
// Do not move this to the [ConditionalFact], otherwise the platform will check if RSA 16384 is supported
// during test discovery for innerloop, and the check itself is expensive.
if (!ImportExport.Supports16384)
{
throw new SkipTestException("Platform does not support RSA 16384");
}

RSAParameters expectedParameters = ImportExport.MakePublic(TestData.RSA16384Params);

// Bonus trait of this XML: the Modulus and Exponent parameters
Expand Down Expand Up @@ -157,9 +166,16 @@ iC2wXFMDafnWp1lxXiGcVVu9dE2LeglCgnMUps9QlJD0aXaJHYi2VDQ3zFdMvn8A imlqKtZGdGf9
expectedParameters);
}

[ConditionalFact(typeof(ImportExport), nameof(ImportExport.Supports16384))]
[ConditionalFact]
public static void TestRead16384Parameters_Private()
{
// Do not move this to the [ConditionalFact], otherwise the platform will check if RSA 16384 is supported
// during test discovery for innerloop, and the check itself is expensive.
if (!ImportExport.Supports16384)
{
throw new SkipTestException("Platform does not support RSA 16384");
}

// Bonus trait of this XML: the D parameter is not in
// canonical order.
TestReadXml(
Expand Down Expand Up @@ -634,11 +650,19 @@ public static void TestWrite2048Parameters(bool includePrivateParameters)
));
}

[ConditionalTheory(typeof(ImportExport), nameof(ImportExport.Supports16384))]
[ConditionalTheory]
[InlineData(true)]
[InlineData(false)]
[OuterLoop("RSA 16384 takes considerable time for primality tests.")]
public static void TestWrite16384Parameters(bool includePrivateParameters)
{
// Do not move this to the [ConditionalFact], otherwise the platform will check if RSA 16384 is supported
// during test discovery for innerloop, and the check itself is expensive.
if (!ImportExport.Supports16384)
{
throw new SkipTestException("Platform does not support RSA 16384");
}

TestWriteXml(
TestData.RSA16384Params,
includePrivateParameters,
Expand Down