Skip to content
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

[mono][ios] Unknown format in import in System.Security.Cryptography.Tests #88050

Closed
kotlarmilos opened this issue Jun 26, 2023 · 11 comments · Fixed by #88340
Closed

[mono][ios] Unknown format in import in System.Security.Cryptography.Tests #88050

kotlarmilos opened this issue Jun 26, 2023 · 11 comments · Fixed by #88340
Labels
area-System.Security disabled-test The test is disabled in source code against the issue os-ios Apple iOS
Milestone

Comments

@kotlarmilos
Copy link
Member

kotlarmilos commented Jun 26, 2023

Build Information

Build: https://dev.azure.com/dnceng-public/public/_build/results?buildId=320749&view=results
Build error leg or test failing: iossimulator-x64 Release AllSubsets_Mono
Pull request: #88042

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Interop+AppleCrypto+AppleCommonCryptoCryptographicException : Unknown format in import",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=320749
Result validation: ✅ Known issue matched with the provided build.

Report

Build Definition Test Pull Request
325432 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
324452 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution #88167
324371 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
323893 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution #88113
323521 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
322934 dotnet/runtime System.Security.Cryptography.X509Certificates.Tests.PfxIterationCountTests_X509Certificate2.Import_BlobHasMoreThanOnePfx_LoadsOnlyOne
322681 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution #87260
322518 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution #88094
321994 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
321687 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
321510 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
321392 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
320749 dotnet/runtime System.Security.Cryptography.X509Certificates.Tests.PfxIterationCountTests_X509Certificate2Collection.Import_BlobHasMoreThanOnePfx_LoadsOnlyOne #88042
320642 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution #88036
320422 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
320144 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
0 5 16
@kotlarmilos kotlarmilos added blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' area-Codegen-AOT-mono os-ios Apple iOS Known Build Error Use this to report build issues in the .NET Helix tab labels Jun 26, 2023
@kotlarmilos kotlarmilos added this to the 8.0.0 milestone Jun 26, 2023
@ghost
Copy link

ghost commented Jun 26, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

Build Information

Build: https://dev.azure.com/dnceng-public/public/_build/results?buildId=320749&view=results
Build error leg or test failing: iossimulator-x64 Release AllSubsets_Mono
Pull request: #88042

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Interop+AppleCrypto+AppleCommonCryptoCryptographicException : Unknown format in import",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}
Author: kotlarmilos
Assignees: -
Labels:

blocking-clean-ci, area-Codegen-AOT-mono, os-ios, Known Build Error

Milestone: 8.0.0

@ghost
Copy link

ghost commented Jun 26, 2023

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

Issue Details

Build Information

Build: https://dev.azure.com/dnceng-public/public/_build/results?buildId=320749&view=results
Build error leg or test failing: iossimulator-x64 Release AllSubsets_Mono
Pull request: #88042

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Interop+AppleCrypto+AppleCommonCryptoCryptographicException : Unknown format in import",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=320749
Result validation: ✅ Known issue matched with the provided build.

Report

Build Definition Test Pull Request
320749 dotnet/runtime System.Security.Cryptography.X509Certificates.Tests.PfxIterationCountTests_X509Certificate2Collection.Import_BlobHasMoreThanOnePfx_LoadsOnlyOne #88042
320642 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution #88036
320422 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution
320144 dotnet/runtime System.Security.Cryptography.Tests.WorkItemExecution

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
4 4 4
Author: kotlarmilos
Assignees: -
Labels:

area-System.Security, blocking-clean-ci, os-ios, Known Build Error

Milestone: 8.0.0

@jozkee
Copy link
Member

jozkee commented Jun 26, 2023

This suggests to me that there could be an iOS specific bug in the Slice logic we just added in #87514:

KdfWorkLimiter.SetIterationLimit((ulong)pkcs12UnspecifiedPasswordIterationLimit);
ulong observedIterationCount = GetIterationCount(pkcs12, out int bytesConsumed);
pkcs12 = pkcs12.Slice(0, bytesConsumed);

@akoeplinger
Copy link
Member

@jozkee that change got backported to 7.0 right?

@jozkee
Copy link
Member

jozkee commented Jun 27, 2023

@akoeplinger yes, and to 6.0 and 3.1 as well.

EDIT: actually no, the Slice change is currently only on 8.0.

@jozkee
Copy link
Member

jozkee commented Jun 27, 2023

Do you think this could be a regression?

@akoeplinger
Copy link
Member

Could be, I'll try to repro it locally and see if reverting the change fixes it.

@akoeplinger
Copy link
Member

akoeplinger commented Jun 28, 2023

@jozkee I confirmed that the test failures don't happen with #87514 reverted. ... which makes sense since it's one of the tests added in that PR that fails 😄

@akoeplinger
Copy link
Member

Given it only fails Import_BlobHasMoreThanOnePfx_LoadsOnlyOne I wonder if this is just using some algorithm that isn't supported by iOS.

@akoeplinger
Copy link
Member

akoeplinger commented Jun 28, 2023

I added more logging and it fails only when trying to import the twoPfxes item, I assume this is getting rejected by the Apple SecCertificateCreateWithData API which we only use on iOS/tvOS/MacCatalyst. I think we can disable the test.

[Fact]
public void Import_BlobHasMoreThanOnePfx_LoadsOnlyOne()
{
// These certs don't use PBES2 so they should be supported everywhere.
byte[] firstPfx = TestData.Pkcs12WindowsDotnetExportEmptyPassword;
Assert.Equal("CN=test", Import(firstPfx).Subject);
byte[] secondPfx = TestData.Pkcs12Builder3DESCBCWithEmptyPassword;
Assert.Equal("CN=potato", Import(secondPfx).Subject);
byte[] twoPfxes = new byte[firstPfx.Length + secondPfx.Length];
Array.Copy(firstPfx, twoPfxes, firstPfx.Length);
Array.Copy(secondPfx, 0, twoPfxes, firstPfx.Length, secondPfx.Length);
Assert.Equal("CN=test", Import(twoPfxes).Subject);

/cc @vcsjones

@vcsjones
Copy link
Member

That's a little odd that iOS is failing two PFXes. A quick glance shows it should work.

The PKCS12 iOS PAL uses ApplePkcs12Reader.

private static AppleCertificatePal ImportPkcs12(
ReadOnlySpan<byte> rawData,
SafePasswordHandle password,
bool ephemeralSpecified)
{
using (ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData))

That uses the base class (UnixPkcs12Reader) ParsePkcs12.

internal sealed class ApplePkcs12Reader : UnixPkcs12Reader
{
internal ApplePkcs12Reader(ReadOnlySpan<byte> data)
{
ParsePkcs12(data);

Which specifically says it handles trailing data:

protected void ParsePkcs12(ReadOnlySpan<byte> data)
{
try
{
// RFC7292 specifies BER instead of DER
AsnValueReader reader = new AsnValueReader(data, AsnEncodingRules.BER);
// Windows compatibility: Ignore trailing data.
ReadOnlySpan<byte> encodedData = reader.PeekEncodedValue();

I think we should disable it with an [ActiveIssue] back to this issue so we can better understand why it doesn't appear to be working when, at first glance, it should.

steveisok pushed a commit to steveisok/runtime that referenced this issue Jun 28, 2023
@akoeplinger akoeplinger added disabled-test The test is disabled in source code against the issue and removed blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' labels Jun 28, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 3, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 5, 2023
@kotlarmilos kotlarmilos removed the Known Build Error Use this to report build issues in the .NET Helix tab label Jul 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security disabled-test The test is disabled in source code against the issue os-ios Apple iOS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants